Repository navigation
fix(laravel): accept null on an optional belongsTo - #8666
devtechedge wants to merge 2 commits into
Conversation
f6995ca to
c1b99eb
Compare
soyuka
left a comment
soyuka
left a comment
There was a problem hiding this comment.
Thanks for the PR, a few update suggestions.
| ]; | ||
| } | ||
|
|
||
| $this->annotateBelongsToNullability($model, $columns); |
There was a problem hiding this comment.
getRelations() now only knows about nullable if getAttributes() ran before it. It's the case today (the property factory and api-platform:metadata:dump call attributes first) but nothing enforces it, if someone reorders these calls every relation silently becomes non-nullable again. Can you compute it where the relations are built so there's no ordering at all?
| /** | ||
| * Records whether a BelongsTo foreign key column is nullable, using the same | ||
| * schema column flag as plain attributes. The foreign key name comes from the | ||
| * relation, not from a guessed column. | ||
| * | ||
| * @param list<array<string, mixed>> $columns | ||
| */ | ||
| private function annotateBelongsToNullability(Model $model, array $columns): void | ||
| { | ||
| if (!isset($this->relations[$model::class])) { | ||
| return; | ||
| } | ||
|
|
||
| $nullableByColumn = []; | ||
| foreach ($columns as $column) { | ||
| if (!isset($column['name'])) { | ||
| continue; | ||
| } | ||
|
|
||
| $nullableByColumn[$column['name']] = (bool) $column['nullable']; | ||
| } | ||
|
|
||
| foreach ($this->relations[$model::class] as $name => $relation) { | ||
| if (!isset($relation['type']) || !is_a($relation['type'], BelongsTo::class, true)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
A few guards here can't happen: getAttributes() calls getRelations() first so $this->relations[$model::class] is always set ([] at worst), $column['name'] and $relation['type'] are always there (the code above uses them without checks). Can we remove these? Also please drop the docblock, the @param list<...> can stay if phpstan needs it.
| foreach ($this->modelMetadata->getRelations($data) as $relation) { | ||
| // isset() is false for null, so an explicit null would be skipped and saved as a column named after the relation. | ||
| if ( | ||
| (BelongsTo::class === $relation['type'] || MorphTo::class === $relation['type']) |
There was a problem hiding this comment.
the MorphTo branch isn't covered, dissociate() also nulls the type column there. Could you add a test for it?
| /** | ||
| * @see https://github.com/api-platform/core/issues/8664 | ||
| */ | ||
| final class Issue8664Test extends TestCase |
There was a problem hiding this comment.
We already have a nullable BelongsTo in the workbench: DeliveryRequest::pickupTimeSlot (pickup_time_slot_id is nullable). Could you use it instead of the two new models + migration? Bonus: the relation is camelCase so it also tests the method_name (used by dissociate()) vs snake_case name (used by the unset) split in the processor, category can't catch that. Then move the tests into EloquentTest or a file with a descriptive name (NullableBelongsToTest?), and the @see link once is enough. Thanks!
| $type = Type::object($relation['related']); | ||
| if ($collection) { | ||
| $type = Type::iterable($type); | ||
| } elseif (true === ($relation['nullable'] ?? false)) { |
There was a problem hiding this comment.
Note that this changes the output for every existing nullable BelongsTo: JSON Schema/OpenAPI become nullable and GraphQL drops the ! on these fields (inputs included). It's the right fix and it matches what Doctrine does but we need to mention this in the changelog.
Move the test onto the existing DeliveryRequest::pickupTimeSlot workbench relation.
|
Thanks for the careful review, these all make the change tighter 👍 Nullability is now computed inside The helper with the extra guards and its docblock is gone. The tests now live in I added a For the For the changelog, should the BC note about nullable schemas and the GraphQL |
2 participants
Thanks for the report.
An optional BelongsTo whose foreign key is nullable should accept an explicit null, the same way Doctrine already does.
EloquentPropertyMetadataFactory wraps a nullable plain column with Type::nullable(), but a relation always became Type::object() and never looked at the foreign key.
I mark that relation nullable when the foreign key column is nullable, using the column name the relation already reports.
PersistProcessor skipped a null relation because isset() is false for null, so the null stayed as a raw attribute and the insert looked for a column named after the relation.
A present null on BelongsTo or MorphTo now calls dissociate() and unsets that attribute before save.
Omitting the key still leaves the stored relation alone.
I ran the filtered Issue8664Test on PHP 8.4, and POST and PATCH with the relation set to null no longer 500, with the foreign key null afterward.
Fixes #8664