Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Messenger] Add method HandlerFailedException::(has|get)NestedExceptionOfClass #34704

Open
wants to merge 1 commit into
base: master
from

Conversation

@tyx
Copy link
Contributor

tyx commented Nov 29, 2019

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

@tyx tyx requested a review from sroze as a code owner Nov 29, 2019
@tyx tyx force-pushed the tyx:feature/has-nested-exception branch from ef17d8c to cdbe8c8 Nov 29, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Nov 29, 2019
To know if specific exception are the origin of messenger failure
@tyx tyx force-pushed the tyx:feature/has-nested-exception branch from cdbe8c8 to 18981b6 Nov 29, 2019
@tyx tyx changed the title [Messenger] Add method HandlerFailedException::hasNestedExceptionOfClass [Messenger] Add method HandlerFailedException::(has|get)NestedExceptionOfClass Nov 29, 2019

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 15, 2019

Member

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

This comment has been minimized.

Copy link
@tyx

tyx Dec 16, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Nyholm

Nyholm Jan 19, 2020

Member

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 11, 2020

Member

can you please remove this method?

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 15, 2019

Member

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.

This comment has been minimized.

Copy link
@tyx

tyx Dec 16, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Nyholm

Nyholm Jan 19, 2020

Member

Lets use is_a() instead.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 11, 2020

Member

please add some related test cases also.

Copy link
Member

Nyholm left a comment

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;
    }
}

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

This comment has been minimized.

Copy link
@Nyholm

Nyholm Jan 19, 2020

Member

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

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

This comment has been minimized.

Copy link
@Nyholm

Nyholm Jan 19, 2020

Member

Lets use is_a() instead.

@tyx

This comment has been minimized.

Copy link
Contributor Author

tyx commented Jan 19, 2020

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

This comment has been minimized.

Copy link
Contributor

Devristo commented Jan 20, 2020

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?

Copy link
Member

nicolas-grekas left a comment

Friendly ping @tyx


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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 11, 2020

Member

can you please remove this method?

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 11, 2020

Member

please add some related test cases also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.