From c2352450ead5368681b17dfd34b157bf34b2a16b Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 23 Jun 2015 12:47:24 -0500 Subject: [PATCH 1/8] Extract `FinalHandler::getStatusCode` to Utils - Extracted `FinalHandler::getStatusCode` to `Utils::getStatusCode`, making it a public static method (per @SpiffyJr). --- src/FinalHandler.php | 30 +----------------------------- src/Utils.php | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/FinalHandler.php b/src/FinalHandler.php index 37d77ee..870e6d8 100644 --- a/src/FinalHandler.php +++ b/src/FinalHandler.php @@ -66,7 +66,7 @@ public function __invoke(Http\Request $request, Http\Response $response, $err = private function handleError($error, Http\Request $request, Http\Response $response) { $response = $response->withStatus( - $this->getStatusCode($error, $response) + Utils::getStatusCode($error, $response) ); $message = $response->getReasonPhrase() ?: 'Unknown Error'; @@ -104,34 +104,6 @@ private function create404(Http\Request $request, Http\Response $response) return $response->end($message); } - /** - * Determine status code - * - * If the error is an exception with a code between 400 and 599, returns - * the exception code. - * - * Otherwise, retrieves the code from the response; if not present, or - * less than 400 or greater than 599, returns 500; otherwise, returns it. - * - * @param mixed $error - * @param Http\Response $response - * @return int - */ - private function getStatusCode($error, Http\Response $response) - { - if ($error instanceof Exception - && ($error->getCode() >= 400 && $error->getCode() < 600) - ) { - return $error->getCode(); - } - - $status = $response->getStatusCode(); - if (! $status || $status < 400 || $status >= 600) { - $status = 500; - } - return $status; - } - private function createDevelopmentErrorMessage($error) { if ($error instanceof Exception) { diff --git a/src/Utils.php b/src/Utils.php index 534acf5..a08481c 100644 --- a/src/Utils.php +++ b/src/Utils.php @@ -9,6 +9,7 @@ namespace Zend\Stratigility; +use Exception; use ReflectionFunction; use ReflectionMethod; @@ -58,4 +59,32 @@ public static function getArity($callable) $r = new ReflectionFunction($callable); return $r->getNumberOfRequiredParameters(); } + + /** + * Determine status code from an error and/or response. + * + * If the error is an exception with a code between 400 and 599, returns + * the exception code. + * + * Otherwise, retrieves the code from the response; if not present, or + * less than 400 or greater than 599, returns 500; otherwise, returns it. + * + * @param mixed $error + * @param Http\Response $response + * @return int + */ + public static function getStatusCode($error, Http\Response $response) + { + if ($error instanceof Exception + && ($error->getCode() >= 400 && $error->getCode() < 600) + ) { + return $error->getCode(); + } + + $status = $response->getStatusCode(); + if (! $status || $status < 400 || $status >= 600) { + $status = 500; + } + return $status; + } } From 7742f8ba67ddbf465ff073aae604f632e611fc9e Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 23 Jun 2015 12:50:35 -0500 Subject: [PATCH 2/8] Typehint on PSR-7 ResponseInterface - instead of package-specific decorators --- src/Utils.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Utils.php b/src/Utils.php index a08481c..df71685 100644 --- a/src/Utils.php +++ b/src/Utils.php @@ -10,6 +10,7 @@ namespace Zend\Stratigility; use Exception; +use Psr\Http\Message\ResponseInterface; use ReflectionFunction; use ReflectionMethod; @@ -70,10 +71,10 @@ public static function getArity($callable) * less than 400 or greater than 599, returns 500; otherwise, returns it. * * @param mixed $error - * @param Http\Response $response + * @param ResponseInterface $response * @return int */ - public static function getStatusCode($error, Http\Response $response) + public static function getStatusCode($error, ResponseInterface $response) { if ($error instanceof Exception && ($error->getCode() >= 400 && $error->getCode() < 600) From 3778a7fd522d0e49090b856d0eba4d0e0f65b55e Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Wed, 24 Jun 2015 09:20:21 -0500 Subject: [PATCH 3/8] Updated CHANGELOG for #13 --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98332e8..8e60bec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,11 @@ details. ### Added -- Nothing. +- [#13](https://github.com/zendframework/zend-stratigility/pull/13) adds + `Utils::getStatusCode($error, ResponseInterface $response)`; this static + method will attempt to use an exception code as an HTTP status code, if it + falls in a valid HTTP error status range. If the error is not an exception, it + ensures that the status code is an error status. ### Deprecated From 0983f98ae6dbbce7d85e6c478c1716cefd048edb Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 23 Jun 2015 09:02:28 -0500 Subject: [PATCH 4/8] FinalHandler should return new responses If no error is present, and the response passed on invocation is different than the response at initialization, the `FinalHandler` should return that response intact. This allows middleware to create a response, but pass it on to `$next()` for post-processing further up the chain. In particular, this is useful for things like routing middleware, so as to allow for application-wide concerns such as logging, finalizing sessions, etc., without requiring onion-style middleware for these behaviors. --- src/FinalHandler.php | 133 ++++++++++++++++++++++++++++++------ src/MiddlewarePipe.php | 2 +- test/FinalHandlerTest.php | 11 +++ test/MiddlewarePipeTest.php | 32 +++++++++ 4 files changed, 155 insertions(+), 23 deletions(-) diff --git a/src/FinalHandler.php b/src/FinalHandler.php index 870e6d8..ff477f4 100644 --- a/src/FinalHandler.php +++ b/src/FinalHandler.php @@ -10,6 +10,8 @@ namespace Zend\Stratigility; use Exception; +use Psr\Http\Message\RequestInterface; +use Psr\Http\Message\ResponseInterface; use Zend\Escaper\Escaper; /** @@ -22,34 +24,55 @@ class FinalHandler */ private $options; + /** + * Original response provided to the middleware. + * + * @var null|Http\Response + */ + private $response; + /** * @param array $options Options that change default override behavior. + * @param null|ResponseInterface $response Original response, if any. */ - public function __construct(array $options = []) + public function __construct(array $options = [], ResponseInterface $response = null) { - $this->options = $options; + $this->options = $options; + $this->response = $response; } /** * Handle incomplete requests * * This handler should only ever be invoked if Next exhausts its stack. - * When that happens, we determine if an $err is present, and, if so, - * create a 500 status with error details. * - * Otherwise, a 404 status is created. + * When that happens, one of three possibilities exists: + * + * - If an $err is present, create a 500 status with error details. + * - If the instance composes a response, and it differs from the response + * passed during invocation, return the invocation response; this is + * indicative of middleware calling $next to allow post-processing of + * a populated response. + * - Otherwise, a 404 status is created. * - * @param Http\Request $request Request instance. - * @param Http\Response $response Response instance. + * @param RequestInterface $request Request instance. + * @param ResponseInterface $response Response instance. * @param mixed $err - * @return Http\Response + * @return ResponseInterface */ - public function __invoke(Http\Request $request, Http\Response $response, $err = null) + public function __invoke(RequestInterface $request, ResponseInterface $response, $err = null) { if ($err) { return $this->handleError($err, $request, $response); } + // Return provided response if it does not match the one provided at + // instantiation; this is an indication of calling `$next` in the fina + // registered middleware and providing a new response instance. + if ($this->response && $this->response !== $response) { + return $response; + } + return $this->create404($request, $response); } @@ -59,11 +82,11 @@ public function __invoke(Http\Request $request, Http\Response $response, $err = * Use the $error to create details for the response. * * @param mixed $error - * @param Http\Request $request Request instance. - * @param Http\Response $response Response instance. + * @param RequestInterface $request Request instance. + * @param ResponseInterface $response Response instance. * @return Http\Response */ - private function handleError($error, Http\Request $request, Http\Response $response) + private function handleError($error, RequestInterface $request, ResponseInterface $response) { $response = $response->withStatus( Utils::getStatusCode($error, $response) @@ -76,7 +99,7 @@ private function handleError($error, Http\Request $request, Http\Response $respo $message = $this->createDevelopmentErrorMessage($error); } - $response = $response->end($message); + $response = $this->completeResponse($response, $message); $this->triggerError($error, $request, $response); @@ -86,24 +109,41 @@ private function handleError($error, Http\Request $request, Http\Response $respo /** * Create a 404 status in the response * - * @param Http\Request $request Request instance. - * @param Http\Response $response Response instance. + * @param RequestInterface $request Request instance. + * @param ResponseInterface $response Response instance. * @return Http\Response */ - private function create404(Http\Request $request, Http\Response $response) + private function create404(RequestInterface $request, ResponseInterface $response) { $response = $response->withStatus(404); - $originalRequest = $request->getOriginalRequest(); - $uri = $originalRequest->getUri(); + $uri = $this->getUriFromRequest($request); $escaper = new Escaper(); $message = sprintf( "Cannot %s %s\n", $escaper->escapeHtml($request->getMethod()), $escaper->escapeHtml((string) $uri) ); - return $response->end($message); + + return $this->completeResponse($response, $message); } + /** + * Create a complete error message for development purposes. + * + * Creates an error message with full error details: + * + * - If the error is an exception, creates a message that includes the full + * stack trace. + * - If the error is an object that defines `__toString()`, creates a + * message by casting the error to a string. + * - If the error is not an object, casts the error to a string. + * - Otherwise, cerates a generic error message indicating the class type. + * + * In all cases, the error message is escaped for use in HTML. + * + * @param mixed $error + * @return string + */ private function createDevelopmentErrorMessage($error) { if ($error instanceof Exception) { @@ -122,11 +162,17 @@ private function createDevelopmentErrorMessage($error) /** * Trigger the error listener, if present * + * If no `onerror` option is present, or if it is not callable, does + * nothing. + * + * If the request is not an Http\Request, casts it to one prior to invoking + * the error handler. + * * @param mixed $error - * @param Http\Request $request + * @param RequestInterface $request * @param Http\Response $response */ - private function triggerError($error, Http\Request $request, Http\Response $response) + private function triggerError($error, RequestInterface $request, Http\Response $response) { if (! isset($this->options['onerror']) || ! is_callable($this->options['onerror']) @@ -135,6 +181,49 @@ private function triggerError($error, Http\Request $request, Http\Response $resp } $onError = $this->options['onerror']; - $onError($error, $request, $response); + $onError( + $error, + ($request instanceof Http\Request) ? $request : new Http\Request($request), + $response + ); + } + + /** + * Retrieve the URI from the request. + * + * If the request instance is a Stratigility decorator, pull the URI from + * the original request; otherwise, pull it directly. + * + * @param RequestInterface $request + * @return \Psr\Http\Message\UriInterface + */ + private function getUriFromRequest(RequestInterface $request) + { + if ($request instanceof Http\Request) { + $original = $request->getOriginalRequest(); + return $original->getUri(); + } + + return $request->getUri(); + } + + /** + * Write the given message to the response and mark it complete. + * + * If the message is an Http\Response decorator, call and return its + * `end()` method; otherwise, decorate the response and `end()` it. + * + * @param ResponseInterface $response + * @param string $message + * @return Http\Response + */ + private function completeResponse(ResponseInterface $response, $message) + { + if ($response instanceof Http\Response) { + return $response->end($message); + } + + $response = new Http\Response($response); + return $response->end($message); } } diff --git a/src/MiddlewarePipe.php b/src/MiddlewarePipe.php index 0728ffb..9ab423a 100644 --- a/src/MiddlewarePipe.php +++ b/src/MiddlewarePipe.php @@ -71,7 +71,7 @@ public function __invoke(Request $request, Response $response, callable $out = n $request = $this->decorateRequest($request); $response = $this->decorateResponse($response); - $done = $out ?: new FinalHandler(); + $done = $out ?: new FinalHandler([], $response); $next = new Next($this->pipeline, $done); $result = $next($request, $response); diff --git a/test/FinalHandlerTest.php b/test/FinalHandlerTest.php index 9c10975..75bbca3 100644 --- a/test/FinalHandlerTest.php +++ b/test/FinalHandlerTest.php @@ -121,4 +121,15 @@ public function test404ResponseIncludesOriginalRequestUri() $response = call_user_func($final, $request, $this->response, null); $this->assertContains($originalUrl, (string) $response->getBody()); } + + public function testReturnsResponseIfItDoesNotMatchResponsePassedToConstructor() + { + $psrResponse = new PsrResponse(); + $originalResponse = new Response($psrResponse); + $final = new FinalHandler([], $originalResponse); + + $passedResponse = new Response($psrResponse); + $result = $final(new Request(new PsrRequest()), $passedResponse); + $this->assertSame($passedResponse, $result); + } } diff --git a/test/MiddlewarePipeTest.php b/test/MiddlewarePipeTest.php index 2ae400b..bc415e3 100644 --- a/test/MiddlewarePipeTest.php +++ b/test/MiddlewarePipeTest.php @@ -441,4 +441,36 @@ public function testNestedMiddlewareMatchesOnlyAtPathBoundaries($topPath, $neste ) ); } + + /** + * Test that FinalHandler is passed the original response. + * + * Tests that MiddlewarePipe passes the original response passed to it when + * creating the FinalHandler instance, and that FinalHandler compares the + * response passed to it on invocation to its original response. + * + * If the two differ, the response passed during invocation should be + * returned unmodified; this is an indication that a middleware has provided + * a response, and is simply passing further up the chain to allow further + * processing (e.g., to allow an application-wide logger at the end of the + * request). + * + * @group nextChaining + */ + public function testPassesOriginalResponseToFinalHandler() + { + $request = new Request([], [], 'http://local.example.com/foo', 'GET', 'php://memory'); + $response = new Response(); + $test = new Response(); + + $pipeline = new MiddlewarePipe(); + $pipeline->pipe(function ($req, $res, $next) use ($test) { + return $next($req, $test); + }); + + // Pipeline MUST return response passed to $next if it differs from the + // original. + $result = $pipeline($request, $response); + $this->assertSame($test, $result); + } } From e13dd06b30a003bbaa491fda841a365cd494bf3d Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 23 Jun 2015 09:30:44 -0500 Subject: [PATCH 5/8] Fix typehint on `$response` property --- src/FinalHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/FinalHandler.php b/src/FinalHandler.php index ff477f4..86dc3cb 100644 --- a/src/FinalHandler.php +++ b/src/FinalHandler.php @@ -27,7 +27,7 @@ class FinalHandler /** * Original response provided to the middleware. * - * @var null|Http\Response + * @var null|ResponseInterface */ private $response; From ab9ca14ffb5dabf551304a3af9c249411c32e5de Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Wed, 24 Jun 2015 16:44:10 -0500 Subject: [PATCH 6/8] Documented #12 --- doc/book/api.md | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/doc/book/api.md b/doc/book/api.md index e148c52..0cef296 100644 --- a/doc/book/api.md +++ b/doc/book/api.md @@ -27,8 +27,11 @@ executed for that path and any subpaths. Middleware is executed in the order in which it is piped to the `MiddlewarePipe` instance. `__invoke()` is itself middleware. If `$out` is not provided, an instance of -`Zend\Stratigility\FinalHandler` will be created, and used in the event that the pipe -stack is exhausted. The callable should use the same signature as `Next()`: +`Zend\Stratigility\FinalHandler` will be created, and used in the event that the pipe stack is +exhausted (`MiddlewarePipe` passes the `$response` instance it receives to `FinalHandler` as well, +so that the latter can determine if the response it receives is new). + +The callable should use the same signature as `Next()`: ```php function ( @@ -151,6 +154,10 @@ And, if not calling `$next()`, returning the response instance: return $response; ``` +The `FinalHandler` implementation will check the `$response` instance passed when invoking it +against the instance passed during instantiation, and, if different, return it. As such, `return +$next(/* ... */)` is the recommended workflow. + ### Raising an error condition To raise an error condition, pass a non-null value as the third argument to `$next()`: @@ -172,13 +179,27 @@ function ($request, $response, $next) exhausts itself. It expects three arguments when invoked: a request instance, a response instance, and an error condition (or `null` for no error). It returns a response. -`FinalHandler` allows an optional argument during instantiation, `$options`, an array of options -with which to configure itself. These options currently include: - -- `env`, the application environment. If set to "production", no stack traces will be provided. -- `onerror`, a callable to execute if an error is passed when `FinalHandler` is invoked. The - callable is invoked with the error (which will be `null` in the absence of an error), the request, - and the response, in that order. +`FinalHandler` allows two optional arguments during instantiation + +- `$options`, an array of options with which to configure itself. These options currently include: + - `env`, the application environment. If set to "production", no stack traces will be provided. + - `onerror`, a callable to execute if an error is passed when `FinalHandler` is invoked. The + callable is invoked with the error (which will be `null` in the absence of an error), the request, + and the response, in that order. +- `Psr\Http\Message\ResponseInterface $response`; if passed, it will compare the response passed + during invocation against this instance; if they are different, it will return the response from + the invocation, as this indicates that one or more middleware provided a new response instance. + +Internally, `FinalHandler` does the following on invocation: + +- If `$error` is non-`null`, it creates an error response from the response provided at invocation, + ensuring a 400 or 500 series response is returned. +- If the response at invocation matches the response provided at instantiation, it returns it + without further changes. This is an indication that some middleware at some point in the execution + chain called `$next()` with a new response instance. +- If the response at invocation does not match the response provided at instantiation, or if no + response was provided at instantiation, it creates a 404 response, as the assumption is that no + middleware was capable of handling the request. ## HTTP Messages From 69057f10696cd5ffb83f18d5a903db23c6b05c19 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 25 Jun 2015 08:34:12 -0500 Subject: [PATCH 7/8] Return the original response if the body size has changed. - Per @ezimuel, when the original response body has been written to, `FinalHandler` was still returning the 404 response. This patch ensures that if the body size changes, the response will still be returned. --- src/FinalHandler.php | 21 +++++++++++++++++++++ test/FinalHandlerTest.php | 18 ++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/FinalHandler.php b/src/FinalHandler.php index 86dc3cb..70f6f8a 100644 --- a/src/FinalHandler.php +++ b/src/FinalHandler.php @@ -19,6 +19,13 @@ */ class FinalHandler { + /** + * Original response body size. + * + * @var int + */ + private $bodySize = 0; + /** * @var array */ @@ -39,6 +46,10 @@ public function __construct(array $options = [], ResponseInterface $response = n { $this->options = $options; $this->response = $response; + + if ($response) { + $this->bodySize = $response->getBody()->getSize(); + } } /** @@ -73,6 +84,16 @@ public function __invoke(RequestInterface $request, ResponseInterface $response, return $response; } + // If the response passed is the same as the one at instantiation, + // check to see if the body size has changed; if it has, return + // the response, as the message body has been written to. + if ($this->response + && $this->response === $response + && $this->bodySize !== $response->getBody()->getSize() + ) { + return $response; + } + return $this->create404($request, $response); } diff --git a/test/FinalHandlerTest.php b/test/FinalHandlerTest.php index 75bbca3..44f322a 100644 --- a/test/FinalHandlerTest.php +++ b/test/FinalHandlerTest.php @@ -122,6 +122,9 @@ public function test404ResponseIncludesOriginalRequestUri() $this->assertContains($originalUrl, (string) $response->getBody()); } + /** + * @group 12 + */ public function testReturnsResponseIfItDoesNotMatchResponsePassedToConstructor() { $psrResponse = new PsrResponse(); @@ -132,4 +135,19 @@ public function testReturnsResponseIfItDoesNotMatchResponsePassedToConstructor() $result = $final(new Request(new PsrRequest()), $passedResponse); $this->assertSame($passedResponse, $result); } + + /** + * @group 12 + */ + public function testReturnsResponseIfBodyLengthHasChanged() + { + $psrResponse = new PsrResponse(); + $response = new Response($psrResponse); + $final = new FinalHandler([], $response); + + $response->write('return this response'); + + $result = $final(new Request(new PsrRequest()), $response); + $this->assertSame($response, $result); + } } From a3441987f24e4205133cec43b0c68e32591c5baa Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 25 Jun 2015 10:00:34 -0500 Subject: [PATCH 8/8] Added CHANGELOG for #12 --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23c3273..9be17a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,11 @@ details. ### Fixed -- Nothing. +- [#12](https://github.com/zendframework/zend-stratigility/pull/12) updates + `FinalHandler` such that it will return the response provided at invocation + if it differs from the response at initialization (i.e., a new response + instance, or if the body size has changed). This allows you to safely call + `$next()` from all middleware in order to allow post-processing. ## 1.0.3 - TBD