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

Contents of response appears after exception is thrown #347

Closed
AndrewCarterUK opened this Issue May 21, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@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:

expressive bug

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

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 22, 2016

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

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

@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK May 22, 2016

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().

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

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 22, 2016

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.

shadowhand commented May 22, 2016

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 comment has been minimized.

Show comment
Hide comment
@nesl247

nesl247 May 23, 2016

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.

nesl247 commented May 23, 2016

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.

@xtreamwayz

This comment has been minimized.

Show comment
Hide comment
@xtreamwayz

xtreamwayz May 24, 2016

Member

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');
    }
}
Member

xtreamwayz commented May 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@nesl247

nesl247 May 24, 2016

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,
        ],

nesl247 commented May 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Dec 7, 2016

Member

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.

Member

weierophinney commented Dec 7, 2016

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.

@weierophinney weierophinney closed this in #405 Dec 8, 2016

weierophinney added a commit that referenced this issue Dec 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment