[Messenger] Add method HandlerFailedException::getNestedExceptionOfClass #34704

tyx · 2019-11-29T09:41:24Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT
Doc PR	in progress

I have many times use this kind of code on my own development. It helps when dealing with specific exception through a Messenger usage.

Example in an ExceptionListener, the exception you get could be a HandlerFailedException but you want a specific treatment when the original exception is different (like a RedirectResponse, or Http error code different).

edit: I finally also added a getNestedExceptionOfClass that could be useful too

src/Symfony/Component/Messenger/Tests/Fixtures/MyOwnException.php

nicolas-grekas · 2019-12-15T11:23:16Z

src/Symfony/Component/Messenger/Exception/HandlerFailedException.php

+
+    public function hasNestedExceptionOfClass(string $exceptionClassName): bool
+    {
+        return 0 < \count($this->getNestedExceptionOfClass($exceptionClassName));


I'm not sure about this method: it looks like calling for double computation of the logic.

Hi ! You mean, I should remove it or you have an idea to improve it ?

If we have 20 exceptions, and the first one is $exceptionClassName, we will still count the other 19. Right?

can you please remove this method?

nicolas-grekas · 2019-12-15T11:24:02Z

src/Symfony/Component/Messenger/Exception/HandlerFailedException.php

+        return array_filter(
+            $this->exceptions,
+            function ($exception) use ($exceptionClassName) {
+                return \get_class($exception) === $exceptionClassName;


should this be an exact match, or an is_a() check?
you feature request looks like an instanceof check. This would mean is_a() here.

Ok good catch. In my mind it was a is_a check but I should admin an instanceof looks better indeed.

Lets use is_a() instead.

please add some related test cases also.

Nyholm

Yes please! I can see this is somewhat useful.

I often write code like:

try {
    $this->bus->dispatch($command);
} catch (HandlerFailedException $e) {
    $exception = $e->getPrevious();
    if ($exception instanceof FooException) {
        // Handle it one way
    } else {
        // I dont know what to do... 
        throw $e;
    }
}

Now I can write:

try {
    $this->bus->dispatch($command);
} catch (HandlerFailedException $e) {
    if ($e->hasNestedExceptionOfClass(FooException::class)) {
        // Handle it one way
    } else {
        // I dont know what to do...
        throw $e;
    }
}

Nyholm · 2020-01-19T20:15:50Z

src/Symfony/Component/Messenger/Exception/HandlerFailedException.php

+
+    public function hasNestedExceptionOfClass(string $exceptionClassName): bool
+    {
+        return 0 < \count($this->getNestedExceptionOfClass($exceptionClassName));


If we have 20 exceptions, and the first one is $exceptionClassName, we will still count the other 19. Right?

Nyholm · 2020-01-19T20:16:41Z

src/Symfony/Component/Messenger/Exception/HandlerFailedException.php

+        return array_filter(
+            $this->exceptions,
+            function ($exception) use ($exceptionClassName) {
+                return \get_class($exception) === $exceptionClassName;


Lets use is_a() instead.

tyx · 2020-01-19T21:04:54Z

Hello @Nyholm !

Thanks for the reminder.

Glad to see I'm not alone writing this kind of code ;)

I postponed updating this PR for a long time. I will fix that asap.

Devristo · 2020-01-20T11:32:25Z

When dispatching a new command from a command handler it could cause additional nesting of HandlerFailedException. For example:

HandlerFailedException
- HandlerFailedException
  - MyCustomException

Is this something we would like to consider?

nicolas-grekas

Friendly ping @tyx

nicolas-grekas · 2020-02-11T14:37:18Z

src/Symfony/Component/Messenger/Exception/HandlerFailedException.php

+
+    public function hasNestedExceptionOfClass(string $exceptionClassName): bool
+    {
+        return 0 < \count($this->getNestedExceptionOfClass($exceptionClassName));


can you please remove this method?

nicolas-grekas · 2020-02-11T14:37:52Z

src/Symfony/Component/Messenger/Exception/HandlerFailedException.php

+        return array_filter(
+            $this->exceptions,
+            function ($exception) use ($exceptionClassName) {
+                return \get_class($exception) === $exceptionClassName;


please add some related test cases also.

Nyholm · 2020-05-12T09:00:06Z

A nice little ping again =)

tyx · 2020-05-12T09:55:33Z

Nice timing @Nyholm ;)

I just updated and rebased the PR

Nyholm · 2020-05-12T11:23:11Z

Thank you for the update.

I think the current code looks good. Im just curious about something. Im not sure we should change anything, but I just want to ask. Is there someway we can stop dealing with arrays?

Maybe Im just interested in the first error. Ie:

try {
    $this->bus->dispatch($command);
} catch (HandlerFailedException $e) {
    if (null !== $exception = $e->getFirstNestedExceptionOfClass(FooException::class)) {
        echo $exception->getMessage()
    } else {
        throw $e;
    }
}

The vast majority of the times, I get an HandlerFailedExcpetion with just a single exception in it. Do we care?

To know if specific exception are the origin of messenger failure

tyx · 2020-05-13T09:20:17Z

Regarding the PR I made a small tweak to ensure returning an array with correct indexes (and update test).

For your point @Nyholm you're totally right. I do the same !

May be we could also add another method to get only the first occurence as sugar ?

fabpot · 2020-08-11T07:52:37Z

Thank you @tyx.

tyx requested a review from sroze as a code owner November 29, 2019 09:41

carsonbot added Status: Needs Review Messenger Feature labels Nov 29, 2019

tyx force-pushed the feature/has-nested-exception branch from ef17d8c to cdbe8c8 Compare November 29, 2019 09:57

nicolas-grekas added this to the next milestone Nov 29, 2019

nicolas-grekas reviewed Nov 29, 2019

View reviewed changes

src/Symfony/Component/Messenger/Tests/Fixtures/MyOwnException.php Outdated Show resolved Hide resolved

tyx force-pushed the feature/has-nested-exception branch from cdbe8c8 to 18981b6 Compare November 29, 2019 16:45

tyx changed the title ~~[Messenger] Add method HandlerFailedException::hasNestedExceptionOfClass~~ [Messenger] Add method HandlerFailedException::(has|get)NestedExceptionOfClass Nov 29, 2019

nicolas-grekas reviewed Dec 15, 2019

View reviewed changes

Nyholm requested changes Jan 19, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 19, 2020

Nyholm mentioned this pull request Jan 26, 2020

[Messenger] Add possibility to disable HandleFailedException for specific bus #35427

Closed

nicolas-grekas requested changes Feb 11, 2020

View reviewed changes

tyx changed the title ~~[Messenger] Add method HandlerFailedException::(has|get)NestedExceptionOfClass~~ [Messenger] Add method HandlerFailedException::getNestedExceptionOfClass May 12, 2020

tyx force-pushed the feature/has-nested-exception branch from 18981b6 to 89083e3 Compare May 12, 2020 09:54

tyx force-pushed the feature/has-nested-exception branch from 89083e3 to 99d8371 Compare May 13, 2020 08:59

[Messenger] Add method HandlerFailedException::getNestedExceptionOfClass

e0dd84b

To know if specific exception are the origin of messenger failure

tyx force-pushed the feature/has-nested-exception branch from 99d8371 to e0dd84b Compare May 13, 2020 08:59

tyx requested a review from nicolas-grekas May 26, 2020 10:26

carsonbot added Status: Needs Review and removed Status: Needs Work labels May 26, 2020

fabpot approved these changes Aug 11, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 11, 2020

fabpot mentioned this pull request Aug 11, 2020

[Messenger] Add method HandlerFailedException::getNestedExceptionOfClass symfony/symfony-docs#14050

Closed

fabpot merged commit e7617da into symfony:master Aug 11, 2020

tyx deleted the feature/has-nested-exception branch August 11, 2020 18:56

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

Uh oh!

[Messenger] Add method HandlerFailedException::getNestedExceptionOfClass #34704

[Messenger] Add method HandlerFailedException::getNestedExceptionOfClass #34704

Uh oh!

Conversation

tyx commented Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm Jan 19, 2020 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Nyholm Jan 19, 2020 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyx commented Jan 19, 2020

Uh oh!

Devristo commented Jan 20, 2020

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented May 12, 2020

Uh oh!

tyx commented May 12, 2020

Uh oh!

Nyholm commented May 12, 2020

Uh oh!

tyx commented May 13, 2020

Uh oh!

fabpot commented Aug 11, 2020

Uh oh!

Uh oh!

tyx commented Nov 29, 2019 •

edited

Loading

Nyholm Jan 19, 2020 •

edited by nicolas-grekas

Loading

Nyholm Jan 19, 2020 •

edited by nicolas-grekas

Loading