[PSR-15] Add a new interface for http errors

[Oscar Otero]

Hello.

Although I like the current psr-15 standard, I’ve realized that a third interface for http errors could be useful. I’m not sure whether this has been discussed before, just drop it here just in case you consider it interesting.

Currently is easy to pass values from outer middlewares to inner middlewares, thanks to the server request attributes, but not the opossite, due there’s no response attributes. This is ok because there should be not need to pass values in reverse order, with the exception of errors. When an error occurs, usually is handled throwing a exception to be catched in a outer middleware. This approach has some limitations:

- There’s no (standard) way to decide whether the exception produces a 500 code, 404, 405, etc… This decision is made by the error handler.

- There’s no way to get context data for loggin purposes.

So I’d like to propose a new HttpErrorException to fix these issues. When an error handler catches a HttpErrorException, it knows what type of error is, because there’s the $exception->getCode() and $exception->getMessage() (that should be the status code and reason phrase). If the error is generated because other exception (p.e. PDOException), it can be retrieved using $exception->getPrevious(). And we could add the methods $exception->setContext() and $exception->getContext() to retrieve the context data to use it in a PSR-3 logger. Let’s see an example:

class ErrorHandler implements MiddlewareInterface

{

    public function process(ServerRequestInterface $request, DelegateInterface $delegate)

    {

        try {

            $response = $delegate->process($request);

        } catch (HttpErrorException $exception) {

            //Log

            $this->logger->error('Http Error', [

                'context' => $exception->getContext(),

                'exception' => $exception->getPrevious() ?: $exception

            ]);

            //Create and return the response

            $response = $this->factory->createResponse($exception->getCode());

            $response->getBody()->write($exception->getMessage());

            return $response;

        } catch (Exception $exception) {

            //Log

            $this->logger->error('Exception', ['exception' => $exception]);

            //Generic error 500

            $response = $this->factory->createResponse(500);

            $response->getBody()->write('Server error');

            return $response;

        }

        return $response;

    }

}

Something like this is used in Zend Expresive (https://zendframework.com/blog/2017-03-23-expressive-error-handling.html), https://github.com/middlewares/error-handler, etc. I think it’s a good idea to create an interface that can be used by any middleware instead create vendor-specific exceptions. This allows to have an error handler that can understand any error and handle it correctly.

[Woody Gilk]

I really like this idea. Translating domain errors into HTTP errors has always been a pain point for me and this is an elegant solution.

Anyone else from PSR-15 (proposed) WG have comments?

--
Woody Gilk
http://about.me/shadowhand

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+unsubscribe@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/AD782EAE-00D2-4D01-BFE9-476427080DEB%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Michael Mayer]

Great idea, though support for `HttpErrorException` make sense for every PSR-7 framework, not only PSR-15.

Thus, it has no dependence to middlewares and I see no reason to delay PSR-15 any further – it should be done in a new PSR.

Bests,

Michael Mayer

On Sunday, April 9, 2017 at 4:08:26 PM UTC+2, Woody Gilk wrote:

Anyone else from PSR-15 (proposed) WG have comments?

Huh?

[Stefano Torresi]

I initially liked this idea, but on a second thought I think this could cause some major design issues.

First and foremost, http errors are not exactly exceptional. Every web application produces negative responses, they're part of their normal life cycle, so it could be said that this interface encourages using an exception to control some flow that is actually part of how the application is expected to work.

Secondarily, middlewares don't need another interface to model the errors they produce, they can already use the responses, which have more than enough to provide context to the outer layers: code, reason phrase and body.

Since one of the points of a middleware collection is to decouple the stages of the request processing from each other, I think that offloading this logic to a specific middleware with such a specific mechanic is an implementation detail.
Now, many could have no problem with it per se, but I don't think standardising it would be wise.

For example, the double catch block could completely avoided:

class ErrorHandler implements MiddlewareInterface

{

    public function process(ServerRequestInterface $request, DelegateInterface $delegate)

    {

        try {

            $response = $delegate->process($request);

        } catch (Exception $e) {

            //Generic error 500

            $response = $this->factory->createResponse(500);

            $this->logger->error('Unexpected error', compact($request, $response, $e));

            return $response; 

        }

        if ($this->debug and is_error($response)) {

            $this->logger->debug('Http Error', compact($request, $response));

        }

        return $response;

    }

}

This way the error handler would be catching only truly unexpected errors, the traditional 500s that make us freak out.
You certainly don't throw exceptions for 404s, do you? ;-)

--

You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.

To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.

To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/235bad6d-dca0-46af-b66e-b9f0d5a2f5d2%40googlegroups.com.

[Michael Mayer]

On Sunday, April 9, 2017 at 12:26:41 PM UTC+2, Oscar Otero wrote:

 This approach has some limitations:

- There’s no (standard) way to decide whether the exception produces a 500 code, 404, 405, etc… This decision is made by the error handler.

- There’s no way to get context data for loggin purposes.

I'd love to see both issues solved by some well known methods:

interface XResponseInterface extends ResponseInterface
{
    public function getAttributes();
    public function getAttribute($name, $default = null);
    public function withAttribute($name, $value);
    public function withoutAttribute($name);

}

Those are badly needed anyway.

class ErrorHandler implements MiddlewareInterface
{
    public function process(ServerRequestInterface $request, DelegateInterface $delegate)
    {
        try {
            $response = $delegate->process($request);

        } catch (Exception $exception) {

            // Generic error 500
            $response = $this
                ->factory
                ->createResponse(500)

                ->withAttribute(Exception::class, $exception);

 
           $response->getBody()->write('Server error');
        }

        if ($exception = $response->getAttribute(Exception::class)) {
            // Logs exceptions of inner middlewares too!

            $this->logger->error('Exception', ['exception' => $exception]);
        }

        return $response;
    }
}

Middlewares are already responsible for status codes, e.g.:

class SendFileMiddleware implements MiddlewareInterface
{
    public function process(ServerRequestInterface $request, DelegateInterface $delegate = null)
    {
        // …
        if (!file_exists($path)) {
            return $this
                ->factory
                ->createResponse(404)
                ->withAttribute(Exception::class, new Exception("The file \"$path\" does not exist."))
                ->…;
        }
        // …
    }

}

Nevertheless, a standardized HttpErrorException might be useful, but with slightly different semantics, e.g. on the client side:

try {
    $gget = new GuzzleGet(['base_uri' => 'https://httpbin.org']);
    $response = $gget(new Request('GET', '/'));

    $response->getStatusCode(); // may return 500!

} catch (HttpErrorException $exception) {
    // a network failure or anything else prevented the request from completing
    $exception->getResponse(); // may return null!
}

[Larry Garfield]

On 04/13/2017 02:59 PM, Stefano Torresi wrote:

I initially liked this idea, but on a second thought I think this could cause some major design issues.

First and foremost, http errors are not exactly exceptional. Every web application produces negative responses, they're part of their normal life cycle, so it could be said that this interface encourages using an exception to control some flow that is actually part of how the application is expected to work.

Secondarily, middlewares don't need another interface to model the errors they produce, they can already use the responses, which have more than enough to provide context to the outer layers: code, reason phrase and body.

Since one of the points of a middleware collection is to decouple the stages of the request processing from each other, I think that offloading this logic to a specific middleware with such a specific mechanic is an implementation detail.
Now, many could have no problem with it per se, but I don't think standardising it would be wise.

For example, the double catch block could completely avoided:

class ErrorHandler implements MiddlewareInterface

{

    public function process(ServerRequestInterface $request, DelegateInterface $delegate)

    {

        try {

            $response = $delegate->process($request);

        } catch (Exception $e) {

            //Generic error 500

            $response = $this->factory->createResponse(500);

            $this->logger->error('Unexpected error', compact($request, $response, $e));

            return $response; 

        }

        if ($this->debug and is_error($response)) {

            $this->logger->debug('Http Error', compact($request, $response));

        }

        return $response;

    }

}

This way the error handler would be catching only truly unexpected errors, the traditional 500s that make us freak out.
You certainly don't throw exceptions for 404s, do you? ;-)

Symfony-based projects do, currently.  That's how HttpKernel works.

I am of very mixed mind about that model myself.  It's very easy and effective for a great many use cases and still forces all output through a common pathway (good), but as you point out it is arguably using exceptions for flow of control in non-exceptional situations (bad).

I can see the argument for standardizing HTTP exceptions a la Symfony, but given that it's a controversial approach (this thread is not the first time I've seen it criticized, and as I said I have very mixed emotions about it myself) I am not convinced FIG should come down firmly on endorsing that approach (which is what such a PSR would do).  So I'd lean toward a pass on this proposal at this point, although could be convinced otherwise.

--Larry Garfield