Modify middleware listener to use stratigility pipe #217

asgrim · 2017-02-19T12:35:53Z

Taking issue #196 into play, this PR proposes to update the MiddlewareListener to use Stratigility to allow piped middleware. This means the middleware param can become an array and everything is pushed into the MiddlewarePipe... as let's be fair, only allowing just a single middleware is only marginally useful.

For this to work, zendframework/zend-stratigility needs to become a requirement for those using MiddlewareListener (I added it as require-dev for now), so could be seen as a BC break potentially; so happy to negotiate on how we'd like this added to composer.json. Otherwise, I believe (pending CI results) all the other tests still pass though.

asgrim · 2017-02-19T12:36:14Z

Apparently I didn't have an up to date develop branch >.<

asgrim · 2017-02-19T12:40:25Z

Rebased correctly now \o/

Ocramius · 2017-02-23T18:56:29Z

src/MiddlewareListener.php

+                $psr7Request,
+                $psr7ResponsePrototype,
+                function (PsrServerRequestInterface $request, PsrResponseInterface $response) {
+                    throw new ReachedFinalHandlerException(


@weierophinney are you OK with moving this to a static named constructor instead? Asking because consistency...

I am, but I'm not convinced it's needed, as there's a static message with no dynamic substitutions, and no additional behavior.

One potential refactor I see here is to instead define ReachedFinalHandlerException as follows:

class ReachedFinalHandlerException extends RuntimeException { public static function createFinalHandler() { return new self('Reached the final handler for middleware pipe - check the pipe configuration'); } public function __invoke() { throw $this; } }

This would then allow you to do the following above:

$return = $pipe($psr7Request, $psr7Response, ReachedFinalHandlerException::createFinalHandler());

Not sure if that's abusing inheritance and SRP, though. 😁

Yikes, I'm not really sure about the exception itself being the final handler, although I gotta say it's quite clever... does feel a bit wrong. I'll just refactor to named constructor for now and if you want to change it to be the actual final handler, I'm happy to :)

Ocramius · 2017-02-23T18:58:21Z

src/MiddlewareListener.php

+
+        $middlewareName = 'noMiddlewarePiped';
+        $middlewareToBePiped = null;
+        foreach ($middlewaresToBePiped as $middlewareToBePiped) {


Segregate to a private method.

Probably should just return $pipe in the private method.

Ocramius · 2017-02-23T18:58:37Z

src/MiddlewareListener.php

+        $pipe = new MiddlewarePipe();
+        $pipe->setResponsePrototype($psr7ResponsePrototype);
+
+        $middlewaresToBePiped = !is_array($middleware) ? [$middleware] : $middleware;


Don't negate: invert the ternary please

Ocramius · 2017-02-23T19:01:45Z

src/MiddlewareListener.php

+        $middlewareName = 'noMiddlewarePiped';
+        $middlewareToBePiped = null;
+        foreach ($middlewaresToBePiped as $middlewareToBePiped) {
+            $middlewareName = is_string($middlewareToBePiped) ? $middlewareToBePiped : get_class($middlewareToBePiped);


Because get_class() is stupid this check is insufficient

Didn't change this from the original code...

Ocramius · 2017-02-23T19:02:25Z

src/MiddlewareListener.php

+        foreach ($middlewaresToBePiped as $middlewareToBePiped) {
+            $middlewareName = is_string($middlewareToBePiped) ? $middlewareToBePiped : get_class($middlewareToBePiped);
+
+            if (is_string($middlewareToBePiped) && $serviceManager->has($middlewareToBePiped)) {


is_string() check is duplicate

Not sure what to do about it; it's just the flexibility around using a string as the middleware (load from container), or a callable (not ideal, but allowed)

Ocramius · 2017-02-23T19:02:49Z

src/MiddlewareListener.php

+            if (is_string($middlewareToBePiped) && $serviceManager->has($middlewareToBePiped)) {
+                $middlewareToBePiped = $serviceManager->get($middlewareToBePiped);
+            }
+            if (! is_callable($middlewareToBePiped)) {


Should probably also NOT be a string?

An example is an incompatible function name being passed in.

If it's a string, it won't be a string any more. Also just following same logic that was already there...

Ocramius · 2017-02-23T19:05:15Z

src/MiddlewareListener.php

@@ -75,7 +99,9 @@ public function onDispatch(MvcEvent $event)
            $event->setName(MvcEvent::EVENT_DISPATCH_ERROR);
            $event->setError($application::ERROR_EXCEPTION);
            $event->setController($middlewareName);
-            $event->setControllerClass(get_class($middleware));
+            if (null !== $middlewareToBePiped) {
+                $event->setControllerClass(get_class($middlewareToBePiped));


You always assume that the last piped middleware is the "responding" one here... Probably needs documenting/commenting

Removed these as there's no longer just one middleware, so doesn't make sense. I guess this causes a definite BC break which is kinda rubbish though?

Ocramius · 2017-02-23T19:05:55Z

src/MiddlewareListener.php

+        $middlewaresToBePiped = !is_array($middleware) ? [$middleware] : $middleware;
+
+        $middlewareName = 'noMiddlewarePiped';
+        $middlewareToBePiped = null;


Can an empty pipeline be considered valid?

I don't see why not; you'll just reach the final handler anyway...

asgrim · 2017-02-25T13:27:47Z

@Ocramius some changes applied as requested.

asgrim · 2017-02-25T13:34:02Z

shakes fist at PHP 5.6 support

Close zendframework#232

weierophinney · 2017-04-27T16:53:42Z

composer.json

@@ -27,7 +27,8 @@
        "zendframework/zend-json": "^2.6.1 || ^3.0",
        "zendframework/zend-psr7bridge": "^0.2",
        "fabpot/php-cs-fixer": "1.7.*",
-        "phpunit/phpunit": "^4.5"
+        "phpunit/phpunit": "^4.5",
+        "zendframework/zend-stratigility": "^2.0"


This should likely be ^2.0.1, but I can take care of that during merge.

weierophinney · 2017-04-27T16:56:02Z

src/MiddlewareListener.php

+                $psr7Request,
+                $psr7ResponsePrototype,
+                function (PsrServerRequestInterface $request, PsrResponseInterface $response) {
+                    throw new ReachedFinalHandlerException(


I am, but I'm not convinced it's needed, as there's a static message with no dynamic substitutions, and no additional behavior.

weierophinney · 2017-04-27T16:56:30Z

src/MiddlewareListener.php

@@ -74,8 +97,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));


Why are these lines removed?

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)

Ah, right - that makes sense, then.

weierophinney · 2017-04-27T17:00:36Z

src/MiddlewareListener.php

+            if (is_string($middlewareToBePiped) && $serviceLocator->has($middlewareToBePiped)) {
+                $middlewareToBePiped = $serviceLocator->get($middlewareToBePiped);
+            }
+            if (! is_callable($middlewareToBePiped)) {


This needs to change to be:

if (! $middlewareToBePiped instanceof \Http\Interop\ServerMiddleware\MiddlewareInterface && ! is_callable($middlewareToBePiped) )

as zend-stratigility 2.0 allows either of those types. Testing only for is_callable() means that http-interop middleware cannot be used!

Once you make those changes, you'll also need to add tests for piping http-interop middleware.

weierophinney · 2017-04-27T17:04:52Z

src/MiddlewareListener.php

+                $psr7Request,
+                $psr7ResponsePrototype,
+                function (PsrServerRequestInterface $request, PsrResponseInterface $response) {
+                    throw new ReachedFinalHandlerException(


One potential refactor I see here is to instead define ReachedFinalHandlerException as follows:

class ReachedFinalHandlerException extends RuntimeException { public static function createFinalHandler() { return new self('Reached the final handler for middleware pipe - check the pipe configuration'); } public function __invoke() { throw $this; } }

This would then allow you to do the following above:

$return = $pipe($psr7Request, $psr7Response, ReachedFinalHandlerException::createFinalHandler());

Not sure if that's abusing inheritance and SRP, though. 😁

weierophinney · 2017-04-27T17:07:06Z

Also, @asgrim — You'll need to rebase against latest develop branch, as I pushed a bunch of QA tooling changes earlier (PHPUnit 5.7/6.0 support, in particular, as well as zend-coding-standards integration).

… instance for consistency

asgrim · 2017-04-28T16:30:14Z

test/MiddlewareListenerTest.php

+    {
+        $expectedOutput = uniqid('expectedOutput', true);
+
+        $event = $this->createMvcEvent('path', new class($expectedOutput) implements MiddlewareInterface {


bah, doesn't work on PHP 5.6 - I thought develop branches were min PHP 7.1 now? Maybe I didn't read properly?

…rategyFactory

asgrim · 2017-04-30T08:59:08Z

@weierophinney all updated, thanks for review!

@param

…g-strategy Fixes @param & @return DefaultRenderingStrategy typo at HttpDefaultRenderingStrategyFactory

Forward port zendframework#237 Conflicts: src/Service/HttpDefaultRenderingStrategyFactory.php

weierophinney

Few last nit-picks, but looks great!

weierophinney · 2017-05-01T15:45:08Z

src/MiddlewareListener.php

@@ -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(


@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.

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)

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. 😄

weierophinney · 2017-05-01T15:46:11Z

src/Exception/MiddlewareNotCallableException.php

+
+namespace Zend\Mvc\Exception;
+
+final class MiddlewareNotCallableException extends RuntimeException


Since we're allowing either callable or http-interop middleware, perhaps we should change this to InvalidMiddlewareException instead?

weierophinney · 2017-05-01T15:47:16Z

src/MiddlewareListener.php

+        $pipe->setResponsePrototype($responsePrototype);
+        foreach ($middlewaresToBePiped as $middlewareToBePiped) {
+            if (null === $middlewareToBePiped) {
+                throw new \InvalidArgumentException('Middleware name cannot be null');


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

Requested changes incorporated.

weierophinney

Per discussion in Slack, I'll perform the last requested changes now and merge.

Thanks, @asgrim!

…stener-to-use-stratigility-pipe Modify middleware listener to use stratigility pipe

Also, adds a `fromNull()` method, allowing it to be used in cases where a `null` value is provided for middleware. This means that `null` middleware is handled exactly the same way as any other invalid middleware type, which required changes to several test expectations.

Instead of `__invoke()`, for forwards compatibility once PSR-15 is ratified.

Close #217 Fixes #196

weierophinney · 2017-05-01T16:40:24Z

Thanks, @asgrim

asgrim force-pushed the modify-middleware-listener-to-use-stratigility-pipe branch from 0e0a650 to 2f947e8 Compare February 19, 2017 12:39

Ocramius previously requested changes Feb 23, 2017

View reviewed changes

Ocramius assigned asgrim Feb 23, 2017

Ocramius added the enhancement label Feb 23, 2017

Ocramius added this to the 3.1.0 milestone Feb 23, 2017

Ocramius added the awaiting author updates label Feb 23, 2017

asgrim removed the awaiting author updates label Mar 6, 2017

asgrim removed their assignment Mar 6, 2017

Merge branch 'hotfix/232'

2c64d2e

Close zendframework#232

weierophinney suggested changes Apr 27, 2017

View reviewed changes

asgrim self-assigned this Apr 28, 2017

asgrim added 12 commits April 28, 2017 16:36

Require stratigility in dev, and suggestion

2712a99

Added tests for handling an array of middleware to be piped

cab17a2

Added implementation of Stratigility middleware pipe

24201cb

Updated RotueMatch#getParams calls to be expected in tests

ab63639

Extract logic to create middleware pipe into private method

81c5f2e

Do not allow a null middleware

6f9c271

Remove null coalesce operator because still need to support PHP 5.6

7e89d6a

Added support for HttpInterop style middlewares

cebe041

Refactored ReachedFinalHandlerException to use named constructor

d0debca

Fixed typo in phpdoc for MvcEvent

6073ca2

Switch MiddlewareNotCallableExceptionTest assertions to be invoked on…

b05cd8b

… instance for consistency

Fixed incorrectly rebased composer dependencies

7d89900

asgrim force-pushed the modify-middleware-listener-to-use-stratigility-pipe branch from b8511d3 to 7d89900 Compare April 28, 2017 16:04

Use namespaced PHPUnit import

4c393f1

Use expectException instead of deprecated setExpectedException

03ffda0

asgrim commented Apr 28, 2017

View reviewed changes

samsonasik and others added 2 commits April 29, 2017 07:48

Fixes @return DefaultRenderingStrategy typo at HttpDefaultRenderingSt…

4116803

…rategyFactory

Use mocks instead of anon classes for middlewares

4dbdb4b

asgrim removed their assignment Apr 30, 2017

weierophinney added 3 commits May 1, 2017 10:36

Merge pull request zendframework#237 from samsonasik/@return-renderin…

2a4a644

…g-strategy Fixes @param & @return DefaultRenderingStrategy typo at HttpDefaultRenderingStrategyFactory

Added CHANGELOG for zendframework#237

7e5c560

Merge branch 'hotfix/237' into develop

00bdd8c

Forward port zendframework#237 Conflicts: src/Service/HttpDefaultRenderingStrategyFactory.php

weierophinney suggested changes May 1, 2017

View reviewed changes

weierophinney approved these changes May 1, 2017

View reviewed changes

weierophinney added 3 commits May 1, 2017 10:59

Merge pull request zendframework#217 from asgrim/modify-middleware-li…

bf741df

…stener-to-use-stratigility-pipe Modify middleware listener to use stratigility pipe

Use http-interop process()

6a4bf9f

Instead of `__invoke()`, for forwards compatibility once PSR-15 is ratified.

weierophinney merged commit 6a4bf9f into zendframework:develop May 1, 2017

weierophinney added a commit that referenced this pull request May 1, 2017

Added CHANGELOG for #217

9c4632e

weierophinney added a commit that referenced this pull request May 1, 2017

Updated middleware listener documentation to reflect #217

665dfbe

weierophinney added a commit that referenced this pull request May 1, 2017

Merge branch 'feature/217' into develop

9935a57

Close #217 Fixes #196

asgrim deleted the modify-middleware-listener-to-use-stratigility-pipe branch May 1, 2017 16:40

Ocramius mentioned this pull request May 1, 2017

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

Merged


		namespace Zend\Mvc\Exception;

		final class MiddlewareNotCallableException extends RuntimeException

Modify middleware listener to use stratigility pipe #217

Modify middleware listener to use stratigility pipe #217

Conversation

asgrim commented Feb 19, 2017

asgrim commented Feb 19, 2017

asgrim commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgrim commented Feb 25, 2017

asgrim commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Apr 27, 2017

Choose a reason for hiding this comment

asgrim commented Apr 30, 2017

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

weierophinney commented May 1, 2017