Skip to content

[Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText #39004

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 10, 2020
Merged

[Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText #39004

merged 1 commit into from
Nov 10, 2020

Conversation

Jean85
Copy link
Contributor

@Jean85 Jean85 commented Nov 5, 2020 •
edited
Loading

Q A
Branch? 5.x (bugfix of a 5.x-only feature)
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #39003
License MIT

WIP This is now complete and, thanks to @yceruto, I've fixed two bugs in this PR:

  • ErrorDetailsStamp couldn't be (de)serialized properly with that constructor argument
  • FlattenException::$statusText wasn't (de)normalized

@Jean85
Copy link
Contributor Author

Jean85 commented Nov 5, 2020

@Nyholm you said in #38792 (comment) that we don't use FlattenException anymore, but it's still present inside the ErrorDetailsStamp, and it still doesn't deserialize well, as the current test failure highlights:

Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
         'message' => 'exception message'
         'code' => 0
         'previous' => null
-        'trace' => Array (...)
-        'traceAsString' => '#0 /home/alai/workspace/OSS/s...{main}'
+        'trace' => null
+        'traceAsString' => null
         'class' => 'Exception'
         'statusCode' => 500
         'statusText' => 'Internal Server Error'
@@ @@
         'headers' => Array ()
         'file' => '/home/alai/workspace/OSS/symf...st.php'
         'line' => 52
-        'asString' => null
+        'asString' => 'Exception: exception message ...{main}'
     )
 )

What should I do?

@jderusse jderusse added this to the 5.2 milestone Nov 5, 2020
@Nyholm
Copy link
Member

Nyholm commented Nov 6, 2020

@yceruto could you have a look at this PR please?

@yceruto
Copy link
Member

yceruto commented Nov 6, 2020

There is a FlattenExceptionNormalizer to serialize/deserialize it correctly, and alternatively you could use PropertyNormalizer too.

Did you find a new problem after this fix #38941?

@Jean85 Jean85 marked this pull request as ready for review November 9, 2020 07:59
@Jean85 Jean85 requested a review from sroze as a code owner November 9, 2020 07:59
@Jean85 Jean85 changed the title [Messenger] Fix JSON deserialization of ErrorDetailsStamp [Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText Nov 9, 2020
@Jean85 Jean85 requested a review from yceruto November 9, 2020 08:07
yceruto
yceruto requested changes Nov 9, 2020
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are still failing, please take a look. Thanks!

src/Symfony/Component/Messenger/Stamp/ErrorDetailsStamp.php Outdated
*/
public function __construct(
string $exceptionClass,
$exceptionCode,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was documented as int|mixed, probably for the same reason as vimeo/psalm#3175

@carsonbot carsonbot changed the title [Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText [Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText Nov 9, 2020
@Jean85 Jean85 requested a review from yceruto November 9, 2020 15:36
@carsonbot carsonbot changed the title [Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText [Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText Nov 9, 2020
@carsonbot carsonbot changed the title [Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText [Messenger] Fix JSON deserialization of ErrorDetailsStamp and normalization of FlattenException::$statusText Nov 9, 2020
@yceruto yceruto removed the BC Break label Nov 10, 2020
@fabpot fabpot force-pushed the fix-error-details-stamp-serialization branch from b5f9889 to 9af554c Compare November 10, 2020 06:17
@fabpot
Copy link
Member

fabpot commented Nov 10, 2020

Thank you @Jean85.

@fabpot fabpot merged commit c721e02 into symfony:5.x Nov 10, 2020
@fabpot fabpot mentioned this pull request Nov 10, 2020
Merged
@wucdbm wucdbm mentioned this pull request Dec 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm left review comments

@yceruto yceruto yceruto approved these changes

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Bug Messenger Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

[Messenger | ErrorHandler] JSON serialization + ErrorDetailsStamp doesn't seem to deserialize correctly
6 participants
@Jean85 @Nyholm @yceruto @fabpot @jderusse @carsonbot