[Messenger] Improve formatting of exception in failed message #38940
Conversation
f0e9bf0 to
e58ce44
Compare
e58ce44 to
22aed49
Compare
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php
Outdated
Show resolved
Hide resolved
|
@yceruto, Is it a good idea to move something like |
|
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. |
|
We have a |
That's what I tried first. However, the |
44848ff to
876b9ee
Compare
@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
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Exception/ThrownExceptionDetails.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/VarDumper/Tests/Caster/ExceptionCasterTest.php
Outdated
Show resolved
Hide resolved
We could use a custom caster attached to the cloner used by the messenger component. |
Good suggestion! I'll look into this. |
|
Before pushing the update, can you please just remove the added branch-alias in the component, |
876b9ee to
f90eb01
Compare
@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 to
4c2a198
Compare
5a6d026 to
578efd2
Compare
|
@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. |
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. |
|
Will do! Not sure why the VarDumper tests still cannot find the |
you'd need to add messenger as a dev-dep of var-dumper. |
Right, of course, sorry, was confusing it with the ErrorHandler component for some reason. Thanks! |
226b315 to
98c36cb
Compare
|
@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? |
src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php
Outdated
Show resolved
Hide resolved
98c36cb to
c3f7f88
Compare
c3f7f88 to
2ad1add
Compare
|
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 to fix #32310. I'm open for other suggestions.