8000 [Messenger] Improve formatting of exception in failed message by jeroennoten · Pull Request #38940 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

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 4, 2020
Merged

[Messenger] Improve formatting of exception in failed message #38940

merged 1 commit into from
Nov 4, 2020

Conversation

jeroennoten
Copy link
Contributor
Q A
Branch? 5.x
Bug fix? no
New feature? not really, enhancement of an existing feature
Deprecations? no
Tickets Fix #32310
License MIT

This PR improves the formatting of exception details in failed messenges when displayed using messenger:failed:show <id> -vv.

Before:
Screen Shot 2020-11-01 at 1 05 24 PM

After:
Screen Shot 2020-11-01 at 1 03 09 PM

I created a ThrownExceptionDetails class 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.

@jeroennoten jeroennoten requested a review from sroze as a code owner November 1, 2020 12:24
@jeroennoten jeroennoten force-pushed the feature/improve-formatting-of-exception-in-failed-message branch from f0e9bf0 to e58ce44 Compare November 1, 2020 12:27
@derrabus derrabus added this to the 5.x milestone Nov 1, 2020
@jeroennoten jeroennoten force-pushed the feature/improve-formatting-of-exception-in-failed-message branch from e58ce44 to 22aed49 Compare November 1, 2020 13:07
Nyholm
Nyholm reviewed Nov 1, 2020
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a 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

Nyholm
Nyholm reviewed Nov 1, 2020
View reviewed changes
Nyholm
Nyholm reviewed Nov 1, 2020
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a 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.

@Nyholm
Copy link
Member
Nyholm commented Nov 1, 2020

@yceruto, Is it a good idea to move something like ThrownExceptionDetails to the ErrorHandler component?

derrabus
derrabus requested changes Nov 1, 2020
View reviewed changes
Copy link
Member
@derrabus derrabus left a 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
Copy link
Member
Nyholm commented Nov 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.

@Nyholm Nyholm added the Feature label Nov 1, 2020
@yceruto
Copy link
Member
yceruto commented Nov 1, 2020

We have a CliErrorRenderer class prepared to do this job, maybe we could use it instead of creating another class to format this specific exception. Could you take a look at it?

@jeroennoten
Copy link
Contributor Author

We have a CliErrorRenderer class prepared to do this job, maybe we could use it instead of creating another class to format this specific exception. Could you take a look at it?

That's what I tried first. However, the CliErrorRender is only capable of rendering Throwables, and we are not able to reconstruct a Throwable from the storend data here. I tried adding a method to the CliErrorRenderer capable of rendering FlattenException, but directly using the VarDumper made more sense to me.

@jeroennoten jeroennoten force-pushed the feature/improve-formatting-of-exception-in-failed-message branch 5 times, most recently from 44848ff to 876b9ee Compare November 2, 2020 09:40
@jeroennoten
Copy link
Contributor Author
jeroennoten commented Nov 2, 2020 •
edited
Loading

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.

@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 "symfony/var-dumper": "^5.3" to Messenger's require-dev in composer.json and added branch alias "dev-main": "5.3-dev" in the VarDumper's composer.json, but it still does not install the version including the changes in this PR. I probably need to replace dev-main with something else, but I have no idea with what. Could you help me out?

nicolas-grekas
nicolas-grekas reviewed Nov 2, 2020
View reviewed changes
@nicolas-grekas
Copy link
Member

I tried adding a method to the CliErrorRenderer capable of rendering FlattenException, but directly using the VarDumper made more sense to me.

We could use a custom caster attached to the cloner used by the messenger component.
That could prevent adding a new class.
We do this in HttpKernel's DataCollector already.

@jeroennoten
Copy link
Contributor Author
jeroennoten commented Nov 2, 2020 •
edited by nicolas-grekas
Loading

We could use a custom caster attached to the cloner used by the messenger component.

Good suggestion! I'll look into this.

@nicolas-grekas
Copy link
Member

Before pushing the update, can you please just remove the added branch-alias in the component,
and update the root composer.json of symfony/symfony by replacing 5.2 by 5.x in the branch-version entry?
I'd like to see if that will fix the CI.
Thanks.

@jeroennoten jeroennoten force-pushed the feature/improve-formatting-of-exception-in-failed-message branch from 876b9ee to f90eb01 Compare November 2, 2020 12:02
@jeroennoten
Copy link
Contributor Author
jeroennoten commented Nov 2, 2020 •
edited
Loading

Before pushing the update, can you please just remove the added branch-alias in the component,
and update the root composer.json of symfony/symfony by replacing 5.2 by 5.x in the branch-version entry?

@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?

@nicolas-grekas nicolas-grekas force-pushed the feature/improve-formatting-of-exception-in-failed-message branch from f90eb01 to 4c2a198 Compare November 2, 2020 14:21
@jeroennoten jeroennoten force-pushed the feature/improve-formatting-of-exception-in-failed-message branch 2 times, most recently from 5a6d026 to 578efd2 Compare November 2, 2020 15:34
@jeroennoten
Copy link
Contributor Author
jeroennoten commented Nov 2, 2020 •
edited
Loading

@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
Copy link
Member

The changes you pushed seem to work, all Messenger tests pass now

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
Copy link
Contributor Author

Will do! Not sure why the VarDumper tests still cannot find the ThrownExceptionDetails class though... Any idea?

@nicolas-grekas
Copy link
Member

Not sure why the VarDumper tests still cannot find the ThrownExceptionDetails class though... Any idea?

you'd need to add messenger as a dev-dep of var-dumper.

@jeroennoten
Copy link
Contributor Author

Not sure why the VarDumper tests still cannot find the ThrownExceptionDetails class though... Any idea?

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!

@jeroennoten jeroennoten force-pushed the feature/improve-formatting-of-exception-in-failed-message branch 5 times, most recently from 226b315 to 98c36cb Compare November 3, 2020 07:28
@jeroennoten jeroennoten changed the title [Messenger][VarDumper] Improve formatting of exception in failed message [Messenger] Improve formatting of exception in failed message Nov 3, 2020
@jeroennoten
Copy link
Contributor Author

@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
nicolas-grekas approved these changes Nov 3, 2020
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a 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.

@jeroennoten jeroennoten force-pushed the feature/improve-formatting-of-exception-in-failed-message branch from 98c36cb to c3f7f88 Compare November 3, 2020 12:22
@jeroennoten jeroennoten force-pushed the feature/improve-formatting-of-exception-in-failed-message branch from c3f7f88 to 2ad1add Compare November 3, 2020 12:22
yceruto
yceruto approved these changes Nov 3, 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.

Thanks!

@fabpot
Copy link
Member
fabpot commented Nov 4, 2020

Thank you @jeroennoten.

@fabpot fabpot mentioned this pull request Nov 4, 2020
Closed
@fabpot fabpot merged commit d2bac32 into symfony:5.x Nov 4, 2020
@fabpot fabpot mentioned this pull request Nov 10, 2020
Merged
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

@derrabus derrabus derrabus requested changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@yceruto yceruto yceruto approved these changes

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Feature Messenger Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

[Messenger] messenger:failed:show use ErrorHandler formatter
7 participants
@jeroennoten @Nyholm @yceruto @nicolas-grekas @fabpot @derrabus @carsonbot
0