Skip to content

Last middleware can invoke next #300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jsor wants to merge 2 commits into from
Closed

Last middleware can invoke next #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d8aba8
Add test for last middleware invoking $next()
jsor Jan 16, 2018
67a32f2
Check and abort if last middleware invoked $next()
jsor Jan 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Io/MiddlewareRunner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ public function call(ServerRequestInterface $request, $position)
return $that->call($request, $position + 1);
};

if (!isset($this->middleware[$position])) {
return null;
Copy link
Member

@clue clue Jan 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about returning null here as this would break the middleware specification (see README).

It's my understanding that this would be an error, so it should probably throw instead (php.net/manual/en/class.outofrangeexception.php)?

As an alternative, we could also use this to create a "default response", i.e. some kind of response factory and just return a "default response" here?

Copy link
Member Author

@jsor jsor Jan 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is, that it should behave like the middleware just didn't return anything because $next() invoked from the last middleware is just no-op because there is no next.

$middleware = new MiddlewareRunner(array(
    function (RequestInterface $request, $next) {
        // no-op
    }
));

Since the MiddlewareRunner is internal and null will produce an error, i think that this is an acceptable behaviour.

If we throw, we probably should explicitly document, that calling $next() from the last middleware is forbidden or just not pass $next() to the last middleware.

Copy link
Member

@clue clue Jan 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[…] there is no next. […] just not pass $next() to the last middleware.

Now that you've said this, I think this actually makes most sense. The last request handler in the chain should not be allowed to call the "next".

Copy link
Member Author

@jsor jsor Jan 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't pass $next, then the last middleware callable must have a different signature. I'd rather then pass a $next() which throws.

Copy link
Member

@clue clue Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're raising a very good point here. I've just filed #308 to implement this as suggested. Let me know what you think 👍

}

$handler = $this->middleware[$position];
return $handler($request, $next);
}
Expand Down
15 changes: 15 additions & 0 deletions tests/Io/MiddlewareRunnerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,4 +454,19 @@ function (RequestInterface $request) use ($once) {
$this->assertTrue($promise instanceof CancellablePromiseInterface);
$promise->cancel();
}

public function testLastMiddlewareCanInvokeNext()
{
$middleware = new MiddlewareRunner(array(
function (RequestInterface $request, $next) {
return $next($request);
}
));

$request = new ServerRequest('GET', 'http://example.com/');

$response = $middleware($request);

$this->assertNull($response);
}
}