Merge branch 'feature/next-response-behavior' into develop

Close #12

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
 

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
 

src/FinalHandler.php

@@ -10,46 +10,90 @@
 namespace Zend\Stratigility;
 
 use Exception;
+use Psr\Http\Message\RequestInterface;
+use Psr\Http\Message\ResponseInterface;
 use Zend\Escaper\Escaper;
 
 /**
  * Handle incomplete requests
  */
 class FinalHandler
 {
+    /**
+     * Original response body size.
+     *
+     * @var int
+     */
+    private $bodySize = 0;
+
     /**
      * @var array
      */
     private $options;
 
+    /**
+     * Original response provided to the middleware.
+     *
+     * @var null|ResponseInterface
+     */
+    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;
+
+        if ($response) {
+            $this->bodySize = $response->getBody()->getSize();
+        }
     }
 
     /**
      * 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:
      *
-     * @param Http\Request $request Request instance.
-     * @param Http\Response $response Response instance.
+     * - 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 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;
+        }
+
+        // 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);
     }
 
@@ -59,11 +103,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 +120,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 +130,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 +183,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 +202,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);
     }
 }

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);
 

test/FinalHandlerTest.php

@@ -121,4 +121,33 @@ public function test404ResponseIncludesOriginalRequestUri()
         $response = call_user_func($final, $request, $this->response, null);
         $this->assertContains($originalUrl, (string) $response->getBody());
     }
+
+    /**
+     * @group 12
+     */
+    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);
+    }
+
+    /**
+     * @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);
+    }
 }

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);
+    }
 }