8000 [Messenger] add handler description as array key to `HandlerFailedException::getWrappedExceptions()` by kbond · Pull Request #51331 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] add handler description as array key to HandlerFailedException::getWrappedExceptions() #51331

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
Oct 25, 2023
Merged

[Messenger] add handler description as array key to HandlerFailedException::getWrappedExceptions() #51331

merged 1 commit into from
Oct 25, 2023

Conversation

kbond
Copy link
Member
@kbond kbond commented Aug 9, 2023
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

Currently, when looking at HandlerFailedException to see what exceptions were thrown for a message, you can't see what handler caused the exception.

@carsonbot carsonbot added this to the 6.4 milestone Aug 9, 2023
@kbond kbond mentioned this pull request Sep 19, 2023
Merged
nicolas-grekas
nicolas-grekas reviewed Sep 20, 2023
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.

can this be tested somehow?

@kbond kbond force-pushed the exception-handler-name branch from 97e3fea to 61a5a06 Compare September 20, 2023 21:36
@kbond
Copy link
Member Author
kbond commented Sep 20, 2023

can this be tested somehow?

Test added.

@fabpot
Copy link
Member
fabpot commented Oct 1, 2023

Should be rebased (see #51653)

@kbond kbond force-pushed the exception-handler-name branch from 61a5a06 to 84abc0d Compare October 2, 2023 12:40
@kbond
Copy link
Member Author
kbond commented Oct 2, 2023

Rebased.

@kbond kbond force-pushed the exception-handler-name branch 2 times, most recently from 3e34ef0 to 7c7fb54 Compare October 2, 2023 13:09
@kbond
Copy link
Member Author
kbond commented Oct 2, 2023

Psalm errors fixed.

@kbond kbond force-pushed the exception-handler-name branch from 7c7fb54 to c2650c1 Compare October 2, 2023 13:46
@chalasr
Copy link
Member
chalasr commented Oct 2, 2023

What about reverting the changes made to the now deprecated getNestedExceptions() to update the newly introduced API only?

@kbond
Copy link
Member Author
kbond commented Oct 2, 2023

What about reverting the changes made to the now deprecated getNestedExceptions() to update the newly introduced API only?

You mean throw an array_values() into getNestedExceptions() before returning?

Another thing, it would be nice if DelayedMessageHandlingException (which implements this new interface) could provide handlers names as array keys as well but.. I don't think this is possible?

@chalasr
Copy link
Member
chalasr commented Oct 2, 2023 •
edited
Loading

You mean throw an array_values() into getNestedExceptions() before returning?

It's probably not a big deal here but yes, we usually avoid changing deprecated code paths.

Another thing, it would be nice if DelayedMessageHandlingException (which implements this new interface) could provide handlers names as array keys as well but.. I don't think this is possible?

I think it would be possible to use exception->getTrace()[0]['class'] as keys in DispatchAfterCurrentBusMiddleware.
I may be missing some edge cases though e.g. if the exception wasn't thrown by the handler or if the handler callable has no class (if that's even possible?)

@kbond kbond mentioned this pull request Oct 23, 2023
Closed
@kbond kbond force-pushed the exception-handler-name branch from c2650c1 to 5e7bb97 Compare October 23, 2023 16:31
@kbond
Copy link
Member Author
kbond commented Oct 23, 2023

It's probably not a big deal here but yes, we usually avoid changing deprecated code paths.

@chalasr, I've made this change.

I think it would be possible to use exception->getTrace()[0]['class'] as keys in DispatchAfterCurrentBusMiddleware.
I may be missing some edge cases though e.g. if the exception wasn't thrown by the handler or if the handler callable has no class (if that's even possible?)

I'm going to leave this off of this PR because of the possible edge cases you describe. This can be revisited later if desired.

@kbond kbond force-pushed the exception-handler-name branch from 5e7bb97 to 06a697c Compare October 23, 2023 16:34
@kbond kbond force-pushed the exception-handler-name branch from 06a697c to dcc788d Compare October 23, 2023 16:57
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 23, 2023
@kbond kbond force-pushed the exception-handler-name branch from dcc788d to 9e67f29 Compare October 24, 2023 14:37
@nicolas-grekas nicolas-grekas changed the title [Messenger] add handler description as array key to HandlerFailedException::getNestedExceptions() [Messenger] add handler description as array key to HandlerFailedException::getWrappedExceptions() Oct 24, 2023
@nicolas-grekas
Copy link
Member

Thank you @kbond.

@nicolas-grekas nicolas-grekas merged commit eb7d88d into symfony:6.4 Oct 25, 2023
This was referenced Oct 29, 2023
Merged
Merged
@kbond kbond deleted the exception-handler-name branch November 13, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@chalasr chalasr 64CB chalasr left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature Messenger ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
6 participants
@kbond @fabpot @chalasr @nicolas-grekas @stof @carsonbot
0