Skip to content
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.

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

Last middleware can invoke next #300

Closed

Conversation

jsor
Copy link
Member

@jsor jsor commented Jan 16, 2018

Allows the last middleware in MiddlewareRunner to invoke $next().

Example

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

See eg. https://travis-ci.org/jsor-labs/http/jobs/329394945 for a failing test.

@jsor jsor force-pushed the last-middleware-can-invoke-next branch from 0b33c94 to 67a32f2 Compare January 16, 2018 13:05
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Good catch! I think it makes perfect sense to address this 👍

@@ -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

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

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

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

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

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants