[PropertyAccess] Throw an InvalidArgumentException when the type do not match #17738
Conversation
|
|
||
| try { | ||
| $object->{$method}($value); | ||
| } finally { |
There was a problem hiding this comment.
This may cause issues with PHP < 5.6 (a.k.a 5.5), which have a bugged implementation of finally.
There was a problem hiding this comment.
|
CI errors look unrelated. |
| @@ -43,7 +43,7 @@ | |||
| * @param string|PropertyPathInterface $propertyPath The property path to modify | |||
| * @param mixed $value The value to set at the end of the property path | |||
| * | |||
| * @throws Exception\InvalidArgumentException If the property path is invalid | |||
| * @throws Exception\InvalidArgumentException If the property path is invalid or the value type do not match with the setter type hint | |||
There was a problem hiding this comment.
Small typo : does not match instead of do not match
| } | ||
|
|
||
| // PHP 5 | ||
| set_error_handler(function ($errno, $errstr) { |
There was a problem hiding this comment.
Sorry for disturbing.
Maybe better to use something like this:
set_error_handler(function ($errno, $errstr) use ($object, $method) {
if (E_RECOVERABLE_ERROR === $errno && false !== strpos($errstr, sprintf('passed to %s::%s() must', get_class($object), $method))) {
throw new InvalidArgumentException($errstr);
}
return false;
});
There was a problem hiding this comment.
I prefer to not test the error message. It's not clean and it still does not handle all cases. Throwing an error (and not an exception) in a mutator should be very very rare in a real code.
There was a problem hiding this comment.
What cases it does not handle?
If found no other messages in PHP source that match used pattern:
https://github.com/php/php-src/blob/be607e724c69c92f8cda72a45a13a26e7c439aec/Zend/zend_execute.c#L644
https://github.com/php/php-src/blob/be607e724c69c92f8cda72a45a13a26e7c439aec/Zend/zend_execute.c#L648
https://github.com/php/php-src/blob/be607e724c69c92f8cda72a45a13a26e7c439aec/Zend/zend_execute.c#L651
And it seems that message is same for all supported PHP 5 versions: https://3v4l.org/VZhkg
I prefer to use more bullet proof solution than relying that something wont happen
There was a problem hiding this comment.
A user can trigger an error with the same or a similar message.
What do you think about that @symfony/deciders
There was a problem hiding this comment.
Users are not supposed to trigger errors with E_RECOVERABLE_ERROR type (there are specific ones with USER_...). So if there are other E_RECOVERABLE_ERROR errors that could be triggered by PHP here, we need to make sure only type errors are handled.
|
I'm not sure the current So I would propose to not cach |
|
@Tobion I like your approach. It's now implemented like that. ping @symfony/deciders |
| * @throws \TypeError | ||
| * @throws \Exception | ||
| */ | ||
| private function callMethod($object, $method, $value) { |
|
Thank you @dunglas. |
… 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 PR was squashed before being merged into the 3.1-dev branch (closes #17959). Discussion ---------- [Serializer] Harden the ObjectNormalizer | Q | A | ------------- | --- | Branch | master | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Transform `\TypeError`s to catchable serializer exceptions. Follows #17738 and completes #17660. Commits ------- 26a07fb [Serializer] Harden the ObjectNormalizer
Currently, when the Property Access Component call a setter with a value not matching its typehint, a
\TypeErroris thrown with PHP 7 and aPHP Catchable fatal errorwith PHP 5.This PR make the component returning an
InvalidArgumentExceptionwith both version. It's a (better) alternative to #17660 (the hardening part) to make the Symfony Serializer (and probably many other pieces of code) more robust when types do not match./cc @csarrazi @mRoca @blazarecki