Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyInfo] Fix edge cases in ReflectionExtractor#20154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4d4b5dd to6a000dcCompare| $phpType = Type::BUILTIN_TYPE_OBJECT; | ||
| $typeClass =$typeHint->name; | ||
| if ($phpType) { | ||
| $collectionValueType =newType($phpType,$nullable,$typeClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
nicolas-grekasOct 4, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The$nullable behavior change is related to the same semantic mistake (no offense to anyone) fixed in#20152 (there areno 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?
| array('c',array(newType(Type::BUILTIN_TYPE_BOOL))), | ||
| array('d',array(newType(Type::BUILTIN_TYPE_BOOL))), | ||
| array('e',null), | ||
| array('e',array(newType(Type::BUILTIN_TYPE_ARRAY,false,null,true,newType(Type::BUILTIN_TYPE_INT)))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe am I missing something but thee 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Technically, it's aniterable, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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, theReflectionExtractor is registered with a higher priority than thePhpDocExtractor because it is faster; but it is also less precise.
The remover for thef 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).
TheReflectionExtractor is able to guess thatf is a collection of\DateTime objects and return this type.
On the other hand, the adder ofe isn't type hinted. TheReflectionExtractor cannot guess the type of elements of the collection. But thePhpDocExtractor may have a chance to guess it if the PHPDoc is defined for thee property. SoReflectionExtractor returnsnull in this case to give a chance to thePhpDocExtractor (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 to61fac70Comparenicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 asarray|Traversable, which could mean no need for a new builtin type, and manage it asarray|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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 =newType(Type::BUILTIN_TYPE_ARRAY,$nullable,null,true); | ||
| }elseif ('void' ===$phpTypeOrClass) { | ||
| $type =newType(Type::BUILTIN_TYPE_NULL,$nullable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing "void" handling added here also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add a test for this new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Test added
dunglas commentedOct 6, 2016
👍 (but the new test for |
61fac70 tobffdfadComparedunglas commentedOct 6, 2016
... 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
Uh oh!
There was an error while loading.Please reload this page.
This should make ReflectionExtractor a bit more robust.
ping@dunglas (
don't miss the changed test case).