8000 [Messenger] Add WrappedExceptionsInterface for nested exceptions by Jeroeny · Pull Request #51653 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] Add WrappedExceptionsInterface for nested exceptions #51653

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&rd 8000 quo;, 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 1, 2023
Merged

[Messenger] Add WrappedExceptionsInterface for nested exceptions #51653

merged 1 commit into from
Oct 1, 2023

Conversation

Jeroeny
Copy link
Contributor
@Jeroeny Jeroeny commented Sep 14, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? yes
License MIT

Metric & logging tools often want to measure / log individual exceptions from the messenger. There are currently two exception classes that hold a bunch of collected exceptions from different messages or handlers. It would be nice if there was a single interface to check and call upon when extracting these nested exceptions.

Example usecase: https://github.com/getsentry/sentry-symfony/pull/760/files#diff-da0fb4498178e4866e794b813999618022c327dea59f9277b86a7abf784aeafaR98

@carsonbot carsonbot added this to the 6.4 milestone Sep 14, 2023
@Jeroeny Jeroeny force-pushed the msg-nested-xcp branch 2 times, most recently from faf2883 to 4422f6b Compare September 14, 2023 12:25
nicolas-grekas
nicolas-grekas reviewed Sep 19, 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.

In order to reduce the cost of upgrading, I'd prefer not deprecating anything and instead add NestedExceptionsInterface::getExceptions()

This would mean removing the $recursive argument, but I think this would be legit: flattening the exception hierarchy isn't needed for the interface to be sufficient.

WDYT?

@kbond
Copy link
Member
kbond commented Sep 19, 2023 •
edited
Loading

On the topic of nested exceptions. I'm looking for feedback on #51331 - getting the context of these exceptions. In the case of HandlerFailedException: what handler threw the exception. Is there a way currently?

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.

Let's replace the "nested" vocabulary by "wrapped" everywhere.
Can you please also account for #51331 and preserve non-numeric keys (using array_merge(_recursive) should do it).
Last but not least, please also add test covering the $class argument.

@nicolas-grekas nicolas-grekas changed the title [Messenger] Add NestedExceptionsInterface for nested exceptions [Messenger] Add WrappedExceptionsInterface for nested exceptions Sep 29, 2023
nicolas-grekas
nicolas-grekas approved these changes Sep 29, 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 you please rebase? This LGTM after some minor tweaks on my side.

@Jeroeny Jeroeny requested a review from OskarStark as a code owner September 30, 2023 13:18
@Jeroeny
Copy link
Contributor Author
Jeroeny commented Sep 30, 2023

@nicolas-grekas Done. I also changed usages of HandlerFailedException::getNestedExceptions() to getWrappedExceptions().

@fabpot
Copy link
Member
fabpot commented Oct 1, 2023

Thank you @Jeroeny.

@fabpot fabpot merged commit 363e217 into symfony:6.4 Oct 1, 2023
This was referenced Oct 1, 2023
Closed
Merged
@chalasr chalasr mentioned this pull request Oct 2, 2023
Merged
fabpot added a commit that referenced this pull request Oct 2, 2023
@fabpot
minor #51801 [Messenger] Fix WrappedExceptionsTrait (chalasr)
a257327 
This PR was merged into the 6.4 branch.

Discussion
----------

[Messenger] Fix WrappedExceptionsTrait

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT

Fixes some minor flaws introduced in #51653

Commits
-------

f7242ca [Messenger] Fix WrappedExceptionsTrait
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@kbond kbond kbond approved these changes

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

@OskarStark OskarStark Awaiting requested review from OskarStark OskarStark is a code owner

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

Successfully merging this pull request may close these issues.
5 participants
@Jeroeny @kbond @fabpot @nicolas-grekas @carsonbot
0