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] Use callable resolver in the error handler to resolve error renderers #2737

Merged

Conversation

@adriansuter
Copy link
Contributor

commented Jun 25, 2019

This PR is a follow-up of #2734 and addresses #2731.

CAUTION

There are some breaking changes in this PR.

  • The property knownContentTypes of \Slim\Handlers\ErrorHandler had been removed.
  • The constructor of \Slim\Handlers\ErrorHandler changed its signature.

Notes

The \Slim\CallableResolver::resolve() method had to be changed as well, as the "callable method" of the error renderer interface is called render.

Description

A user can register custom error renderers in the following ways:

1. Register a class name

Create a new class implementing the \Slim\Interfaces\ErrorRendererInterface, and then simply register the fully qualified class name.

class MyCustomHtmlErrorRenderer implements ErrorRendererInterface
{
    /**
     * {@inheritdoc}
     */
    public function render(Throwable $exception, bool $displayErrorDetails): string
    {
        return '<h1>Custom Error Renderer</h1><pre>'.$exception->getTraceAsString().'</pre>';
    }
}

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

2. Register an instance of an error renderer interface

Create a new class like in the example above. Instantiate the class and register the instance.

$errorHandler = $errorMiddleware->getDefaultErrorHandler();
$errorHandler->registerErrorRenderer('text/html', new MyCustomHtmlErrorRenderer());
$errorMiddleware->setDefaultErrorHandler($errorHandler);

This is the way to go, if you need to inject objects into your custom error renderer.

3. Closure

Register a closure.

$errorHandler = $errorMiddleware->getDefaultErrorHandler();
$errorHandler->registerErrorRenderer('text/html', function(Throwable $exception, bool $displayErrorDetails): string {
    return 'Error: '.$exception->getMessage();
});
$errorMiddleware->setDefaultErrorHandler($errorHandler);

Open questions

I think we could remove (make private) the property \Slim\Handlers\ErrorHandler::$renderer. This was formerly used to override the renderer. As that can be achieved by the technique described above, the property should be removed. Maybe that would be another breaking change?

Is there a reason for determining the renderer in the __invoke() method of the error handler? If not, I suggest to determine the renderer directly in the \Slim\Handlers\ErrorHandler::respond() method. Then we can actually get rid of the $renderer property.

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

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Travis failed because of the property $renderer. As soon as I can remove that, Travis should succeed. What do you think, can that property be removed?

@l0gicgate l0gicgate added this to the 4.0.0 milestone Jun 25, 2019

@l0gicgate
Copy link
Contributor

left a comment

if ($renderer !== null
&& (
(is_string($renderer) && !class_exists($renderer))
|| !in_array(ErrorRendererInterface::class, class_implements($renderer), true)
)
) {
throw new RuntimeException(
'Non compliant error renderer provided (%s). ' .
'Renderer must implement the ErrorRendererInterface'
);
}

We can remove that entire logic block since CallableResolver does all the logic here. All we care about is that a callable is being returned. It will throw an exception if a callable can't be resolved.

I also wonder if we should change the ErrorRendererInterface to use __invoke() instead of render() which would enable us to remove the extra logic added to CallableResolver. What do you think?

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Yes, let's rename render to __invoke and therefore make the renderers invokable. I will update the PR.

@coveralls

This comment has been minimized.

Copy link

commented Jun 26, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 37b099a on adriansuter:feature-customErrorRenderer-callableResolver into 6d6d34e on slimphp:4.x.

adriansuter added some commits Jun 26, 2019

Remove test case for error renderers
The test case is no longer needed, as the error renderers are now directly  invokable.
Delete MockErrorRenderer.php
This mock is no longer needed
@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

CAUTION

As noted in the description of this PR, #2737 (comment), there are some breaking changes:

  • The property knownContentTypes of \Slim\Handlers\ErrorHandler had been removed.
  • The property renderer of \Slim\Handlers\ErrorHandler had been removed.
  • The constructor of \Slim\Handlers\ErrorHandler changed its signature.
  • The error renderers had been made invokable. Therefore the method render is now called __invoke.

@adriansuter adriansuter requested a review from l0gicgate Jun 26, 2019

@adriansuter adriansuter changed the title [WIP] [4.x] Use callable resolver in the error handler to resolve error renderers [4.x] Use callable resolver in the error handler to resolve error renderers Jun 26, 2019

Removve if condition
`$renderer` is always caallable.

@adriansuter adriansuter requested a review from l0gicgate Jun 26, 2019

adriansuter added some commits Jun 26, 2019

Remove if condition
`$renderer` is always callable.

@l0gicgate l0gicgate merged commit 854b3ce into slimphp:4.x Jun 26, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adriansuter adriansuter deleted the adriansuter:feature-customErrorRenderer-callableResolver 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.