[PropertyAccess] Throw an UnexpectedTypeException when the type do not match #18032
Conversation
| * @throws \Exception | ||
| */ | ||
| private function callMethod($object, $method, $value) { | ||
| if (PHP_MAJOR_VERSION >= 7) { |
There was a problem hiding this comment.
we usually prefer comparing against PHP_VERSION_ID
|
Is this really necessary? This looks like adding a lot of overhead to a very sensitive part of the component with little benefit. |
|
@webmozart the component is used in the Serializer ( class MyEntity
{
public function setFoo(string $foo)
{
// ...
}
}It currently fail with a 500 HTTP code, but it should be properly caught and throw a 400 HTTP code. |
|
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? |
|
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). |
|
@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. |
|
The heavy code (the reflection part) will be called only when a problem occurs isnt'it? |
|
wdyt about moving the type-error logic in setValue, out of the for loop? |
|
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 thrown Commits ------- 5fe2b06 [PropertyAccess] Reduce overhead of UnexpectedTypeException tracking 10c8d5e [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: