Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Add check of internal type to ContainerBuilder::getReflectionClass#27025
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
| returnnull; | ||
| } | ||
| if ($this->isInternalType($class)) { |
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.
I'd suggest to use a private static array instead, and put types as keys in the array, so that checking could be done inline with a simple isset()
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.
Oh, you are right. It would be much better.
| 'array', | ||
| 'null', | ||
| 'callable', | ||
| 'iterable', |
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.
I'd suggest to add "mixed" to the list: even if it's not an official one, it's pretty common in phpdoc so might be OK to use it 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.
After some reflections I agree with you :)
9964403 to8c3c060Compare
nicolas-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.
(for 3.4)
stof commentedApr 24, 2018
Thank you@upyx. |
…flectionClass (upyx)This PR was submitted for the 4.0 branch but it was merged into the 3.4 branch instead (closes#27025).Discussion----------[DI] Add check of internal type to ContainerBuilder::getReflectionClass| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27018| License | MIT| Doc PR | noThis PR fixes bug described in#27018 issue.I think that `getReflectionClass` should return `NULL` when internal type was used instead class name but not throws exception. As well as it does when class is empty. If someone have other opinion we can discuss it.Commits-------314e881 [DI] Add check of internal type to ContainerBuilder::getReflectionClass
upyx commentedApr 24, 2018
Thank you for your review! |
chalasr commentedApr 24, 2018
Congratz for your first contribution to Symfony@upyx :) |
Uh oh!
There was an error while loading.Please reload this page.
This PR fixes bug described in#27018 issue.
I think that
getReflectionClassshould returnNULLwhen internal type was used instead class name but not throws exception. As well as it does when class is empty. If someone have other opinion we can discuss it.