-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Catch \Throwable in getCacheKey() #36336
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7f404b7
to
ea24bfc
Compare
'ignored' => $this->ignoredAttributes, | ||
'camelized' => $this->camelizedAttributes, | ||
])); | ||
} catch (\Throwable $exception) { |
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.
\Exception
?
catching \Throwable
is a terrible practice, it hides coding issues and helps ppl lose their time to figure out what is broken...
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.
Not here, it's a very specific edge case, see #36332 (comment)
(We were already catch \Exception
)
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.
If we don't do that, if any value in the context contains a Doctrine entity, an error can be thrown, leading to a 500.
1b8e7f0
to
86ab77c
Compare
86ab77c
to
f4cd9a5
Compare
I opened https://bugs.php.net/79447 |
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
I've submitted php/php-src#5396 to fix the issue on the PHP side, please have a look. |
PR on php-src accepted, this is going to be fixed on PHP's side. Anyone having this issue, please upgrade to PHP >= 7.4.6. |
serialize()
can throw\Throwable
with PHP 7+, and it will if you try to serialize a Doctrine entity. As the serialization context will likely contain Doctrine entity, this must be caught.