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 inconsistentcy between report and render methods #47201

Merged
merged 2 commits into from
May 24, 2023
Merged

[10.x] Fix inconsistentcy between report and render methods #47201

merged 2 commits into from
May 24, 2023

Conversation

xalaida
Copy link
Contributor

@xalaida xalaida commented May 24, 2023

What is the problem?

If ExceptionA implements a render method and ExceptionB is mapped to ExceptionA in ExceptionHandler, the render hook will not be called. This is because ExceptionHandler calls the render hook after the exception mapping. However, with the report method, it works fine because it maps the exception before calling the report hook.

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('Test exception');

// 👌 The `report` method is called on the mapped `AppException` instance.

// ❌ The `render` method is ignored and the default error response is returned.

Solution

Move mapException to the top of the render method.

@taylorotwell taylorotwell merged commit c95df92 into laravel:10.x May 24, 2023
@RobertBoes
Copy link
Contributor

RobertBoes commented Jun 2, 2023

It seems like this PR introduced a breaking change

We have an exception that extends the Illuminate\Auth\Access\AuthorizationException as following:

<?php

namespace App\Exceptions;

use Illuminate\Auth\Access\AuthorizationException as BaseAuthorizationException;

class AuthorizationException extends BaseAuthorizationException
{
    public function render($request)
    {
        // render our exception
    }
}

Before this change it would call the render method on this exception, but with this change the exception is now wrapped in a Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException and it would never call the render method

@driesvints
Copy link
Member

We reverted this PR. Thanks @RobertBoes

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.

4 participants