Ensure that MiddlewarePipes are called as callables if an error is present #95
Conversation
…esent As reported in zendframework/zend-expressive#416, error middleware nested inside a `MiddlewarePipe` was not being dispatched. This was due to the fact that `Dispatch` was identifying the pipeline as http-interop middleware, and thus dropping the `$err` argument (as interop middleware cannot accept that argument). Theis patch updates `Dispatch` to check if the middleware is a `MiddlewarePipe` and a non-null `$err` is present; if so, it now dispatches it as callable middleware instead of as interop middleware.
| */ | ||
| public function testInvokingWithMiddlewarePipeAndErrorDispatchesNextErrorMiddleware() | ||
| { | ||
| $error = new RuntimeException('expected'); |
There was a problem hiding this comment.
Consider using just Exception. Even better, Throwable may be needed
There was a problem hiding this comment.
I'll rewrite to use a data provider to seed multiple exception/throwable types. Thanks for the feedback!
| @@ -442,6 +442,99 @@ public function testInvokingWithInteropMiddlewareDispatchesIt() | |||
| } | |||
|
|
|||
| /** | |||
| * @todo Remove this test for version 2.0 | |||
There was a problem hiding this comment.
Should probably spawn an issue for 2.0 for this, then link the issue instead
There was a problem hiding this comment.
Also: why is this not needed in 2.0?
There was a problem hiding this comment.
Because this class goes away entirely, and that version will only work with http-interop middleware. 😉
There was a problem hiding this comment.
Which means, really, that this @todo can go away, as the top of the classfile already indicates the entire file goes away. Updating...
| } | ||
|
|
||
| /** | ||
| * @todo Remove this test for version 2.0 |
There was a problem hiding this comment.
Should probably spawn an issue for 2.0 for this, then link the issue instead
There was a problem hiding this comment.
See note above; I'll just remove these, as there's a note at the top of this file already.
- Class and test class go away in 2.0.0 entirely
|
@Ocramius Feedback incorporated; waiting for build, but should be ready for you to review. |
|
@weierophinney awesome, thanks! Note that this cannot be forward ported, as |
Yep - and hence the reason I could omit the TODO items. 😄 Should not affect merging from develop to master later. Thanks for the review; I'll get a tag out shortly. |
|
@weierophinney I'm currently tagging: want me to hold back? |
|
Nope, go ahead, @Ocramius. |
As reported in zendframework/zend-expressive#416, error middleware nested inside a
MiddlewarePipewas not being dispatched. This was due to the fact thatDispatchwas identifying the pipeline as http-interop middleware, and thus dropping the$errargument (as interop middleware cannot accept that argument).Theis patch updates
Dispatchto check if the middleware is aMiddlewarePipeand a non-null$erris present; if so, it now dispatches it as callable middleware instead of as interop middleware.