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

Ocramius · 2017-04-28T13:53:09Z

This patch makes sure that the successful dispatch of a middleware won't trigger exception and 404 strategies. This can happen when the return value of the MiddlewareListener is not a Zend\Stdlib\ResponseInterface instance, and therefore the EventManager will not short-circuit and stop event propagation.


 * ~~~`['foo' => 'bar']`~~~
 * ~~~`'Hello World!'`~~~
 * ~~~`new ViewModel(['foo' => 'bar'])`~~~

~~~This also fixes https://github.com/zendframework/zendframework/issues/5806~~ ERRATA: it doesn't, as #217 now enforces middleware to return `ResponseInterface` instances.

~~~I'm still stuck on this though. As per tradition `Zend\Mvc\ViewManager` is being absolutely useless, and [attaches all listeners to the shared manager `DispatchableInterface` identifier](https://github.com/zendframework/zend-mvc/blob/2c64d2ed50063e13b23f1c7e19f17ce3354bea36/src/View/Http/ViewManager.php#L124-L152): that means that basically nothing in the view layer works when using this stuff.~~~

~~~Currently preparing a workaround that triggers events that have the `DispatchableInterface` identifier, since otherwise it is impossible to get this thing running correctly.~~~

Ocramius · 2017-04-28T15:05:51Z

src/MiddlewareListener.php

@@ -91,6 +90,8 @@ public function onDispatch(MvcEvent $event)
            }
        }

+        $event->setError('');


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.

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?

@weierophinney done for both listeners

weierophinney

I like this approach a ton; I think triggering events for dispatched middleware in the same way as for controllers could have a ton of benefits.

A few comments:

This is technically new functionality (new class), and I think it would be best to do for the 3.1.0 release. As such, you'd need to rebase against develop... which has a number of changes due to me merging Modify middleware listener to use stratigility pipe #217 earlier today. I'd be happy to help with that if you wish.
The MiddlewareController should also allow http-interop middleware; I have detailed comments on that below.
I agree that this and the DispatchListener should return early if the MvcEvent composes a result already, and think that change should be part of this PR (as this PR is the one that demonstrates the need for that change).

weierophinney · 2017-05-01T16:49:57Z

src/MiddlewareListener.php

@@ -91,6 +90,8 @@ public function onDispatch(MvcEvent $event)
            }
        }

+        $event->setError('');


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?

weierophinney · 2017-05-01T16:51:37Z

src/Controller/MiddlewareController.php

+     */
+    private $middleware;
+
+    public function __construct(callable $middleware, EventManager $eventManager, MvcEvent $event)


Just a note: on the develop branch, we're now targeting Stratigility ^2.0.1, which means we can have:

callable middleware

http-interop middleware

If this patch targets master, we would need to remove the typehint for $middleware, and also update how middleware is invoked based on whether or not it implements Interop\Http\ServerMiddleware\MiddlewareInterface.

weierophinney · 2017-05-01T16:51:55Z

src/Controller/MiddlewareController.php

+            }
+        }
+
+        $result = \call_user_func($this->middleware, $psr7Request, Psr7Response::fromZend($response));


See previous note about http-interop.

weierophinney · 2017-05-01T16:53:05Z

test/MiddlewareListenerTest.php

@@ -76,6 +84,7 @@ public function testSuccessfullyDispatchesMiddleware()
        $application = $event->getApplication();

        $application->getEventManager()->attach(MvcEvent::EVENT_DISPATCH_ERROR, function ($e) {
+            die(var_dump($e->getParam('exception')->getMessage()));


Um, die()? Why not just a var_dump() or capturing that info to a variable to emit with the fail() below?

This is also gone :-)

…n the given `MvcEvent`

…middleware

…eware produces, regardless of its shape/type, unless it is a PSR7-response

…t is mocking anything particular at this point

…ware act like a normal controller

…patch

…e useless view layer can do its terrible view model magic

…present in the dispatched event

…kipped dispatch

…` already yields a non-null result

…controller wrapper

…e of a middleware is OK This is likely going to land into a separate dispatch listener instead

Ocramius · 2017-05-01T18:33:48Z

This is technically new functionality (new class), and I think it would be best to do for the 3.1.0 release. As such, you'd need to rebase against develop... which has a number of changes due to me merging #217 earlier today. I'd be happy to help with that if you wish.

Provided

The MiddlewareController should also allow http-interop middleware; I have detailed comments on that below.

That's also handled. The MiddlewareController, being an internal class, can be changed as much as it suits us.

I agree that this and the DispatchListener should return early if the MvcEvent composes a result already, and think that change should be part of this PR (as this PR is the one that demonstrates the need for that change).

Implemented/tested

…uld raise a `RuntimeException`

Ocramius · 2017-05-01T19:05:53Z

src/Controller/MiddlewareController.php

+
+    public function __construct(
+        MiddlewarePipe $pipe,
+        ResponseInterface $responsePrototype,


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.

…o-behave-like-controller-dispatch Fix: allow middleware dispatch to behave like controller dispatch

Close #236

Ocramius commented Apr 28, 2017

View reviewed changes

weierophinney suggested changes May 1, 2017

View reviewed changes

Ocramius added 23 commits May 1, 2017 20:00

MiddlewareListener should cancel any previous dispatch errors set i…

c27997f

…n the given `MvcEvent`

Removing route match mock - not needed

5a914bd

Canceling previous MvcEvent errors when successfully dispatching a …

ecab36d

…middleware

Correcting assertion, since MvcEvent#getError() is always a string

cb4d7ee

Verifying that the MiddlewareListener will relay anything the middl…

0af1831

…eware produces, regardless of its shape/type, unless it is a PSR7-response

Using a real service manager in the MiddlewareListenerTest - no tes…

606ce94

…t is mocking anything particular at this point

Removed traditional var_dump() statement

1ab5b13

Creating an intermediate MiddlewareController that makes any middle…

71066ad

…ware act like a normal controller

Using the MiddlewareListener within the context of a zend-mvc dis…

a6d23ff

…patch

A shared listener should be attached to middleware, so that the entir…

3c3136a

…e useless view layer can do its terrible view model magic

Added missing docblock for a test class property

a46ae7e

The MiddlewareListener should bail out if an MVC result is already …

a37be06

…present in the dispatched event

Skipping dispatch if an MvcEvent#getResult() is not null (fixes tests)

0ce89d1

Verifying that the MiddlewareListener triggers no error events on s…

533a489

…kipped dispatch

The dispatch listener should not interfere when `MvcEvent#getResult()…

e78b531

…` already yields a non-null result

Skipping dispatch when a non-null MvcEvent#getResult() is detected

2d2f887

Corrected rebase conflicts - moved middleware pipe dispatch into the …

396a650

…controller wrapper

Optimised imports

50ec228

Removing unused callable parameters

16a77c3

Splitting request attribute population into a private method

e4824a3

Splitting request checks into their own method

7d33ff7

Removed unused imports

6170700

Removing test that specifies that allowing anything as the return typ…

98c6d39

…e of a middleware is OK This is likely going to land into a separate dispatch listener instead

Ocramius force-pushed the fix/allow-middleware-dispatch-to-behave-like-controller-dispatch branch from 80200a7 to 98c6d39 Compare May 1, 2017 18:24

Ocramius changed the base branch from master to develop May 1, 2017 18:25

Documenting the reason for the MiddlewareController to exist

8442764

Basic test coverage for the MiddlewareController

29d8e5b

Ocramius added 3 commits May 1, 2017 20:58

Dispatching a MiddlewareController with an invalid request type sho…

145e300

…uld raise a `RuntimeException`

Removed unused imports

4628c44

Corrected EventManagerInterface type-hint

b7f52a9

Ocramius added the enhancement label May 1, 2017

CS (whitespace)

a122791

Ocramius commented May 1, 2017

View reviewed changes

Ocramius changed the title ~~[WIP] Fix: allow middleware dispatch to behave like controller dispatch~~ Fix: allow middleware dispatch to behave like controller dispatch May 1, 2017

weierophinney merged commit a122791 into zendframework:develop May 1, 2017

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

Merge pull request #236 from Ocramius/fix/allow-middleware-dispatch-t…

aa031e8

…o-behave-like-controller-dispatch Fix: allow middleware dispatch to behave like controller dispatch

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

Added CHANGELOG for #236

47a1a11

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

Merge branch 'feature/236' into develop

486ff29

Close #236

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Ocramius commented Apr 28, 2017 •

edited

Loading

Ocramius Apr 28, 2017

weierophinney May 1, 2017

Ocramius May 1, 2017

weierophinney left a comment

weierophinney May 1, 2017

weierophinney May 1, 2017

weierophinney May 1, 2017

weierophinney May 1, 2017

Ocramius May 1, 2017

Ocramius commented May 1, 2017

Ocramius May 1, 2017

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

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

Conversation

Ocramius commented Apr 28, 2017 • edited Loading

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

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

Ocramius commented May 1, 2017

Choose a reason for hiding this comment

Ocramius commented Apr 28, 2017 •

edited

Loading