Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Fix: allow middleware dispatch to behave like controller dispatch #236

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c27997f
`MiddlewareListener` should cancel any previous dispatch errors set i…
Ocramius Apr 28, 2017
5a914bd
Removing route match mock - not needed
Ocramius Apr 28, 2017
ecab36d
Canceling previous `MvcEvent` errors when successfully dispatching a …
Ocramius Apr 28, 2017
cb4d7ee
Correcting assertion, since `MvcEvent#getError()` is always a `string`
Ocramius Apr 28, 2017
0af1831
Verifying that the `MiddlewareListener` will relay anything the middl…
Ocramius Apr 28, 2017
606ce94
Using a real service manager in the `MiddlewareListenerTest` - no tes…
Ocramius Apr 28, 2017
1ab5b13
Removed traditional `var_dump()` statement
Ocramius Apr 28, 2017
71066ad
Creating an intermediate `MiddlewareController` that makes any middle…
Ocramius Apr 28, 2017
a6d23ff
Using the `MiddlewareListener` within the context of a `zend-mvc` dis…
Ocramius Apr 28, 2017
3c3136a
A shared listener should be attached to middleware, so that the entir…
Ocramius Apr 28, 2017
a46ae7e
Added missing docblock for a test class property
Ocramius Apr 28, 2017
a37be06
The `MiddlewareListener` should bail out if an MVC result is already …
Ocramius May 1, 2017
0ce89d1
Skipping dispatch if an `MvcEvent#getResult()` is not null (fixes tests)
Ocramius May 1, 2017
533a489
Verifying that the `MiddlewareListener` triggers no error events on s…
Ocramius May 1, 2017
e78b531
The dispatch listener should not interfere when `MvcEvent#getResult()…
Ocramius May 1, 2017
2d2f887
Skipping dispatch when a non-null `MvcEvent#getResult()` is detected
Ocramius May 1, 2017
396a650
Corrected rebase conflicts - moved middleware pipe dispatch into the …
Ocramius May 1, 2017
50ec228
Optimised imports
Ocramius May 1, 2017
16a77c3
Removing unused callable parameters
Ocramius May 1, 2017
e4824a3
Splitting request attribute population into a private method
Ocramius May 1, 2017
7d33ff7
Splitting request checks into their own method
Ocramius May 1, 2017
6170700
Removed unused imports
Ocramius May 1, 2017
98c6d39
Removing test that specifies that allowing anything as the return typ…
Ocramius May 1, 2017
8442764
Documenting the reason for the `MiddlewareController` to exist
Ocramius May 1, 2017
29d8e5b
Basic test coverage for the `MiddlewareController`
Ocramius May 1, 2017
145e300
Dispatching a `MiddlewareController` with an invalid request type sho…
Ocramius May 1, 2017
4628c44
Removed unused imports
Ocramius May 1, 2017
b7f52a9
Corrected `EventManagerInterface` type-hint
Ocramius May 1, 2017
a122791
CS (whitespace)
Ocramius May 1, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 122 additions & 0 deletions src/Controller/MiddlewareController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/

namespace Zend\Mvc\Controller;

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Zend\EventManager\EventManagerInterface;
use Zend\Http\Request;
use Zend\Mvc\Exception\ReachedFinalHandlerException;
use Zend\Mvc\Exception\RuntimeException;
use Zend\Mvc\MvcEvent;
use Zend\Psr7Bridge\Psr7ServerRequest;
use Zend\Router\RouteMatch;
use Zend\Stratigility\Delegate\CallableDelegateDecorator;
use Zend\Stratigility\MiddlewarePipe;

/**
* @internal don't use this in your codebase, or else @ocramius will hunt you down. This is just an internal
* @internal hack to make middleware trigger 'dispatch' events attached to the DispatchableInterface identifier.
*
* Specifically, it will receive a @see MiddlewarePipe, a @see ResponseInterface prototype, and then dispatch
* the pipe whilst still behaving like a normal controller. That is needed for any events attached to
* the @see \Zend\Stdlib\DispatchableInterface identifier to reach their listeners on any attached
* @see \Zend\EventManager\SharedEventManagerInterface
*/
final class MiddlewareController extends AbstractController
{
/**
* @var MiddlewarePipe
*/
private $pipe;

/**
* @var ResponseInterface
*/
private $responsePrototype;

public function __construct(
MiddlewarePipe $pipe,
ResponseInterface $responsePrototype,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not particularly proud of this, but PSR-7's immutability makes it quite acceptable to pass in a request as a prototype. Still, this class is designed as "fire once" and "internal" specifically due to this weirdness.

EventManagerInterface $eventManager,
MvcEvent $event
) {
$this->eventIdentifier = __CLASS__;
$this->pipe = $pipe;
$this->responsePrototype = $responsePrototype;

$this->setEventManager($eventManager);
$this->setEvent($event);
}

/**
* {@inheritDoc}
*
* @throws RuntimeException
*/
public function onDispatch(MvcEvent $e)
{
$routeMatch = $e->getRouteMatch();
$psr7Request = $this->populateRequestParametersFromRoute(
$this->loadRequest()->withAttribute(RouteMatch::class, $routeMatch),
$routeMatch
);

$result = $this->pipe->process($psr7Request, new CallableDelegateDecorator(
function () {
throw ReachedFinalHandlerException::create();
},
$this->responsePrototype
));

$e->setResult($result);

return $result;
}

/**
* @return \Zend\Diactoros\ServerRequest
*
* @throws RuntimeException
*/
private function loadRequest()
{
$request = $this->request;

if (! $request instanceof Request) {
throw new RuntimeException(sprintf(
'Expected request to be a %s, %s given',
Request::class,
get_class($request)
));
}

return Psr7ServerRequest::fromZend($request);
}

/**
* @param ServerRequestInterface $request
* @param RouteMatch|null $routeMatch
*
* @return ServerRequestInterface
*/
private function populateRequestParametersFromRoute(ServerRequestInterface $request, RouteMatch $routeMatch = null)
{
if (! $routeMatch) {
return $request;
}

foreach ($routeMatch->getParams() as $key => $value) {
$request = $request->withAttribute($key, $value);
}

return $request;
}
}
4 changes: 4 additions & 0 deletions src/DispatchListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ public function attach(EventManagerInterface $events, $priority = 1)
*/
public function onDispatch(MvcEvent $e)
{
if (null !== $e->getResult()) {
return;
}

$routeMatch = $e->getRouteMatch();
$controllerName = $routeMatch instanceof RouteMatch
? $routeMatch->getParam('controller', 'not-found')
Expand Down
24 changes: 13 additions & 11 deletions src/MiddlewareListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
use Zend\EventManager\EventManagerInterface;
use Zend\Mvc\Exception\InvalidMiddlewareException;
use Zend\Mvc\Exception\ReachedFinalHandlerException;
use Zend\Psr7Bridge\Psr7ServerRequest as Psr7Request;
use Zend\Mvc\Controller\MiddlewareController;
use Zend\Psr7Bridge\Psr7Response;
use Zend\Router\RouteMatch;
use Zend\Stratigility\Delegate\CallableDelegateDecorator;
Expand All @@ -45,6 +45,10 @@ public function attach(EventManagerInterface $events, $priority = 1)
*/
public function onDispatch(MvcEvent $event)
{
if (null !== $event->getResult()) {
return;
}

$routeMatch = $event->getRouteMatch();
$middleware = $routeMatch->getParam('middleware', false);
if (false === $middleware) {
Expand Down Expand Up @@ -78,16 +82,12 @@ public function onDispatch(MvcEvent $event)

$caughtException = null;
try {
$psr7Request = Psr7Request::fromZend($request)->withAttribute(RouteMatch::class, $routeMatch);
foreach ($routeMatch->getParams() as $key => $value) {
$psr7Request = $psr7Request->withAttribute($key, $value);
}
$return = $pipe->process($psr7Request, new CallableDelegateDecorator(
function (PsrServerRequestInterface $request, PsrResponseInterface $response) {
throw ReachedFinalHandlerException::create();
},
$psr7ResponsePrototype
));
$return = (new MiddlewareController(
$pipe,
$psr7ResponsePrototype,
$application->getServiceManager()->get('EventManager'),
$event
))->dispatch($request, $response);
} catch (\Throwable $ex) {
$caughtException = $ex;
} catch (\Exception $ex) { // @TODO clean up once PHP 7 requirement is enforced
Expand All @@ -107,6 +107,8 @@ function (PsrServerRequestInterface $request, PsrResponseInterface $response) {
}
}

$event->setError('');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this still means that the DispatchListener will try to execute and fail. The patch is not yet done. Ideally, the DispatchListener should check whether there is a $event->getResult(), and bail out if there already is one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue both the DispatchListener and MiddlewareListener should act that way, with the following at the top of the listeners:

if (null !== $e->getResult()) {
    return;
}

Do you want to incorporate that change in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weierophinney done for both listeners


if (! $return instanceof PsrResponseInterface) {
$event->setResult($return);
return $return;
Expand Down
147 changes: 147 additions & 0 deletions test/Controller/MiddlewareControllerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/

namespace ZendTest\Mvc\Controller;

use PHPUnit\Framework\TestCase;
use Psr\Http\Message\ResponseInterface;
use Zend\EventManager\EventManager;
use Zend\EventManager\EventManagerInterface;
use Zend\Http\Request;
use Zend\Http\Response;
use Zend\Mvc\Controller\AbstractController;
use Zend\Mvc\Controller\MiddlewareController;
use Zend\Mvc\Exception\RuntimeException;
use Zend\Mvc\MvcEvent;
use Zend\Stdlib\DispatchableInterface;
use Zend\Stdlib\RequestInterface;
use Zend\Stratigility\MiddlewarePipe;

/**
* @covers \Zend\Mvc\Controller\MiddlewareController
*/
class MiddlewareControllerTest extends TestCase
{
/**
* @var MiddlewarePipe|\PHPUnit_Framework_MockObject_MockObject
*/
private $pipe;

/**
* @var ResponseInterface|\PHPUnit_Framework_MockObject_MockObject
*/
private $responsePrototype;

/**
* @var EventManagerInterface
*/
private $eventManager;

/**
* @var AbstractController|\PHPUnit_Framework_MockObject_MockObject
*/
private $controller;

/**
* @var MvcEvent
*/
private $event;

/**
* {@inheritDoc}
*/
protected function setUp()
{
$this->pipe = $this->createMock(MiddlewarePipe::class);
$this->responsePrototype = $this->createMock(ResponseInterface::class);
$this->eventManager = $this->createMock(EventManagerInterface::class);
$this->event = new MvcEvent();
$this->eventManager = new EventManager();

$this->controller = new MiddlewareController(
$this->pipe,
$this->responsePrototype,
$this->eventManager,
$this->event
);
}

public function testWillAssignCorrectEventManagerIdentifiers()
{
$identifiers = $this->eventManager->getIdentifiers();

self::assertContains(MiddlewareController::class, $identifiers);
self::assertContains(AbstractController::class, $identifiers);
self::assertContains(DispatchableInterface::class, $identifiers);
}

public function testWillDispatchARequestAndResponseWithAGivenPipe()
{
$request = new Request();
$response = new Response();
$result = $this->createMock(ResponseInterface::class);
/* @var $dispatchListener callable|\PHPUnit_Framework_MockObject_MockObject */
$dispatchListener = $this->getMockBuilder(\stdClass::class)->setMethods(['__invoke'])->getMock();

$this->eventManager->attach(MvcEvent::EVENT_DISPATCH, $dispatchListener, 100);
$this->eventManager->attach(MvcEvent::EVENT_DISPATCH_ERROR, function () {
self::fail('No dispatch error expected');
}, 100);

$dispatchListener
->expects(self::once())
->method('__invoke')
->with(self::callback(function (MvcEvent $event) use ($request, $response) {
self::assertSame($this->event, $event);
self::assertSame(MvcEvent::EVENT_DISPATCH, $event->getName());
self::assertSame($this->controller, $event->getTarget());
self::assertSame($request, $event->getRequest());
self::assertSame($response, $event->getResponse());

return true;
}));

$this->pipe->expects(self::once())->method('process')->willReturn($result);

$controllerResult = $this->controller->dispatch($request, $response);

self::assertSame($result, $controllerResult);
self::assertSame($result, $this->event->getResult());
}

public function testWillRefuseDispatchingInvalidRequestTypes()
{
/* @var $request RequestInterface */
$request = $this->createMock(RequestInterface::class);
$response = new Response();
/* @var $dispatchListener callable|\PHPUnit_Framework_MockObject_MockObject */
$dispatchListener = $this->getMockBuilder(\stdClass::class)->setMethods(['__invoke'])->getMock();

$this->eventManager->attach(MvcEvent::EVENT_DISPATCH, $dispatchListener, 100);

$dispatchListener
->expects(self::once())
->method('__invoke')
->with(self::callback(function (MvcEvent $event) use ($request, $response) {
self::assertSame($this->event, $event);
self::assertSame(MvcEvent::EVENT_DISPATCH, $event->getName());
self::assertSame($this->controller, $event->getTarget());
self::assertSame($request, $event->getRequest());
self::assertSame($response, $event->getResponse());

return true;
}));

$this->pipe->expects(self::never())->method('process');

$this->expectException(RuntimeException::class);

$this->controller->dispatch($request, $response);
}
}
47 changes: 47 additions & 0 deletions test/DispatchListenerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
use Zend\Mvc\MvcEvent;
use Zend\Router\RouteMatch;
use Zend\ServiceManager\ServiceManager;
use Zend\Stdlib\ResponseInterface;
use Zend\View\Model\ModelInterface;

class DispatchListenerTest extends TestCase
{
Expand Down Expand Up @@ -83,4 +85,49 @@ public function testUnlocatableControllerViaAbstractFactory()
$this->assertArrayHasKey('error', $log);
$this->assertSame('error-controller-not-found', $log['error']);
}

/**
* @dataProvider alreadySetMvcEventResultProvider
*
* @param mixed $alreadySetResult
*/
public function testWillNotDispatchWhenAnMvcEventResultIsAlreadySet($alreadySetResult)
{
$event = $this->createMvcEvent('path');

$event->setResult($alreadySetResult);

$listener = new DispatchListener(new ControllerManager(new ServiceManager(), ['abstract_factories' => [
Controller\TestAsset\UnlocatableControllerLoaderAbstractFactory::class,
]]));

$event->getApplication()->getEventManager()->attach(MvcEvent::EVENT_DISPATCH_ERROR, function () {
self::fail('No dispatch failures should be raised - dispatch should be skipped');
});

$listener->onDispatch($event);

self::assertSame($alreadySetResult, $event->getResult(), 'The event result was not replaced');
}

/**
* @return mixed[][]
*/
public function alreadySetMvcEventResultProvider()
{
return [
[123],
[true],
[false],
[[]],
[new \stdClass()],
[$this],
[$this->createMock(ModelInterface::class)],
[$this->createMock(ResponseInterface::class)],
[$this->createMock(Response::class)],
[['view model data' => 'as an array']],
[['foo' => new \stdClass()]],
['a response string'],
];
}
}
Loading