[PropertyInfo] Fix edge cases in ReflectionExtractor #20154
Conversation
4d4b5dd to
6a000dc
Compare
| $phpType = Type::BUILTIN_TYPE_OBJECT; | ||
| $typeClass = $typeHint->name; | ||
| if ($phpType) { | ||
| $collectionValueType = new Type($phpType, $nullable, $typeClass); |
There was a problem hiding this comment.
the behavior for $nullable does not match the previous code, which was using a $collectionNullable variable. $nullable is weird the collection itself is nullable, not whether collection values are
There was a problem hiding this comment.
The $nullable behavior change is related to the same semantic mistake (no offense to anyone) fixed in #20152 (there are no two "allowsNull" & "isNullable" separate concepts).
$collectionNullable === $nullable && $arrayMutator, which is what the code does.
Or did you spot something else? Any demo case where you think the adjusted behavior is wrong?
| @@ -69,7 +69,7 @@ public function typesProvider() | |||
| array('b', array(new Type(Type::BUILTIN_TYPE_OBJECT, true, 'Symfony\Component\PropertyInfo\Tests\Fixtures\ParentDummy'))), | |||
| array('c', array(new Type(Type::BUILTIN_TYPE_BOOL))), | |||
| array('d', array(new Type(Type::BUILTIN_TYPE_BOOL))), | |||
| array('e', null), | |||
| array('e', array(new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, new Type(Type::BUILTIN_TYPE_INT)))), | |||
There was a problem hiding this comment.
Maybe am I missing something but the e virtual property is defined only by a adder and a remover. We know for sure that it's a collection, but we cannot know if it's an indexed array or any other type of collection.
There was a problem hiding this comment.
Technically, it's an iterable, but as it's a 7.1+ only feature, I don't think that it's ok to use it in Symfony for now.
There was a problem hiding this comment.
After double checking, I've an explanation for this test. It is not related at all with the type of the property, but with the type of elements of its collection.
Some context: in the default setup of the standard edition, the ReflectionExtractor is registered with a higher priority than the PhpDocExtractor because it is faster; but it is also less precise.
The remover for the f property is type hinted with \DateTime. It's a reliable source of information. Regardless the type defined in the PHPDoc of the $e property, elements of the collections will be of type \DateTime (it's ensured at runtime because of the type hint).
The ReflectionExtractor is able to guess that f is a collection of \DateTime objects and return this type.
On the other hand, the adder of e isn't type hinted. The ReflectionExtractor cannot guess the type of elements of the collection. But the PhpDocExtractor may have a chance to guess it if the PHPDoc is defined for the e property. So ReflectionExtractor returns null in this case to give a chance to the PhpDocExtractor (or any other extractor registered with a lower property) to detect the type of elements.
This change is a (small) BC break, I think that the old behavior is preferable.
I'm not sure that my explanation is clear. Let me know if I need to add an example.
6a000dc to
61fac70
Compare
There was a problem hiding this comment.
@dunglas understood, behavior fixed. Should be good now.
As a side note, we don't handle "iterable" yet. By looking at the RFC, iterable is the same as array|Traversable, which could mean no need for a new builtin type, and manage it as array|Traversable instead? (Just throwing the idea, that's for another PR)
| if (!$reflectionType->isBuiltin() && Type::BUILTIN_TYPE_ARRAY === $type->getBuiltinType()) { | ||
| return; | ||
| } | ||
| } elseif (preg_match('/^(?:[^ ]++ ){4}([a-zA-Z_\x7F-\xFF][^ ]++)/', $reflectionParameter, $info)) { |
There was a problem hiding this comment.
By using the string representation of the parameter to get the type hint (on PHP5 only of course), we prevent autoloading of the corresponding class.
| if (Type::BUILTIN_TYPE_ARRAY === $phpTypeOrClass) { | ||
| $type = new Type(Type::BUILTIN_TYPE_ARRAY, $nullable, null, true); | ||
| } elseif ('void' === $phpTypeOrClass) { | ||
| $type = new Type(Type::BUILTIN_TYPE_NULL, $nullable); |
There was a problem hiding this comment.
missing "void" handling added here also
There was a problem hiding this comment.
Can you add a test for this new one?
|
👍 (but the new test for |
61fac70 to
bffdfad
Compare
|
... and one more bug fixed, thank you Nicolas. |
…las-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [PropertyInfo] Fix edge cases in ReflectionExtractor | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This should make ReflectionExtractor a bit more robust. ping @dunglas (~~don't miss the changed test case~~). Commits ------- bffdfad [PropertyInfo] Fix edge cases in ReflectionExtractor
This should make ReflectionExtractor a bit more robust.
ping @dunglas (
don't miss the changed test case).