[GH-8723] Remove use of nullability to automatically detect nullable status #8732
Conversation
| @@ -1527,14 +1523,6 @@ private function validateAndCompleteTypedAssociationMapping(array $mapping): arr | |||
| $mapping['targetEntity'] = $type->getName(); | |||
| } | |||
|
|
|||
| if (isset($mapping['joinColumns'])) { | |||
| foreach ($mapping['joinColumns'] as &$joinColumn) { | |||
| if ($type->allowsNull() === false) { | |||
There was a problem hiding this comment.
instead of removing it, do we maybe want the same logic as for $mapping['nullable']?
e.g. if (! isset($joinColumn['nullable'])) $joinColumn['nullable'] = $type->allowsNull();
There was a problem hiding this comment.
No, tthe column nullable behavior is also flawed, as there are legit cases where you want nullable behavior to be different between entity and database.
There was a problem hiding this comment.
ah, at least for me the bug was only that I could not set the value at all in the JoinColumn annotation. it was always false even when providing it explicitly true.
but alright, I see setting the default based on the php type may be a bc where it is not defined in the annotation. so maybe this is really something for orm 3?
* Mark 2.8.x as unmaintained, and 2.9.x as current * Fix ClassMetadataInfo template inference * Fix metadata cache compatibility layer * Bump doctrine/cache patch dependency to fix build with lowest deps * Add generics to parameters * Add note about performance and inheritance mapping (#8704) Co-authored-by: Claudio Zizza <[email protected]> * Adapt flush($argument) in documentation as it's deprecated. (#8728) * [GH-8723] Remove use of nullability to automatically detect nullable status (#8732) * [GH-8723] Remove use of nullability to automatically detect nullable status. * [GH-8723] Make Column::$nullable default to false again, fix tests. * Add automatic type detection for Embedded. (#8724) * Add automatic type detection for Embedded. * Inline statement. Co-authored-by: Benjamin Eberlei <[email protected]> Co-authored-by: Grégoire Paris <[email protected]> Co-authored-by: Vincent Langlet <[email protected]> Co-authored-by: Andreas Braun <[email protected]> Co-authored-by: Fran Moreno <[email protected]> Co-authored-by: Juan Iglesias <[email protected]> Co-authored-by: Claudio Zizza <[email protected]> Co-authored-by: Yup <[email protected]> Co-authored-by: Benjamin Eberlei <[email protected]>
|
A question as a user: why is it preferable to remove this as a default instead of simply allowing the user to override the nullable status whenever it misaligns with the property type? I considered the alignment with property type to be a hugely positive addition alongside the support for attributes, since it made it possible for, in a majority of cases, a simple variable declaration and single attribute to be all that is required to have a database column that corresponds intuitively to that property. That included the nullability of that property. The changelog suggests that this fixes a bug with joinColumn nullability; rather than simply not depending on the nullable status at all, is there another way to resolve this problem? |
because in an existing application that has an entity with a nullable typed php property and nullable not defined in the annotation/attribute, it changes the behavior, which is a breaking change. according to semantic versioning a bc is only allowed in a major version increment, so handling it strictly, it can not be in orm 2.x versions. let's hope we get this in 3.0, basically it's a good idea :) |
|
@zolex sounds right, but god it feels so weird to revert everything I did alongside updating to |
|
Same; in upgrading to 2.9.0 I took all the nullable specifications out in favor of the native PHP types, which helped me find numerous instances (particularly on It's definitely a step back in the name of compatibility, but I would love to see it be something that's configurable to use now. |
The new typed properties support has a flaw around the the concept of nullable. Setting a type in the entity to be or not be nullable does not necessarily mean the databsae column has to be the same. They can differ from each other.
As such it is better to remove the the use of
allowsNull()as the default value for a column's or joincolumn's nullable flag.This supersedes #8726
Fixes #8723