Contents of response appears after exception is thrown

[AndrewCarterUK]

Using the skeleton project I changed PingAction to this:

    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next = null)
    {
        $response->getBody()->write(json_encode(['ack' => time()]));
        $response = $response->withHeader('Content-Type', 'application/json');

        throw new \Exception('Access denied');

        return $response;
    }

However, it appears that a new response isn't used to render the error page. The JSON that was written to the response before the exception was thrown still appears in the error page:

I'm happy to submit a PR, but I checked the TemplatedErrorHandler and it seemed like some of the behaviour using the original response was intentional?

[shadowhand]

rewind should be called here before writing to the body to ensure that the stream only contains the exception output.

[AndrewCarterUK]

What about if the stream isn't seekable or if the output written is longer than the error page?

I think maybe a better solution would be to create a new StreamInterface object and do withBody().

[shadowhand]

You're totally right. Unfortunately there's no easy way to create a new StreamInterface without depending on a concrete implementation. This won't be a problem for Expressive, since it depends on Diactoros, but it will be a problem for middleware attempting to depend only on psr/http-message.

[nesl247]

This would probably also be an issue with the FinalHandler. In my opinion, the handleError and handleException methods should be creating new Responses not using the existing response. What happens if some headers and what not were added that shouldn't be during an error?

The downside to this is if you need to be able to add custom headers, you cannot do so.

One of my biggest gripes with expressive is actually the error handling. It's probably the least straight-forward system. Making more use of the middleware-pipeline for the error handling would be really great. It would also be a good way to solve this, as well as still allow custom headers.

[geerteltink]

In my opinion, if an exception is thrown, it shouldn't re-use any existing Response:

class UseEmptyDefaultResponseMiddleware
{
    public function __invoke($req, $res, $next)
    {
        // Trigger Exception and use a new empty response
        throw new \Exception('Page Not Found', 404);
    }
}

If you want to reuse the Response object and add headers, that is already covered:

class ReuseCurrentResponseMiddleware
{
    public function __invoke($req, $res, $next)
    {
        // Trigger error with a specific response
        $res = $res->withHeader('X-Powered-By', 'Zend Expressive');
        return $next($req, $res->withStatus(404), 'Page Not Found');
    }
}

[nesl247]

I agree that it shouldn't reuse an existing response. However my question remains that shouldn't error handling be done via the error middleware? Seems weird to me to have two ways to do error handling. I can understand needing a FinalHandler, but in my opinion, that should really be a worst case scenario. Nothing should ever get to it unless even the error middleware fail to return a response (aka they are throwing exceptions for some reason).

This is kind of how I would expect to see a template error handler implemented. This would allow modifying the response from that error handler if you put another middleware before it and did $response = $next($request, $response, $error);. Of course in this scenario TemplateErrorHandler would be a middleware that does a return new Response(...);.

        'error' => [
            'middleware' => [
                \TemplateErrorHandler::class
            ],
            'error' => true,
            'priority' => -10000,
        ],

[weierophinney]

There are two issues being discussed here:

• The fact that the FinalHandler writes to the same response body, appending its contents, which can lead to unexpected results.
• Error handling is split between Stratigility error middleware and the final handler, causing confusion.

The second is not what was originally reported, but a topic that came up during discussion of the issue; it is being resolved for version 1.1.0 via #396.

The first, however, is not resolved, and should be. I'll work on a PR for this shortly.