Skip to content

[9.x] Refactor ExceptionHandler render method #39930

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

Merged
merged 5 commits into from
Dec 8, 2021

Conversation

seriquynh
Copy link
Contributor

What is the problem?

For years, a bunch of features such as render check and Responsable check have been added to the ExcepionHandler render method. The latest thing is render callbacks registration. Because of them, the ExcepionHandler render method becomes giant and messy. I think it's time to refactor this method, so it will be easier to read and maintain.

What do I do?

In this MR, I've done three things:

  1. Remove a few useless else statements.
  2. Replace a few checks by match expressions.
  3. Separate logic into smaller methods.

How is it improved?

The ExcepionHandler render method now has a clear logic in this order:

  1. Return if the given exception defines render method that returns a not null result.
  2. Return if the given exception implements Responsable interface.
  3. Convert the given exception into our expected exception before continue the rest logic.
  4. Return if developers handle the given exception inside their render callbacks.
  5. Handle other cases such as authentication and validation or return the default response if any.

This is a quick overview of the final method:

class Handler
{
    public function render($request, Throwable $e)
    {
        if (method_exists($e, 'render') && $response = $e->render($request)) {
            return Router::toResponse($request, $response);
        }

        if ($e instanceof Responsable) {
            return $e->toResponse($request);
        }

        $e = $this->prepareException($this->mapException($e));

        if ($response = $this->renderViaCallbacks($request, $e)) {
            return $response;
        }

        return match(true) {
            $e instanceof HttpResponseException => $e->getResponse(),
            $e instanceof AuthenticationException => $this->unauthenticated($request, $e),
            $e instanceof ValidationException => $this->convertValidationExceptionToResponse($e, $request),
            default => $this->renderExceptionResponse($request, $e),
        };
    }
}

@seriquynh seriquynh changed the title Refactor handler render [9.x] Refactor ExceptionHandler render method Dec 8, 2021
@seriquynh
Copy link
Contributor Author

1) Illuminate\Tests\Support\SupportHelpersTest::testRetryWithPassingSleepCallback
Failed asserting that 0.3255300521850586 matches expected 0.3.

D:\a\framework\framework\tests\Support\SupportHelpersTest.php:586

This failure isn't related to this MR. There is a problem with the retry function and windows platform. I test it on my windows laptop. It often passes but seldom fails. For example, run this test will result 9 passes and 1 failure.

restry_test

@taylorotwell taylorotwell merged commit 1873d2a into laravel:master Dec 8, 2021
@taylorotwell
Copy link
Member

Thanks for contributing to Laravel! ❤️

@seriquynh seriquynh deleted the refactor-handler-render branch December 8, 2021 13:13
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.

2 participants