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#18210
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
dunglas commentedMar 17, 2016
👍 |
| }catch (\TypeError$e) { | ||
| try { | ||
| self::throwUnexpectedTypeException($e->getMessage(),$e->getTrace(),0); | ||
| }catch (UnexpectedTypeException$e) { |
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.
why is it thrown to catch it again ?
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.
throwing is done conditionally (see throwUnexpectedTypeException). Thus if the method throws, $e changes, otherwise $e stays as the original TypeError. This is exactly the logic we need here.
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.
OK, I missed the rethrow below (this code should be cleaned to usefinally instead in 3.0+)
fabpot commentedMar 17, 2016
👍 |
fabpot commentedMar 18, 2016
Thank you@nicolas-grekas. |
… 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
Tobion commentedMar 19, 2016
Isn't this a BC break? PHP 7 code that previously caught TypeError will not work anymore, or am I missing something? |
Made in coordination with@dunglas,
Diff best viewedignoring whitspaces.
Quoting#18032: