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

Modify middleware listener to use stratigility pipe #217

Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2c64d2e
Merge branch 'hotfix/232'
weierophinney Apr 27, 2017
2712a99
Require stratigility in dev, and suggestion
asgrim Feb 19, 2017
cab17a2
Added tests for handling an array of middleware to be piped
asgrim Feb 19, 2017
24201cb
Added implementation of Stratigility middleware pipe
asgrim Feb 19, 2017
ab63639
Updated RotueMatch#getParams calls to be expected in tests
asgrim Feb 19, 2017
81c5f2e
Extract logic to create middleware pipe into private method
asgrim Feb 25, 2017
6f9c271
Do not allow a null middleware
asgrim Feb 25, 2017
7e89d6a
Remove null coalesce operator because still need to support PHP 5.6
asgrim Feb 25, 2017
cebe041
Added support for HttpInterop style middlewares
asgrim Apr 28, 2017
d0debca
Refactored ReachedFinalHandlerException to use named constructor
asgrim Apr 28, 2017
6073ca2
Fixed typo in phpdoc for MvcEvent
asgrim Apr 28, 2017
b05cd8b
Switch MiddlewareNotCallableExceptionTest assertions to be invoked on…
asgrim Apr 28, 2017
7d89900
Fixed incorrectly rebased composer dependencies
asgrim Apr 28, 2017
4c393f1
Use namespaced PHPUnit import
asgrim Apr 28, 2017
03ffda0
Use expectException instead of deprecated setExpectedException
asgrim Apr 28, 2017
4116803
Fixes @return DefaultRenderingStrategy typo at HttpDefaultRenderingSt…
samsonasik Apr 29, 2017
4dbdb4b
Use mocks instead of anon classes for middlewares
asgrim Apr 30, 2017
2a4a644
Merge pull request #237 from samsonasik/@return-rendering-strategy
weierophinney May 1, 2017
7e5c560
Added CHANGELOG for #237
weierophinney May 1, 2017
00bdd8c
Merge branch 'hotfix/237' into develop
weierophinney May 1, 2017
bf741df
Merge pull request #217 from asgrim/modify-middleware-listener-to-use…
weierophinney May 1, 2017
6d09019
Rename MiddlewareNotCallableException to InvalidMiddlewareException
weierophinney May 1, 2017
6a4bf9f
Use http-interop process()
weierophinney 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
6 changes: 4 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@
"phpunit/phpunit": "^6.0.7 || ^5.7.14",
"zendframework/zend-coding-standard": "~1.0.0",
"zendframework/zend-json": "^2.6.1 || ^3.0",
"zendframework/zend-psr7bridge": "^0.2"
"zendframework/zend-psr7bridge": "^0.2",
"zendframework/zend-stratigility": "^2.0.1"
},
"suggest": {
"zendframework/zend-json": "(^2.6.1 || ^3.0) To auto-deserialize JSON body content in AbstractRestfulController extensions, when json_decode is unavailable",
Expand All @@ -40,7 +41,8 @@
"zendframework/zend-mvc-plugin-prg": "To provide Post/Redirect/Get functionality within controllers",
"zendframework/zend-paginator": "^2.7 To provide pagination functionality via PaginatorPluginManager",
"zendframework/zend-psr7bridge": "(^0.2) To consume PSR-7 middleware within the MVC workflow",
"zendframework/zend-servicemanager-di": "zend-servicemanager-di provides utilities for integrating zend-di and zend-servicemanager in your zend-mvc application"
"zendframework/zend-servicemanager-di": "zend-servicemanager-di provides utilities for integrating zend-di and zend-servicemanager in your zend-mvc application",
"zendframework/zend-stratigility": "zend-stratigility is required to use middleware pipes in the MiddlewareListener"
},
"config": {
"sort-packages": true
Expand Down
108 changes: 107 additions & 1 deletion composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 38 additions & 0 deletions src/Exception/MiddlewareNotCallableException.php
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
Copy link
Member

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?

{
/**
* @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 : '';
}
}
21 changes: 21 additions & 0 deletions src/Exception/ReachedFinalHandlerException.php
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');
}
}
69 changes: 60 additions & 9 deletions src/MiddlewareListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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
);
Expand All @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The 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 process() method here instead of __invoke(), which means using a DelegateInterface instead of a callable.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this seems reasonable, though I note that __invoke does the wrapping in the CallableDelegateDecorator for us, but I have no strong feelings either way. If __invoke is eventually going to go, then yes the switch makes sense!

setResponsePrototype is already called in createPipeFromSpec btw, so I don't think needs calling again (unless I missed something)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay, nice.

As noted, using process() will be forwards compatible, meaning that when Stratigility adopts a finalized PSR-15, we likely can just add an || to the constraint and not need any actual code changes. I figure, do it once. 😄

$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
Expand All @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

Why are these lines removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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();
Expand All @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

If we rename the exception to InvalidMiddlewareException, we could also add another named constructor: ::becauseNull() (obviously, choose something better!).

}

$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
*
Expand Down
32 changes: 32 additions & 0 deletions test/Exception/MiddlewareNotCallableExceptionTest.php
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());
}
}
27 changes: 27 additions & 0 deletions test/Exception/ReachedFinalHandlerExceptionTest.php
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()
);
}
}
Loading