[PropertyInfo] Check if property is nullable when using ReflectionExtractor
#57708
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
OskarStark
changed the title
ReflectionExtractor
| @@ -548,6 +548,7 @@ public static function typeProvider(): iterable | |||
| yield ['collection', Type::list(Type::object(\DateTimeImmutable::class)), null, null]; | |||
| yield ['nestedCollection', Type::list(Type::list(Type::string())), null, null]; | |||
| yield ['mixedCollection', Type::array(), null, null]; | |||
| yield ['nullableTypedCollection', Type::nullable(Type::list(Type::object(Dummy::class))), null, null]; | |||
There was a problem hiding this comment.
we should expand provideLegacyTypes() in the same way to ensure that the behaviour of the old and new type system matches
There was a problem hiding this comment.
Done, amended to my last commit
| @@ -865,6 +868,7 @@ public function testTypedProperties() | |||
| $this->assertEquals(Type::list(Type::string()), $this->extractor->getType(Php74Dummy::class, 'stringCollection')); | |||
| $this->assertEquals(Type::nullable(Type::int()), $this->extractor->getType(Php74Dummy::class, 'nullableWithDefault')); | |||
| $this->assertEquals(Type::array(), $this->extractor->getType(Php74Dummy::class, 'collection')); | |||
| $this->assertEquals(Type::nullable(Type::list(Type::object(Dummy::class))), $this->extractor->getType(Php74Dummy::class, 'nullableTypedCollection')); | |||
There was a problem hiding this comment.
we should expand testTypedPropertiesLegacy() in the same way to ensure that the behaviour of the old and new type system matches
|
Yes please, maybe even 5.4 instead of 6.4 if that’s affected too. |
Done on #57802, i've updated the relevant issue to correct the affected versions |
…mMutator on CollectionType (benjilebon) This PR was merged into the 5.4 branch. Discussion ---------- [PropertyInfo] Fix nullable value returned from extractFromMutator on CollectionType | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #57707 | License | MIT Follow-up on #57708 to provide the fix for the legacy type system on the 5.4 branch Commits ------- 6d77170 [PropertyInfo] Fix nullable value returned from extractFromMutator on CollectionType
|
Thank you @benjilebon. |
When
$typewas resolved into aCollectionTypewith an adder mutator on the given$class, the replaced $type ignored if the actual property was nullable or not, causing the returned Type to always report as non nullable even when given property was declared with anullunion type or a?(?arrayfor example)See issue for more information and an example case