Throw EmptyPipelineException if passed in continuation is invoked more than once.

[alihammad-gist]

• Are you fixing a bug?
  Yes

  • Detail how the bug is invoked currently.
    By calling the handler that is passed in the middlewares more than once.

  • Detail the original, incorrect behavior.
    The handler Next::$fallbackHandler is invoked more than once. The first invocation
    of $handler->handle inside a middleware drains the middleware queue in Next::queue,
    subsequent invocations of $handler->handle invoke Next::$fallbackHandler.

  • Detail the new, expected behavior.
    The handler should be allowed to be invoked only once or the behavior will be inconsistent. The first middleware
    invokes the second middleware in the pipe when calling hanlder::handle first time,
    second time it will be invoking the handler Next::$fallbackHandler.

  • Base your feature on the master branch, and submit against that branch.

  • Add a regression test that demonstrates the bug, and proves the fix.
    Added the smallest failing test, and the code to pass it in NextTest.php . Didn't remove uneeded Next::$fallbackHandler because one of the tests checks of its existence.

  • Add a CHANGELOG.md entry for the fix.

[Xerkus]

This PR can not be accepted as it breaks intended behavior of the Next handler while not addressing the actual issue of allowing multiple invocations.

The problem you experience is due to your usage. Is there a reason why you need to invoke it multiple times?
Normally middleware acts on request then potentially delegates to next handler then acts on the response returned by next handler.

Do you use this middleware outside of its intended usage as a part of middleware pipe?

zend-stratigility/src/MiddlewarePipe.php

Lines 81 to 84 in e62d1c5

	public function process(ServerRequestInterface $request, RequestHandlerInterface $handler) : ResponseInterface
	{
	return (new Next($this->pipeline, $handler))->handle($request);
	}

I will keep this PR open for your feedback.

[alihammad-gist]

All test were passing, is there an implicit Next behavior I am ignoring? I added the test, If the test captures a valid Use case instead of an exception please regect the PR. What is the standard behavior when middleware delegates to the next handler twice? This captures the edge case when a continuation is called twice, you are right about normally middle ware delegates to the next handler (once).

zend-stratigility/test/NextTest.php

Lines 219 to 239 in 90da95e

	public function testCallingContinuationMoreThanOnceRaisesException()
	{
	$this->expectException(EmptyPipelineException::class);
	$fallBackHandler = $this->prophesize(RequestHandlerInterface::class);
	$fallBackHandler
	->handle(Argument::any())
	->shouldBeCalledTimes(1);
	// Middleware calling $handler->handle() twice. The first call will empty the
	// middleware queue while the second call will raise the empty pipline exception.
	$middleware = (new class () implements MiddlewareInterface {
	public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface {
	$handler->handle($request);
	return $handler->handle($request);
	}
	});
	$this->queue->push($middleware);
	$next = new Next($this->queue, $fallBackHandler->reveal());
	$next->handle($this->request);
	}

[Xerkus]

Hm. Next does not provide a way to push to queue once it has been passed to constructor, so one use case I had in mind is out.
That also means that only questionable edge case behavior is affected by pushing final handler to queue. I was too fast to reject it.

Going back to original issue, we have two options here: go ahead and explicitly forbid second invocation or allow multiple invocations.

Second option is easy to achieve by passing cloned Next handler to queued middlewares. Next handler is a hot path though and it will have a performance impact as it will have to be cloned for each piped middleware.

$next = clone $this;
$middleware = $next->queue->dequeue();
return $middleware->process($request, $next);

public function __clone()
{
    $this->queue = clone $this->queue;
}

So, my question stands: Is there a reason why you need to invoke it multiple times?

@weierophinney I would also like your input on the two options outlined above.

[alihammad-gist]

So, my question stands: Is there a reason why you need to invoke it multiple times?

I do not want to invoke it multiple times, I rather have it throw an exception if it is invoked more than once so I am told explicitly that I have done something wrong.

[Xerkus]

After discussion in slack we came to conclusion that Next handler should not be allowed to be invoked multiple times from the same context as it constitutes bad design and leads to unpredictable and/or hard to test and debug results.

[Xerkus]

Summarised the feedback from the slack discussion plus some of the changes i would like to see

[Xerkus]

Please update typehint for Next::$queue property to null|SplQueue

Looking good. Here is final set of changes I would like you to do.

[weierophinney]

Thanks, @alihammad-gist!