zendframework / zend-expressive

PSR-15 middleware in minutes!
BSD 3-Clause "New" or "Revised" License
710 stars 197 forks source link

Factory backed pipelines cannot be composed/nested in other pipelines #645

Closed gsteel closed 5 years ago

gsteel commented 5 years ago

I'm trying to compose pipelines with MiddlewareFactory and MiddlewareContainer where pipelines are retrieved from the container and composed into another pipeline for routing or whatever. For example, the pipe to process might look something like this:

Code to reproduce the issue

I created a failing test case illustrating what I'm trying to achieve: https://gist.github.com/gsteel/f6a5b2a45fd1630139e907e75fc94299

Expected results

I'd expect the nested Pipe (Pipe 2) to execute, followed by Pipe 3 then the final handler.

Actual results

A Zend\Stratigility\EmptyPipelineException is thrown once Pipe 2 has been exhausted.

Workaround

If Pipe 2 is retrieved from the container and the instance is composed into pipe 1 instead of it's identifier in the container then processing continues as expected, but this means that pipe cannot be lazy loaded

weierophinney commented 5 years ago

Okay, I've been able to create a small project that has the dependencies necessary to run the test, and verified the behavior.

For those who have clicked through to the gist, it's the second test that fails. Essentially, the first middleware executes, and then it descends into the nested pipeline, which executes correctly - but once complete, it does not return to the parent middleware.

This is quite odd, as this is almost exactly the sort of behavior that occurs when you run routed pipelines, which definitely work!

I need to investigate more here to determine if the issue is with how the MiddlewareFactory is creating the nested pipeline, or if there's an issue within MiddlewarePipe that needs to be resolved. I'll post more when I've had more time to investigate.

weierophinney commented 5 years ago

I figured out the problem: https://github.com/zendframework/zend-expressive/blob/master/src/MiddlewareContainer.php#L66-L68

What happens here is that MiddlewarePipe is also a RequestHandlerInterface, which means that pipelines then get cast to RequestHandlerMiddleware, which then calls handle() on the pipeline, instead of process().

Locally, I changed that to:

if ($middleware instanceof RequestHandlerInterface
    && ! $middleware instanceof MiddlewareInterface
) {
    $middleware = new RequestHandlerMiddleware($middleware);
}

This allows both of your tests to pass, which is good. Next step is to figure out a succinct way to reproduce the issue as a unit test, at which point I can write a patch.

gsteel commented 5 years ago

That's great! Thanks ever so much - That test I wrote is a country mile from succinct I'll agree 😂