Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Improve formatting of exception in failed message#38940
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Messenger] Improve formatting of exception in failed message#38940
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f0e9bf0 toe58ce44Comparee58ce44 to22aed49Compare
Nyholm left a comment
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.
Wow, thank you. I like how simple this solution is.
Good work
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Nyholm left a comment
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.
Just some minor things.
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Nyholm commentedNov 1, 2020
@yceruto, Is it a good idea to move something like |
derrabus left a comment
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.
Please have a look at the Travis php 7.4 run. The failures looks related to your changes.
Nyholm commentedNov 1, 2020
I think the Travis error in 7.4 is because we need to require symfony/var-dumper:5.3 in messenger's composer.json's require-dev. We also need to update branch alias is VarDumps's composer.json. |
yceruto commentedNov 1, 2020
We have a |
jeroennoten commentedNov 1, 2020
That's what I tried first. However, the |
44848ff to876b9eeComparejeroennoten commentedNov 2, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Nyholm that's indeed the reason with the test fails on Travis: the version of symfony/var-dumper used in the tests of symfony/messenger doesn't include the changes I made to the VarDumper component in this PR. I added |
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/VarDumper/Tests/Caster/ExceptionCasterTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedNov 2, 2020
We could use a custom caster attached to the cloner used by the messenger component. |
jeroennoten commentedNov 2, 2020 • edited by nicolas-grekas
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by nicolas-grekas
Uh oh!
There was an error while loading.Please reload this page.
Good suggestion! I'll look into this. |
nicolas-grekas commentedNov 2, 2020
Before pushing the update, can you please just remove the added branch-alias in the component, |
876b9ee tof90eb01Comparejeroennoten commentedNov 2, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas thanks for you help. Unfortunately, that doesn't seem to work either, see the Travis build. What do you want me to do? Make other composer.json changes or push the suggested changes that do not involve modifications in the VarDumper componed? |
f90eb01 to4c2a198Compare5a6d026 to578efd2Comparejeroennoten commentedNov 2, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas The changes you pushed seem to work, all Messenger tests pass now. The only thing that is missing to make the VarDumper tests pass as well was a dev-depencency version bump of symfony/messenger to "^5.3" in the VarDumper's composer.json. I added that one. Let me know when I can remove all unrelated composer.json changes from this MR and update the code to a change in only the Messenger component. |
nicolas-grekas commentedNov 2, 2020
yep, thanks for giving us a good use case to fix this :) This is now merged into 4.4 up to master, you can rebase, and you can also replace the implementation completely with the custom caster. |
jeroennoten commentedNov 2, 2020
Will do! Not sure why the VarDumper tests still cannot find the |
nicolas-grekas commentedNov 2, 2020
you'd need to add messenger as a dev-dep of var-dumper. |
jeroennoten commentedNov 2, 2020
Right, of course, sorry, was confusing it with the ErrorHandler component for some reason. Thanks! |
226b315 to98c36cbComparejeroennoten commentedNov 3, 2020
@nicolas-grekas I made the changes you suggested resulting in only one changed source file and one changed test file. Could somebody please remove the VarDumper label from this PR? |
nicolas-grekas left a comment
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.
Much nicer :)
Just one minor suggestion and good to go on my side.
src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
98c36cb toc3f7f88Comparec3f7f88 to2ad1addCompare
yceruto left a comment
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.
Thanks!
fabpot commentedNov 4, 2020
Thank you@jeroennoten. |
This PR improves the formatting of exception details in failed messenges when displayed using
messenger:failed:show <id> -vv.Before:

After:

I created a
ThrownExceptionDetailsclass which will be displayed as a normal exception when dumped with the VarDumper component. Not sure if this is the right way to do it and if the class is in the right namespace, but this is the best solution I could came up with tofix#32310. I'm open for other suggestions.