fix(presentations): catch Throwable in PresentationMaterial::getSummitId - #603
Conversation
Removing a media upload nulls the owning presentation before Doctrine's PreRemove fires, so getSummitId() dereferences null and PHP throws \Error. The catch only covered \Exception, so the error escaped the lifecycle listener and aborted the flush, which is what breaks the Services CI job on main since #579. Catch \Throwable and fall back to 0, the degraded case PresentationMaterialEventDispatchTest already documents and asserts.
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-603/ This page is automatically updated on each push to this PR. |
…ntationId too Same shape as getSummitId(): a material whose presentation is not set dereferences null and throws \Error, which the \Exception catch did not cover. Add a test asserting both accessors return 0 in that state.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-603/ This page is automatically updated on each push to this PR. |
1 participant
Summary
mainis red since #579: theintegration-tests (Services, tests/Unit/Services/)job fails onPresentationMaterialEventDispatchTest::testDeleteMediaUploadDispatchesLifeCycleEventWithoutErrorwithError: Call to a member function getSummitId() on nullatPresentationMaterial.php:82.Root cause
Presentation::removeMediaUpload()nulls the owning presentation before Doctrine'sPreRemovefires, soPresentationMaterial::getSummitId()dereferencesnulland PHP throws\Error. The method only caught\Exception, which\Errordoes not extend, so the error escaped the lifecycle listener and aborted the flush. The test added in #579 already documents this as the accepted degraded case (summit_id0) and expects the catch to cover\Throwable.PresentationMaterial::getPresentationId()has the same shape ($this->presentation->getId()inside acatch (\Exception)) and fails the same way on a material whose presentation is not set.Change
PresentationMaterial::getSummitId()andPresentationMaterial::getPresentationId():catch (\Exception $ex)becomescatch (\Throwable $ex), falling back to0as before. These are the only two catches in the file.PresentationMaterialEventDispatchTest::testAccessorsReturnZeroWhenPresentationIsNotSet: asserts both accessors return0on a material with no presentation.Not changed:
ScheduleEntity::_getSummitId()andScheduleEntity::deleting()also catch only\Exception. Left as-is to keep the hotfix minimal.Verification
Run inside the local Docker stack:
Red/green checked: with the new test and the original production file the run reports 2 errors (
getSummitId() on nullin the delete test,getId() on nullin the new test); with the fix both pass.