[biobank] Fix fatal errors on shipment submission#10882
Conversation
Guard CenterID construction in the Shipment and Log entity constructors. When a shipment or log was submitted without a center (e.g. no containers selected, so no derived centerId), the raw null was passed straight into the CenterID constructor, which requires a non-null string, throwing a TypeError before the handler's validation could run. Construction is now guarded with isset so a null center is stored as null and caught by the existing validator, which returns a clean field-level error to the user. Also remove the #[\Override] attribute from Shipments::isAccessibleBy, which has no matching parent method and caused a fatal error under PHP 8.3's override enforcement.
|
@HenriRabalais . Here's a 1 minute video. Your changes work so with the integration tests passed you'll get my LGTM Kooha-2026-07-13-21-24-00.mp4 |
|
There are other issues :like temperature can be a string value. Should it be contrained to an integer value and set to Celcius? |
|
Also: why does |
|
The other issue ": I entered something in the container field and the error message showed "at least 1 container must be selected" which is not intuitive behaviour, given that I entered something. I also don't know what I am supposed to enter in terms of container. Should it not be a list. I know these issues are off topic. Should I make new issues for each? lmk |
| * | ||
| * @return bool | ||
| */ | ||
| #[\Override] |
There was a problem hiding this comment.
This should not be removed. #[\Override] means that it overrides or implements an interface and isAccessibleBy comes from the AccessibleResource interface.
There was a problem hiding this comment.
right, for whatever reason I was still on php 8.3 so it was giving me trouble
Fixed in #10801. @HenriRabalais you might want to link |
HenriRabalais
added
Module: biobank
|
@adamdaudrich the container field on the left functions as a loader for the list on the right. The primary workflow involves users scanning container barcodes, at which point they appear in the list on the right. You can go to the container tab and copy a barcode from there to test it out. I think for biobank workflow suggestions, perhaps you can make a separate issue and we can tackle them there? |
|
@HenriRabalais |
4 participants
Brief summary of changes
Fixes two fatal errors in the biobank shipments module that occurred when submitting a shipment.
The main bug: when a shipment was submitted without a center (for example when no containers were selected, so no
centerIdcould be derived on the frontend) the rawnullwas passed straight into theCenterIDconstructor.CenterIDextendsValidatableIdentifier, whose constructor requires a non-null string, so this threw aTypeErrorand returned a 500 before the shipment handler's validation could run. Because construction happened before validation, the existing_validate()logic was never reached.The
CenterIDconstruction in both theShipmentandLogentity constructors is now guarded withisset, so a null center is stored asnulland caught by the existing validator, which returns the intended field-level error to the user rather than a fatal.DateTimeconstruction inLogwas guarded the same way, since it had the same latent issue with null date/time values.Separately, the
#[\Override]attribute was removed fromShipments::isAccessibleBy(). The method has no matching parent method, so under PHP 8.3's override enforcement it threw its own fatal error on every request to the module.Resolves #10761 and #10183 — the non-intuitive save behaviour in that issue was this failure path: submitting without containers threw the fatal instead of showing a graceful error. With this fix the submission returns the proper field validation errors.
Testing instructions