Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyAccess] Throw an UnexpectedTypeException when the type do not match#18032
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
| * @throws \Exception | ||
| */ | ||
| privatefunctioncallMethod($object,$method,$value) { | ||
| if (PHP_MAJOR_VERSION >=7) { |
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.
we usually prefer comparing against PHP_VERSION_ID
webmozart commentedMar 7, 2016
Is this really necessary? This looks like adding a lot of overhead to a very sensitive part of the component with little benefit. |
dunglas commentedMar 7, 2016
@webmozart the component is used in the Serializer ( class MyEntity{publicfunctionsetFoo(string$foo) {// ... }} It currently fail with a 500 HTTP code, but it should be properly caught and throw a 400 HTTP code. |
webmozart commentedMar 7, 2016
What about doing the check and setting the error handler in the serializer (or at some other, higher level) instead of doing it for every single property access? |
dunglas commentedMar 7, 2016
Of course we can do that but IMO it's the responsibility of the PropertyAccess component to do that (it can be useful in many contexts). |
webmozart commentedMar 7, 2016
@dunglas Technically I agree. My concern is that the code in question can be called a considerable number of times for big forms. I think we should avoid adding (even more) code here unless absolutely necessary. |
dunglas commentedMar 7, 2016
The heavy code (the reflection part) will be called only when a problem occurs isnt'it? |
nicolas-grekas commentedMar 7, 2016
wdyt about moving the type-error logic in setValue, out of the for loop? |
nicolas-grekas commentedMar 17, 2016
Replaced by#18210 |
… type do not match (dunglas, nicolas-grekas)This PR was merged into the 2.3 branch.Discussion----------[PropertyAccess] Throw an UnexpectedTypeException when the type do not match| Q | A| ------------- | ---| Branch? | 2.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#17738,#18032| License | MIT| Doc PR | -Made in coordination with@dunglas,Diff best viewed [ignoring whitspaces](https://github.com/symfony/symfony/pull/18210/files?w=1).Quoting#18032:> it appears that the current implementation is error prone because it throws a `\TypeError` that is an `Error` in PHP 7 but a regular `Exception` in PHP 5 because it uses the Symfony polyfill.Programs wrote in PHP 5 and catching all exceptions will catch this "custom" `\TypeError ` but not those wrote in PHP 7. It can be very hard to debug.> In this alternative implementation:> * catching type mismatch is considered a bug fix and applied to Symfony 2.3> * for every PHP versions, a domain exception is thrownCommits-------5fe2b06 [PropertyAccess] Reduce overhead of UnexpectedTypeException tracking10c8d5e [PropertyAccess] Throw an UnexpectedTypeException when the type do not match
This an alternative to#17738.
After discussing with@nicolas-grekas, it appears that the current implementation is error prone because it throws a
\TypeErrorthat is anErrorin PHP 7 but a regularExceptionin PHP 5 because it uses the Symfony polyfill.Programs wrote in PHP 5 and catching all exceptions will catch this "custom"
\TypeErrorbut not those wrote in PHP 7. It can be very hard to debug.In this alternative implementation: