Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fatal error in DownloadController #3154

Merged
merged 1 commit into from
Dec 22, 2023
Merged

Conversation

glye
Copy link
Member

@glye glye commented Nov 9, 2023

Regression from IBEXA-SA-2023-005

Question Answer
JIRA issue IBX-7021
Type bug
Target eZ Platform version v2.5
BC breaks no

Fatal error "undefined method getId" on download. Regression from IBEXA-SA-2023-005:
https://developers.ibexa.co/security-advisories/ibexa-sa-2023-005-vulnerabilities-in-solr-search-and-file-downloads

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

Regression from IBEXA-SA-2023-005
Copy link

sonarcloud bot commented Nov 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@glye glye requested a review from a team November 17, 2023 17:12
@adamwojs adamwojs requested a review from a team November 18, 2023 06:04
@@ -63,7 +63,7 @@ public function downloadBinaryFileByIdAction(Request $request, int $contentId, i
protected function findFieldInContent(int $fieldId, Content $content): Field
{
foreach ($content->getFields() as $field) {
if ($field->getId() === $fieldId) {
if ($field->id === $fieldId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally I would request to add missing getId method, but given this is unmaintained 2.5 fix and it can go into 2.5 only, I'll allow it. Magic is bad because of performance (from 0.2s to 5s when it's scaled to e.g 30 000 calls, which is not uncommon on simple properties).
Here we can leave it as-is.

@alongosz alongosz requested a review from a team November 20, 2023 12:16
@alongosz
Copy link
Member

@glye seems this one got forgotten. Are those CI issues something we can fix?

@glye
Copy link
Member Author

glye commented Nov 28, 2023

@alongosz Not forgotten, it was sent to QA some time ago, but I see I must fix the labels here.

"The operation was cancelled" in the REST test I have no idea about. The constant time auth test in AppVeyor I should find time to look into.

@alongosz
Copy link
Member

@alongosz Not forgotten, it was sent to QA some time ago, but I see I must fix the labels here.

"The operation was cancelled" in the REST test I have no idea about. The constant time auth test in AppVeyor I should find time to look into.

Oh. My bad, I missed it. Thanks for clarifying and fixing labels. Cancelled operation usually means that there were not enough resources and job exceeded time limit (unless cancelled manually). I restarted it, let's see if there are issues. We don't worry about AppVeyor, it's very old CI for Windows support. We don't have it on maintained branches anymore.

@glye
Copy link
Member Author

glye commented Nov 28, 2023

@alongosz It was cancelled again, at the same point as before. (When I run it locally it runs to the end, though with errors, so many tests are skipped.) On 7.5 branch it's been cancelling since 957e67a
Trying a PR to revert the phpunit changes from that commit... #3155

@alongosz
Copy link
Member

alongosz commented Nov 28, 2023

@glye REST Functional tests are executed using eZ/Bundle/EzPublishRestBundle/Tests/Functional directory only, neither of the changed files #3155 affect that. The issue is simple - REST test suite is running > 10min while the timeout is 10min. If it was like that before, then we can skip it for 7.5. This branch is past EOM. I'll need to look into this, but it's not a top priority.

@micszo micszo self-assigned this Dec 21, 2023
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and retested on eZ Platform 2.5.32 / eZ Commerce 2.5.21.

@micszo micszo removed their assignment Dec 22, 2023
@alongosz alongosz merged commit 4e93c5b into 7.5 Dec 22, 2023
29 of 31 checks passed
@alongosz alongosz deleted the fatal_error_on_download branch December 22, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants