-
Notifications
You must be signed in to change notification settings - Fork 89
Modify middleware listener to use stratigility pipe #217
Changes from 14 commits
2c64d2e
2712a99
cab17a2
24201cb
ab63639
81c5f2e
6f9c271
7e89d6a
cebe041
d0debca
6073ca2
b05cd8b
7d89900
4c393f1
03ffda0
4116803
4dbdb4b
2a4a644
7e5c560
00bdd8c
bf741df
6d09019
6a4bf9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
<?php | ||
/** | ||
* Zend Framework (http://framework.zend.com/) | ||
* | ||
* @link http://github.com/zendframework/zf2 for the canonical source repository | ||
* @copyright Copyright (c) 2005-2017 Zend Technologies USA Inc. (http://www.zend.com) | ||
* @license http://framework.zend.com/license/new-bsd New BSD License | ||
*/ | ||
|
||
namespace Zend\Mvc\Exception; | ||
|
||
final class MiddlewareNotCallableException extends RuntimeException | ||
{ | ||
/** | ||
* @var string | ||
*/ | ||
private $middlewareName; | ||
|
||
/** | ||
* @param string $middlewareName | ||
* @return self | ||
*/ | ||
public static function fromMiddlewareName($middlewareName) | ||
{ | ||
$middlewareName = (string)$middlewareName; | ||
$instance = new self(sprintf('Cannot dispatch middleware %s', $middlewareName)); | ||
$instance->middlewareName = $middlewareName; | ||
return $instance; | ||
} | ||
|
||
/** | ||
* @return string | ||
*/ | ||
public function toMiddlewareName() | ||
{ | ||
return null !== $this->middlewareName ? $this->middlewareName : ''; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
<?php | ||
/** | ||
* Zend Framework (http://framework.zend.com/) | ||
* | ||
* @link http://github.com/zendframework/zf2 for the canonical source repository | ||
* @copyright Copyright (c) 2005-2017 Zend Technologies USA Inc. (http://www.zend.com) | ||
* @license http://framework.zend.com/license/new-bsd New BSD License | ||
*/ | ||
|
||
namespace Zend\Mvc\Exception; | ||
|
||
class ReachedFinalHandlerException extends RuntimeException | ||
{ | ||
/** | ||
* @return self | ||
*/ | ||
public static function create() | ||
{ | ||
return new self('Reached the final handler for middleware pipe - check the pipe configuration'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,12 +9,19 @@ | |
|
||
namespace Zend\Mvc; | ||
|
||
use Interop\Container\ContainerInterface; | ||
use Interop\Http\ServerMiddleware\MiddlewareInterface; | ||
use Psr\Http\Message\ResponseInterface as PsrResponseInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
use Psr\Http\Message\ServerRequestInterface as PsrServerRequestInterface; | ||
use Zend\EventManager\AbstractListenerAggregate; | ||
use Zend\EventManager\EventManagerInterface; | ||
use Zend\Mvc\Exception\MiddlewareNotCallableException; | ||
use Zend\Mvc\Exception\ReachedFinalHandlerException; | ||
use Zend\Psr7Bridge\Psr7ServerRequest as Psr7Request; | ||
use Zend\Psr7Bridge\Psr7Response; | ||
use Zend\Router\RouteMatch; | ||
use Zend\Stratigility\MiddlewarePipe; | ||
|
||
class MiddlewareListener extends AbstractListenerAggregate | ||
{ | ||
|
@@ -47,15 +54,19 @@ public function onDispatch(MvcEvent $event) | |
$application = $event->getApplication(); | ||
$response = $application->getResponse(); | ||
$serviceManager = $application->getServiceManager(); | ||
$middlewareName = is_string($middleware) ? $middleware : get_class($middleware); | ||
|
||
if (is_string($middleware) && $serviceManager->has($middleware)) { | ||
$middleware = $serviceManager->get($middleware); | ||
} | ||
if (! is_callable($middleware)) { | ||
$psr7ResponsePrototype = Psr7Response::fromZend($response); | ||
|
||
try { | ||
$pipe = $this->createPipeFromSpec( | ||
$serviceManager, | ||
$psr7ResponsePrototype, | ||
is_array($middleware) ? $middleware : [$middleware] | ||
); | ||
} catch (MiddlewareNotCallableException $middlewareNotCallableException) { | ||
$return = $this->marshalMiddlewareNotCallable( | ||
$application::ERROR_MIDDLEWARE_CANNOT_DISPATCH, | ||
$middlewareName, | ||
$middlewareNotCallableException->toMiddlewareName(), | ||
$event, | ||
$application | ||
); | ||
|
@@ -69,7 +80,13 @@ public function onDispatch(MvcEvent $event) | |
foreach ($routeMatch->getParams() as $key => $value) { | ||
$psr7Request = $psr7Request->withAttribute($key, $value); | ||
} | ||
$return = $middleware($psr7Request, Psr7Response::fromZend($response)); | ||
$return = $pipe( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @asgrim So sorry I didn't notice this earlier, but... If we're pinning to Stratigility 2, we should likely use the http-interop May I suggest the following: $pipe->setResponsePrototype($psr7ResponsePrototype);
$return = $pipe->process($psr7Request, new \Zend\Stratigility\Delegate\CallableDelegateDecorator(
function (PsrServerRequestInterface $request, PsrResponseInterface $response) {
throw ReachedFinalHandlerException::create();
}
)); That would accomplish the same as what you have here, and be forwards compatible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this seems reasonable, though I note that
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, okay, nice. As noted, using |
||
$psr7Request, | ||
$psr7ResponsePrototype, | ||
function (PsrServerRequestInterface $request, PsrResponseInterface $response) { | ||
throw ReachedFinalHandlerException::create(); | ||
} | ||
); | ||
} catch (\Throwable $ex) { | ||
$caughtException = $ex; | ||
} catch (\Exception $ex) { // @TODO clean up once PHP 7 requirement is enforced | ||
|
@@ -79,8 +96,6 @@ public function onDispatch(MvcEvent $event) | |
if ($caughtException !== null) { | ||
$event->setName(MvcEvent::EVENT_DISPATCH_ERROR); | ||
$event->setError($application::ERROR_EXCEPTION); | ||
$event->setController($middlewareName); | ||
$event->setControllerClass(get_class($middleware)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these lines removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is potentially no longer just "one" middleware name... how do we decide which is the failed one? First? Last? IIRC I don't think there's any way (outside of the exception bubble here) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right - that makes sense, then. |
||
$event->setParam('exception', $caughtException); | ||
|
||
$events = $application->getEventManager(); | ||
|
@@ -100,6 +115,42 @@ public function onDispatch(MvcEvent $event) | |
return $response; | ||
} | ||
|
||
/** | ||
* Create a middleware pipe from the array spec given. | ||
* | ||
* @param ContainerInterface $serviceLocator | ||
* @param ResponseInterface $responsePrototype | ||
* @param array $middlewaresToBePiped | ||
* @return MiddlewarePipe | ||
* @throws \InvalidArgumentException | ||
* @throws \Zend\Mvc\Exception\MiddlewareNotCallableException | ||
*/ | ||
private function createPipeFromSpec( | ||
ContainerInterface $serviceLocator, | ||
ResponseInterface $responsePrototype, | ||
array $middlewaresToBePiped | ||
) { | ||
$pipe = new MiddlewarePipe(); | ||
$pipe->setResponsePrototype($responsePrototype); | ||
foreach ($middlewaresToBePiped as $middlewareToBePiped) { | ||
if (null === $middlewareToBePiped) { | ||
throw new \InvalidArgumentException('Middleware name cannot be null'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we rename the exception to |
||
} | ||
|
||
$middlewareName = is_string($middlewareToBePiped) ? $middlewareToBePiped : get_class($middlewareToBePiped); | ||
|
||
if (is_string($middlewareToBePiped) && $serviceLocator->has($middlewareToBePiped)) { | ||
$middlewareToBePiped = $serviceLocator->get($middlewareToBePiped); | ||
} | ||
if (! $middlewareToBePiped instanceof MiddlewareInterface && ! is_callable($middlewareToBePiped)) { | ||
throw MiddlewareNotCallableException::fromMiddlewareName($middlewareName); | ||
} | ||
|
||
$pipe->pipe($middlewareToBePiped); | ||
} | ||
return $pipe; | ||
} | ||
|
||
/** | ||
* Marshal a middleware not callable exception event | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<?php | ||
/** | ||
* Zend Framework (http://framework.zend.com/) | ||
* | ||
* @link http://github.com/zendframework/zf2 for the canonical source repository | ||
* @copyright Copyright (c) 2005-2017 Zend Technologies USA Inc. (http://www.zend.com) | ||
* @license http://framework.zend.com/license/new-bsd New BSD License | ||
*/ | ||
|
||
namespace ZendTest\Mvc; | ||
|
||
use PHPUnit\Framework\TestCase; | ||
use Zend\Mvc\Exception\MiddlewareNotCallableException; | ||
|
||
final class MiddlewareNotCallableExceptionTest extends TestCase | ||
{ | ||
public function testFromMiddlewareName() | ||
{ | ||
$middlewareName = uniqid('middlewareName', true); | ||
$exception = MiddlewareNotCallableException::fromMiddlewareName($middlewareName); | ||
|
||
$this->assertInstanceOf(MiddlewareNotCallableException::class, $exception); | ||
$this->assertSame('Cannot dispatch middleware ' . $middlewareName, $exception->getMessage()); | ||
$this->assertSame($middlewareName, $exception->toMiddlewareName()); | ||
} | ||
|
||
public function testToMiddlewareNameWhenNotSet() | ||
{ | ||
$exception = new MiddlewareNotCallableException(); | ||
$this->assertSame('', $exception->toMiddlewareName()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
<?php | ||
/** | ||
* Zend Framework (http://framework.zend.com/) | ||
* | ||
* @link http://github.com/zendframework/zf2 for the canonical source repository | ||
* @copyright Copyright (c) 2005-2017 Zend Technologies USA Inc. (http://www.zend.com) | ||
* @license http://framework.zend.com/license/new-bsd New BSD License | ||
*/ | ||
|
||
namespace ZendTest\Mvc; | ||
|
||
use PHPUnit\Framework\TestCase; | ||
use Zend\Mvc\Exception\ReachedFinalHandlerException; | ||
|
||
final class ReachedFinalHandlerExceptionTest extends TestCase | ||
{ | ||
public function testFromNothing() | ||
{ | ||
$exception = ReachedFinalHandlerException::create(); | ||
|
||
$this->assertInstanceOf(ReachedFinalHandlerException::class, $exception); | ||
$this->assertSame( | ||
'Reached the final handler for middleware pipe - check the pipe configuration', | ||
$exception->getMessage() | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're allowing either callable or http-interop middleware, perhaps we should change this to
InvalidMiddlewareException
instead?