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

[4.x] Make error renderers in the error handler customizable #2734

Merged
merged 2 commits into from Jun 25, 2019

Conversation

@adriansuter
Copy link
Contributor

commented Jun 20, 2019

This PR addresses #2731.

The default error handler has now a registry of known content types and their corresponding error renderers. It is possible to register new content types (or update existing content types) using \Slim\Handlers\ErrorHandler::registerErrorRenderer().

The default error renderer can also be defined using \Slim\Handlers\ErrorHandler::setDefaultErrorRenderer().

I removed the property $knownContentTypes as this information could now be read directly from the array keys of the new property $errorRenderers. Not sure if that would break things for users.

Usage

A user could define a new renderer

class MyErrorRenderer implements \Slim\Interfaces\ErrorRendererInterface
{
    /**
     * {@inheritdoc}
     */
    public function render(Throwable $exception, bool $displayErrorDetails): string
    {
        // TODO Build the custom error string that should be sent as response to the client
        // For example:
        return 'Custom Translated Error';
    }
}

and then after instantiating the app, the user could do the following

// Add error handling middleware
$callableResolver = $app->getCallableResolver();
$responseFactory  = $app->getResponseFactory();
$errorMiddleware  = new ErrorMiddleware($callableResolver, $responseFactory, $displayErrorDetails, false, false);

$eh = $errorMiddleware->getDefaultErrorHandler();
$eh->registerErrorRenderer('text/html', MyErrorRenderer::class);
$errorMiddleware->setDefaultErrorHandler($eh);

$app->add($errorMiddleware);

Further work to do

I think it would be nice, if we could get the Container into the renderer (if defined). So that the renderer can get the template engine for example.

Idea: A new interface called ContainerAwareInterface that has exactly one method called setContainer(). So when the renderer gets instantiated, we could check if the renderer implements this interface and if so, we could inject the container into the renderer.

@adriansuter adriansuter added the Slim 4 label Jun 20, 2019

@coveralls

This comment has been minimized.

Copy link

commented Jun 20, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling c821bba on adriansuter:path-customErrorRenderer into c120b49 on slimphp:4.x.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

I wonder if we should just pass CallableResolver into ErrorHandler and resolve the renderers. Then it gives the ability to someone to inject dependencies via the renderer's constructor.

Everything looks good though so far for this PR, great work!

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

I am currently away. Will have to check that next week.

Thanks for your kind words.

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

@l0gicgate

I wonder if we should just pass CallableResolver into ErrorHandler and resolve the renderers. Then it gives the ability to someone to inject dependencies via the renderer's constructor.

I think, that could work. In this case though, the renderer(s) would be instantiated in every request, even if no error gets triggered. Could that lead to a performance issue?

To achieve that, we would have to change the signature of the constructor \Slim\Handlers\ErrorHandler::__construct() as we have to be sure that the resolver exists. Could that be a potential breaking change?

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

To achieve that, we would have to change the signature of the constructor \Slim\Handlers\ErrorHandler::__construct() as we have to be sure that the resolver exists. Could that be a potential breaking change?

If someone extended the base handler and overrode the existing constructor then they'd have to change the constructor signature then I guess we'd break their code but that's a pretty minor fix. We're still in beta so I'd rather break things now if anything.

It's a much cleaner way to instantiate things to just use CallableResolver. The logic is all there so.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@adriansuter this PR looks good to go, when you remove the WIP from the title I'll merge. Thanks

@adriansuter adriansuter changed the title [WIP] [4.x] Make error renderers in the error handler customizable [4.x] Make error renderers in the error handler customizable Jun 25, 2019

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

What about #2737?

@l0gicgate l0gicgate merged commit 6d6d34e into slimphp:4.x Jun 25, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@adriansuter adriansuter deleted the adriansuter:path-customErrorRenderer branch Jun 26, 2019

@l0gicgate l0gicgate referenced this pull request Aug 1, 2019

Merged

Slim 4 Release #2769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.