Deprecate FinalHandler and ErrorMiddlewareInterface

[weierophinney]

This patch will provide the forwards compatibility features for the next minor version of Stratigility that implement the RFC on FinalHandler and ErrorMiddlewareInterface removal.

• Create NotFoundHandler and ErrorHandler middleware
  • NotFoundHandler returns a basic 404 response, always.
  • ErrorHandler:
    • creates a PHP error handler that will throw any errors not in the current error_handler mask as ErrorExceptions.
    • Does a try/catch around $next()
    • Raises a new exception, MissingResponseException, if $next does not return a response.
    • Casts all Throwable/Exception items to error responses:
      • If a boolean true flag is passed to the constructor, it will use the stack trace in the response.
      • Triggers listeners on the instance with the exception/throwable, request, and response, for reporting purposes.
• Create a no-op final handler that returns the provided response on invocation, ignoring all other arguments.
• Update MiddlewarePipe to issue a deprecation notice if $out is omitted.
• Mark Dispatch as deprecated.
• Update Next to raise deprecation notice when receiving a non-null $err argument.
• Mark FinalHandler as deprecated.
• Mark ErrorMiddlewareInterface as deprecated.
• Write migration guide.

Notes

• In order to return a response, each new middleware type (NotFoundHandler, ErrorHandler) accepts a prototype ResponseInterface instance to the constructor; this is then used to generate the returned response in 404/error conditions. This is done to allow re-use with any PSR-7 implementation.

[weierophinney]

Marked as a BC break, as it introduces deprecation notices. These are, however, documented, and the messages themselves point to online (not-yet-published, but included in this PR) documentation to assist users in making their code forwards-compatible.

[weierophinney]

Pinging @zendframework/community-review-team / @ezimuel / @michaelmoussa for review!

[vaclavvanik]

I think new ErrorHandler should have possibility to create response depending on content-type.

It may be optional, eg. any callable param in constructor. Thoughts?

[weierophinney]

@vaclavvanik I like the idea, but I think that belongs more properly in zend-expressive or its skeleton, and not in Stratigility. Stratigility is a foundation, and the expectation is that it provides only the minimum functionality; in fact, the docs I wrote even recommend replacing the ErrorHandler and NotFoundHandler in your own applications in order to provide your own features.

[weierophinney]

@vaclavvanik I've updated the implementation to provide an extension point in ErrorHandler, allowing developers to extend the class and provide a protected createErrorResponse() method. This allows a common workflow of:

• creating a PHP error handler that casts to ErrorException
• using a try/catch block to handle all exceptions/throwables and respond to them
• generating an error response
• providing listeners that can act on the final error, request, and response

This will provide a better starting point with flexibility for end-users, while still being minimalist.

[vaclavvanik]

@weierophinney Imo custom error response factory could be injected as optional callable into ErrorHandler constructor. This allow great flexibility when pulling ErrorHandler from container and createErrorResponse method can be still private.

class ErrorHandler
{
    private function createErrorResponse($e, ServerRequestInterface $request, ResponseInterface $response)
    {
        if ($this->errorResponseCallback) {
            return call_user_func($this->errorResponseCallback, $e, $request, $response);
        }

        $response = $response->withStatus(Utils::getStatusCode($e, $response));
        $body = $response->getBody();

        if ($this->isDevelopmentMode) {
             $body->write($this->createDevelopmentErrorMessage($e));
             return $response;
        }

        $body->write($response->getReasonPhrase() ?: 'Unknown Error');
        return $response;
    }
}

$errorResponse = function ($e, ServerRequestInterface $request, ResponseInterface $response) {
  // do content negotiation and other things
  return $response;
};

$errorHandler = new ErrorHandler(new Response(), $isDevelopmentMode,  $errorResponse);

Ummm... thinking maybe whole error response factory could be moved to another class. So ErrorHandler constructor could look:

class ErrorHandlerResponseFactory
{
    public function __construct(ResponseInterface $responsePrototype, $isDevelopmentMode = false);
    public function __invoke($e, ServerRequestInterface $request, ResponseInterface $response);
}

class ErrorHandler
{
    public function __construct(callable $errorResponseFactory)
    {
        $this->errorResponseFactory = $errorResponseFactory;
    } 
}

Thanks for your opinions.

[michaelmoussa]

@weierophinney what are your thoughts on a more granular configuration $options parameter with an is_development_mode setting rather than a simple flag? Or would further configurability be something that should be accomplished by extending the handler?

[weierophinney]

I've actually just pushed changes based on the feedback from @vaclavvanik that shoves that stuff into the response generator. As such, the ErrorHandler now has only the following responsibilities:

• compose a response prototype, a response generator, and listeners
• create and enable a PHP error handler that casts errors to exceptions
• detect when no inner middleware returns a response, and raise an exception
• catch all exceptions, and pass them, the request, and the prototype response to the response generator
• notifiy all listeners of the error, the request, and the final response

As such, no "optional" types of behavior are present, and those would be handled by the response generator and/or listeners, which fall out of scope of the class itself.

[michaelmoussa]

@weierophinney I'm not clear on why the responsePrototype is needed in the ErrorHandler. Could we not use the $response that's passed to __invoke and treat that as the prototype?

[weierophinney]

I'm using the response prototype in part to prepare for PSR-15, where the response is not passed to middleware.

[michaelmoussa]

@weierophinney shouldn't this implement MiddlewareInterface?

[weierophinney]

Possibly, but if MiddlewareInterface is replaced in v2 with the PSR-15 versions, it'd be an extra change.

[weierophinney]

I've updated to make this implement MiddlewareInterface now. We can change if/when PSR-15 is ratified.

[michaelmoussa]

Thanks. I'll look at the remainder of modifications later this afternoon.

In general, should we tend to approach solutions based on what works best for Expressive in the present? At least until PSR-15 is close to ratification or the decision to not include the Response in the signature is as close to a sure thing as possible?

I figure that no matter what happens, there will need to be changes, so perhaps sticking to the conventions we have now will make it easier for people to continue upgrading until it's time for a major migration.

[weierophinney]

the decision to not include the Response in the signature is as close to a sure thing as possible

This is a sure thing at this point; all those working on http-interop have agreed it should not be present.

In terms of upgrading, in the case of these two middleware, it's an implementation detail unless they choose to write their own implementations. If they do, they can do so using whatever conventions make sense to them. We'd just be providing minimalist barebones implementations (though the ErrorHandler can likely be used verbatim everywhere, due to the ability to inject a response generator).

[michaelmoussa]

@weierophinney I'm a little unclear on $responsePrototype here as well, in addition to the different __invoke signature. If we add the response param to __invoke, then that could be used as the prototype, and if we add $next, then the NoopFinalHandler will complete the pipe and return the response back, while still giving the user the option to execute some additional middleware after the NotFoundHandler without having to extend it (perhaps logging or metrics gathering of some sort following a 404). I think in that case the NotFoundHandler would also be able to implement MiddlewareInterface.

[weierophinney]

Please see my response above regarding the ErrorHandler.

Essentially, you should only manipulate the response when traversing outwards; i.e., the response returned by $next().

Since this particular middleware does not call on $next(), I omitted that argument entirely. However, to be useful in other stacks, it would need to implement the full interface. I'll update both of these middleware to do so shortly.

[weierophinney]

Incorporated feedback from @vaclavvanik and @michaelmoussa .

[weierophinney]

@vaclavvanik I've adopted the idea of injecting a response generator via the ErrorHandler constructor; if none is provided, the default ErrorResponseGenerator is used, in production mode. Thanks for the suggestion!

[vaclavvanik]

Should'nt be $app->pipe(new ErrorHandler(new Response(), new ErrorResponseGenerator($isDevelopmentMode));?

[weierophinney]

Good catch! fixed!

[michaelmoussa]

@weierophinney should this be final if we have the extra attachListener public method w/no interface to cover it? (i'm sure you've seen Marco's blog post on the subject). Perhaps final class ErrorHandler implements ErrorHandlingMiddleware instead, with this interface?

interface ErrorHandlerMiddleware extends MiddlewareInterface
{
    public function attachListener(callable $listener);
}

[michaelmoussa]

OTOH it actually might not matter in this case since nothing typehints specifically on ErrorHandler, so it shouldn't affect anyone's ability to test or reuse this component.

[weierophinney]

@michaelmoussa Exactly. 😄

Essentially, if they need to extend it, it's small enough they can copy/paste. In terms of testing a system, they can provide a stub implementing MiddlewareInterface against which to perform assertions.