[4.x] Error handler does not add content-type header if default renderer is used

[adriansuter]

Depending on the error renderer, the error handler should correctly add a content-type header to the response.

HtmlErrorRenderer
text/html

JsonErrorRenderer
application/json

PlainTextErrorRenderer
text/plain

XmlErrorRenderer
text/xml

Proposition A

Add a new method to the renderer interface that would return the content-type.

Proposition B

Add a method to the renderer interface that receives the response and returns a possibly altered response (for example headers added). Method name to be discussed 😃.

Caution

Both propositions would introduce a breaking change.

Other ideas or inputs? Could we just use the $contentType property from the error handler itself? Or could that be dangerous as this is actually user input? What to do in case of the default error renderer?

[l0gicgate]

So I don’t think it should be the renderer’s responsibility to set the Content-Type. Since the error handler generates the response and determines the Content-Type I would say let’s have a setter to force a certain content type, add a parameter to the renderer registering function so you can also set the content type associated with it, and then if the default content type isn’t set we fallback on the detected content type parsed during renderer determination logic.

What do you think? This doesn’t introduce any breaking changes.

[adriansuter]

You are right, the renderer is just responsible for rendering the error.

By the way, I just saw, that actually the error handler is already adding the content-type header.

Slim/Slim/Handlers/ErrorHandler.php

Line 273 in 01f6923

$response = $response->withHeader('Content-type', $this->contentType);

Hence the only renderer that the handler does not know the content type of, is the default renderer. Well, currently it appears that the handler assumes the default renderer to be text/html. The method \Slim\Handlers\ErrorHandler::determineContentType() would return text/html if no match could be found between Accept header of the request and registered error renderers.

Probably we should return null in those cases, i.e. the content type could not be determined (or could not be matched with registered error renderers) successfully. Then we should bring the content type of the default error renderer into the error handler. Probably as new argument to setDefaultErrorRenderer(). To make it consistent (with registerErrorRenderer) I would suggest to add the content type argument as first argument.

    public function setDefaultErrorRenderer(string $contentType, $errorRenderer): void
    {
        $this->defaultErrorRendererContentType = $contentType;
        $this->defaultErrorRenderer = $errorRenderer;
    }

So the respond() method would look like

    protected function respond(): ResponseInterface
    {
        $response = $this->responseFactory->createResponse($this->statusCode);
        if (array_key_exists($this->contentType, $this->errorRenderers)) {
            $response = $response->withHeader('Content-type', $this->contentType);
        } else {
            $response = $response->withHeader('Content-type', $this->defaultErrorRendererContentType);
        }
        // ....

What do you think?

[l0gicgate]

Looks good to me. Let’s go ahead and do that!