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

[10.x] Fix mapped renderable exception handling #47347

Merged

Conversation

nevadskiy
Copy link
Contributor

In my previous PR #47201, I tries to fix the handling of mapped exceptions with a render hook. The PR was reverted due to a significant mistake I made by moving the mapException method together with prepareException, which I should not have touched. As it was reported by RobertBoes that change breaks exception rendering on the extended internal exceptions. I sincerely apologize for that.

In this PR I have fixed issue from #47201 and also added a missing test for renderable exceptions.


Let me describe the progrem again. When we have a custom exception that defines report and render hooks, only the report hook works while the render hook is never called. For instance:

class Handler extends ExceptionHandler
{
    public function register(): void
    {
        $this->map(RuntimeException::class, AppException::class);
    }
}

class AppException extends Exception
{
    public function report()
    {
        app('sentry')->report($this->getPrevious());
    }

    public function render()
    {
        return new JsonResponse(['message' => 'Something went wrong.']);
    }
}

throw new RuntimeException;

// 👌 The `report` method is called.

// ❌ The `render` method is ignored.

With this change both methods report and render will map the exception at the beginning and handle it correctly:

class Handler
{
    public function report(Throwable $e)
    {
        $e = $this->mapException($e);

        // ...
    }

    public function render($request, Throwable $e)
    {
        $e = $this->mapException($e);

        // ...
    }
}

@taylorotwell
Copy link
Member

Tests fail.

@nevadskiy
Copy link
Contributor Author

@taylorotwell sorry. Now should be fine.

@taylorotwell taylorotwell merged commit 438d02d into laravel:10.x Jun 4, 2023
16 checks passed
@nevadskiy nevadskiy deleted the fix-mapped-renderable-exceptions branch June 4, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants