From f3c90f6c2c20d0c7c9f0f6b66ac12b4dd28c4220 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 10 Nov 2016 18:05:46 -0600 Subject: [PATCH 1/3] Ensure Next processes the next delegate correctly when an error is present Discovered when updating tests for Expressive to work with Stratigility 1.3. Essentially, when nesting pipelines, if an inner middleware pipeline is exhausted and a non-null `$err` is present, if the next delegate was a `Next` instance, it was improperly being called as an http-interop `DelegateInterface`, losing the error argument. As such, if any parent delegate contained error middleware, it would never be invoked. --- src/Next.php | 4 +++- test/NextTest.php | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/Next.php b/src/Next.php index 5eaa31c..8717b6a 100644 --- a/src/Next.php +++ b/src/Next.php @@ -396,7 +396,9 @@ private function dispatchNextDelegate( ResponseInterface $response = null, $err = null ) { - if ($nextDelegate instanceof DelegateInterface) { + if ($nextDelegate instanceof DelegateInterface + && (! $nextDelegate instanceof Next || $err === null) + ) { return $nextDelegate->process($request); } diff --git a/test/NextTest.php b/test/NextTest.php index 7a4c7a5..3c55877 100644 --- a/test/NextTest.php +++ b/test/NextTest.php @@ -910,4 +910,39 @@ public function testEnablingRaiseThrowablesFlagWillCauseInvocationToRaiseMiddlew $this->assertTrue($triggered, 'Deprecation notice not triggered'); } + + /** + * @todo Remove for 2.0.0 + * @group error-handling + */ + public function testNestedNextInvocationWithAnErrorShouldDispatchErrorMiddleware() + { + $internalQueue = clone $this->queue; + $internalQueue->enqueue(new Route('/', function ($request, $response, $next) { + return $next($request, $response, 'ERROR'); + })); + + $nextDelegateQueue = clone $this->queue; + $nextDelegateQueue->enqueue(new Route('/', function ($err, $request, $response, $next) { + $response->getBody()->write('ERROR DETECTED'); + return $response->withStatus(599); + })); + + $finalDelegate = $this->prophesize(DelegateInterface::class); + $finalDelegate->process(Argument::any())->shouldNotBeCalled(); + + $nextDelegate = new Next($nextDelegateQueue, $finalDelegate->reveal()); + $internalNext = new Next($internalQueue, $nextDelegate); + + set_error_handler(function ($errno, $errstr) { + return false !== strstr($errstr, 'error middleware is deprecated'); + }, E_USER_DEPRECATED); + + $response = $internalNext($this->request, $this->response); + + restore_error_handler(); + + $this->assertEquals(599, $response->getStatusCode()); + $this->assertEquals('ERROR DETECTED', (string) $response->getBody()); + } } From 33602550f46b6c32a4832e58f03f379eb293c317 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 10 Nov 2016 18:06:04 -0600 Subject: [PATCH 2/3] Added test to ensure that callable middleware is not decorated if no response prototype is present I suspected at one point that callable middleware was being decorated as interop middleware, which would cause problems with manipulated responses. It was not, but this test proves it. --- test/MiddlewarePipeTest.php | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/MiddlewarePipeTest.php b/test/MiddlewarePipeTest.php index 82e2815..3524d0b 100644 --- a/test/MiddlewarePipeTest.php +++ b/test/MiddlewarePipeTest.php @@ -638,6 +638,27 @@ public function testWillDecorateCallableMiddlewareAsInteropMiddlewareIfResponseP $this->assertAttributeSame($this->response, 'responsePrototype', $test); } + /** + * @group http-interop + */ + public function testWillNotDecorateCallableMiddlewareAsInteropMiddlewareIfResponsePrototypeIsNotPresent() + { + $pipeline = new MiddlewarePipe(); + + $middleware = function () { + }; + $pipeline->pipe($middleware); + + $r = new ReflectionProperty($pipeline, 'pipeline'); + $r->setAccessible(true); + $queue = $r->getValue($pipeline); + + $route = $queue->dequeue(); + $test = $route->handler; + $this->assertNotInstanceOf(CallableMiddlewareWrapper::class, $test); + $this->assertInternalType('callable', $test); + } + /** * @todo Remove with 2.0.0 */ From 14079ea2e78ed5e69fdf780bbc24fd5dd413e4f8 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 10 Nov 2016 18:16:01 -0600 Subject: [PATCH 3/3] Added CHANGELOG for #85 --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a13f7c..4086545 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,29 @@ Versions prior to 1.0 were originally released as `phly/conduit`; please visit its [CHANGELOG](https://github.com/phly/conduit/blob/master/CHANGELOG.md) for details. +## 1.3.1 - 2016-11-10 + +### Added + +- Nothing. + +### Deprecated + +- Nothing. + +### Removed + +- Nothing. + +### Fixed + +- [#85](https://github.com/zendframework/zend-stratigility/pull/85) fixes an + issue with how the `$done` or `$nextDelegate` is invoked by `Next` when an + error is present. Previously, the class was detecting a `Next` instance as an + http-interop `DelegateInterface` instance and dropping the error; this would + then mean if the instance contained error middleware, it would never be + dispatched. + ## 1.3.0 - 2016-11-10 ### Added