Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Conversation

@jeroennoten
Copy link
Contributor

QA
Branch?5.x
Bug fix?no
New feature?not really, enhancement of an existing feature
Deprecations?no
TicketsFix#32310
LicenseMIT

This PR improves the formatting of exception details in failed messenges when displayed usingmessenger: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 aThrownExceptionDetails 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 tofix#32310. I'm open for other suggestions.

@jeroennotenjeroennotenforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch fromf0e9bf0 toe58ce44CompareNovember 1, 2020 12:27
@derrabusderrabus added this to the5.x milestoneNov 1, 2020
@jeroennotenjeroennotenforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch frome58ce44 to22aed49CompareNovember 1, 2020 13:07
Copy link
Member

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

Copy link
Member

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

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

Copy link
Member

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

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.

derrabus reacted with thumbs up emoji

@yceruto
Copy link
Member

We have aCliErrorRenderer 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?

Nyholm reacted with thumbs up emoji

@jeroennoten
Copy link
ContributorAuthor

We have aCliErrorRenderer 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, theCliErrorRender is only capable of renderingThrowables, and we are not able to reconstruct aThrowable from the storend data here. I tried adding a method to theCliErrorRenderer capable of renderingFlattenException, but directly using the VarDumper made more sense to me.

@jeroennotenjeroennotenforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch 5 times, most recently from44848ff to876b9eeCompareNovember 2, 2020 09:40
@jeroennoten
Copy link
ContributorAuthor

jeroennoten commentedNov 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 replacedev-main with something else, but I have no idea with what. Could you help me out?

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

jeroennoten commentedNov 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.

@jeroennotenjeroennotenforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch from876b9ee tof90eb01CompareNovember 2, 2020 12:02
@jeroennoten
Copy link
ContributorAuthor

jeroennoten commentedNov 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-grekasnicolas-grekasforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch fromf90eb01 to4c2a198CompareNovember 2, 2020 14:21
@jeroennotenjeroennotenforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch 2 times, most recently from5a6d026 to578efd2CompareNovember 2, 2020 15:34
@jeroennoten
Copy link
ContributorAuthor

jeroennoten commentedNov 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
ContributorAuthor

Will do! Not sure why the VarDumper tests still cannot find theThrownExceptionDetails 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
ContributorAuthor

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!

@jeroennotenjeroennotenforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch 5 times, most recently from226b315 to98c36cbCompareNovember 3, 2020 07:28
@jeroennotenjeroennoten changed the title[Messenger][VarDumper] Improve formatting of exception in failed message[Messenger] Improve formatting of exception in failed messageNov 3, 2020
@jeroennoten
Copy link
ContributorAuthor

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

Copy link
Member

@nicolas-grekasnicolas-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.

@jeroennotenjeroennotenforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch from98c36cb toc3f7f88CompareNovember 3, 2020 12:22
@jeroennotenjeroennotenforce-pushed thefeature/improve-formatting-of-exception-in-failed-message branch fromc3f7f88 to2ad1addCompareNovember 3, 2020 12:22
Copy link
Member

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

Thank you@jeroennoten.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@NyholmNyholmNyholm left review comments

@derrabusderrabusderrabus requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@ycerutoycerutoyceruto approved these changes

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

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

[8]ページ先頭

©2009-2025 Movatter.jp