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

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.

@coveralls
Copy link

coveralls 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
Copy link
Member

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
Copy link
Contributor Author

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

Thanks for your kind words.

@adriansuter
Copy link
Contributor Author

@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
Copy link
Member

l0gicgate 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
Copy link
Member

@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
Copy link
Contributor Author

What about #2737?

@l0gicgate l0gicgate merged commit 6d6d34e into slimphp:4.x Jun 25, 2019
@adriansuter adriansuter deleted the path-customErrorRenderer branch June 26, 2019 16:41
@l0gicgate l0gicgate mentioned this pull request Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants