[Messenger] PhpSerializer: TypeError should throw MessageDecodingFailedException
#53183
Conversation
3f55e5e to
7c777ed
Compare
|
Should it target version |
MessageDecodingFailedException
src/Symfony/Component/Messenger/Tests/Transport/Serialization/PhpSerializerTest.php
Outdated
Show resolved
Hide resolved
...ponent/Messenger/Tests/Transport/Serialization/PhpSerializerWithClassNotFoundSupportTest.php
Outdated
Show resolved
Hide resolved
No 8000 t sure, for me this is a feature and should target 7.1 @nicolas-grekas @fabpot please decide |
|
Changing the type of thrown exceptions is not a bug fix so that's for 7.1 to me. |
|
@nicolas-grekas I think it should have been handled since the beginning because it's an unserialization issue that currently ends with generic kind of exception instead of I think I do not understand what's the BC break or added functionality here. For me that's a better error handling logic. Anyway I will let you know when it's ready for 7.1 👍 If you can explain me further why it's not bugfix I would be glad 😊 |
|
Any chance to get some new feedbacks based on my last comment before rebasing? |
src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php
Outdated
Show resolved
Hide resolved
df6786e to
ed65b77
Compare
6acc9d8 to
d896fbf
Compare
|
|
||
| namespace Symfony\Component\Messenger\Tests\Fixtures; | ||
|
|
||
| class DummyMessageTyped implements DummyMessageInterface |
There was a problem hiding this comment.
I had to add a new object with typed property to assess the behavior of the test.
Can be removed with Symfony 6.0+
| @@ -99,6 +100,22 @@ public function testNonUtf8IsBase64Encoded() | |||
| $this->assertTrue((bool) preg_match('//u', $encoded['body']), 'Encodes non-UTF8 payloads'); | |||
| $this->assertEquals($envelope, $serializer->decode($encoded)); | |||
| } | |||
|
|
|||
| /** | |||
| * @requires PHP >= 7.4 | |||
There was a problem hiding this comment.
Requirement can be removed with Symfony 6.0+
d896fbf to
18e0030
Compare
|
@nicolas-grekas @OskarStark thanks for the review, It's ready for 5.4 👍 Let me know for any other updates that should be performed. |
…edException Actually, the fix should handle more cases than only TypeError.
18e0030 to
ebe5c3a
Compare
|
Thank you @B-Galati. |
|
Thank @nicolas-grekas! BTW, I think we could prevent message losses upon That could be implemented with either a new What do you think? It would fix #44117 I implemented such a behavior for my company thanks to a custom transport but I would prefer to have the behavior built in Symfony. |
| if (__FILE__ === $file) { | ||
| throw $signalingException; | ||
| throw new \ErrorException($msg, 0, $type, $file, $line); |
There was a problem hiding this comment.
Thanks that's going to be more debug-able like this 👍
|
I don't know, please follow up on an open issue or a PR :) |
At the moment, some unserialization issues don't throw a
MessageDecodingFailedExceptionwhich prevent transport to trigger their decoding failure logic.There are many possibilities to fix the issue. I've implemented a very conservative one. I believe we could simplify the code by a lot if we rely on the global error handler in user land.