Requests with an unmatched route do not contain the RouteResult

[nesl247]

I found this bug when I was tracking down why an internal application was throwing an error in one of my post-route-matched middleware to configure NewRelic. To my surprise, a route that is not matched does not actually throw an exception.

I have two opinions on how to fix it.

The first option is that an exception should be thrown in Application::routeMiddleware() (https://github.com/zendframework/zend-expressive/blob/master/src/Application.php#L405) because the rest of the application is kinda done at this point. This is the option that I prefer because Application::dispatchMiddleware() (https://github.com/zendframework/zend-expressive/blob/master/src/Application.php#L446) already depends upon a RouteResult, thus one would likely have to have two routers both supported expressive in order for the second option to really make sense.

The other option is that this isn't done because technically the pipeline is still there, and anyone can have other middleware to handle a route. While this might be the case for some people, it seems really odd to have two routers that both would inject a RouteResult into the request.

If an exception is not the desired way to fix this, then we should really always inject the RouteResult into the request, and change dispatchMiddleware() to check isFailure() rather than just assuming the result being in the request means it was a success. The justification for this is that it is called RouteResult and even has a success property, indicating it does not guarantee a success.

The other benefit to use an exception is that all post-route-matched middleware in the pipeline don't have to make a check for whether or not the route was successful or not, as they probably shouldn't be executed anyways. If the exception isn't to be used, the comment in the middleware-pipeline.global.php should probably state that you should probably call the isFailure() method and and if it is true, return $next($request, $response) or throw an exception (if that's the intended behavior).

I would have submitted a PR for this, but since there are two ways to fix it, it's not worth submitting one if it won't be used due to the other method being preferred. Once discussed I can submit one.

[moderndeveloperllc]

@nesl247 I think the logic here was that an unmatched route would be handled by a middleware further down the stack. The issue (and I have the same one) is if any middleware calls something like below BEFORE the 404 handler is called :

$route = $request->getAttribute(RouteResult::class);

On a failed route match, this returns a null instead of a RouteResult with $this->success = false. So instead of being able to do an isFailure() check, you would need to check if array_key_exists() - not optimal I would think.

@weierophinney Can you further explain the logic of letting non-routable URLs pass through the routing and dispatching? It is necessary to just have a 404-handling middleware right after ApplicationFactory::ROUTING_MIDDLEWARE in the routing pipeline?

Here is a use case where you would need a RouteResult between the two.:

middleware-pipeline.global.php

'routing' => [
            'middleware' => [
                ApplicationFactory::ROUTING_MIDDLEWARE,
                Helper\UrlHelperMiddleware::class,
                App\Action\AuthenticationAction::class,
                App\Action\AuthorizationAction::class,
                ApplicationFactory::DISPATCH_MIDDLEWARE,
            ],
            'priority'   => 1,
        ],

If we only want to run authentication checks on certain routes, AuthenticationAction would have something like:

$route = $request->getAttribute(RouteResult::class);
if (in_array($route->getMatchedMiddleware(), self::NON_AUTH_ACTION)) {
    return $next($request, $response);
}

I think @nesl247 and I were expecting that $route would exist regardless of the URL matching a route, but that's not the case because Application::routeMiddleware only adds that attribute on a successful route match.

[geerteltink]

If there is no matched route there is no RouteResult.

To me it sounds pretty logical. However an exception should be thrown by the FinalHandler if the middleware stack is exhausted, and no middleware has returned a response. The logic in the final handler should kick in and recover gracefully by returning a 404 response since no middleware was able to handle the request.

What I do is check for the RouteResult where needed and if there is none the middleware returns early. Basically the same as Application::dispatchMiddleware() is doing it.

I think one of the reasons that there is no Exception thrown if there is no route might be that you can have a sort of "catch all route". e.g. In case no route matches, it checks the database if a document matches the current path. If an exception would be thrown you can't have that after the main routes are checked.

You can always add middleware that checks for the RouteResult and throws an exception, right after ApplicationFactory::ROUTING_MIDDLEWARE and before anything else tries to access the RouteResult.

Also I think submitting a PR might be useless because it would mean a BC break. Changes have been made already to the Stratigility develop branch which changes the error handler completely (The FinalHandler is removed). I'm not sure about the timeframe but eventually it should make its way into the next version of Expressive.

[nesl247]

Why is it logical that there is no RouteResult, especially when it has a method to check if it's a failure or not? It's a RouteResult, not a RouteMatch after all.

[geerteltink]

Like I said, if there is no route, I don't expect a RouteResult. I guess it's something personal.

Why there are isSuccess() and isFailure() methods? AFAIK they were only used by the route result observers, which are deprecated.

The RouteResult wasn't available publicly since the beginning, it is injected into the Request since version 1.0.0.rc3.

[moderndeveloperllc]

I agree that there is a logic to not returning a RouteResult, but I just wanted to make sure that's the "official" position so I know I won't be coding against a bug. Practically speaking, testing against isFailure() vs null is equivalent. I would say that if the pattern is to "manually" check for a 404, then I wonder about the 405 (Method Not Allowed) checking that is going on in Application::routeMiddleware(). Should that too be moved to Application::dispatchMiddleware() to allow for a "catch all" middleware that responds to missing routes (404) or missing methods (405)?

Either way, it would be nice if the documentation reflected the fact that any middleware put between ApplicationFactory::ROUTING_MIDDLEWARE and ApplicationFactory::DISPATCH_MIDDLEWARE will need to check for the existence of RouteResult if it is using any logic based off of routing (something UrlHelperMiddleware::__invoke() is doing already FWIW).

[moderndeveloperllc]

@xtreamwayz Have you by chance tested out the Strategility 2.0.0-dev branch with Expressive develop branch? I'm already not letting anything through to the FinalHandler and I already do something rather similar to ErrorHandler as I'm using Monolog to catch errors/exceptions. Just wanted to see if there are known issues before I test the dev branches out.

[geerteltink]

I don't know what the "official" position is but taken from that PR:

On successful routing, inject the route result as the Zend\Expressive\Router\RouteResult request attribute.

I would interpret that as it only injects the RouteResult if a route is matched and if there is no route nothing is injected into the Request object. For an "official" statement you need to wait for @weierophinney to reply.

I agree that the documentation should explain this a lot better. Probably the whole chapter about Route Result Observers can be replaced with more info about the RouteResult.

Have you by chance tested out the Strategility 2.0.0-dev branch with Expressive develop branch?

I've been playing with it but it doesn't work yet. So far only the changes to Stratigility have been made and some Expressive parts need to be rewritten first. However you can sort of mimic the new error handling if you don't setup Expressive via configuration: https://github.com/weierophinney/mwop.net/blob/master/public/index.php#L27-L35

[weierophinney]

@nesl247 —

The first option is that an exception should be thrown in Application::routeMiddleware() (https://github.com/zendframework/zend-expressive/blob/master/src/Application.php#L405) because the rest of the application is kinda done at this point.

I disagree with this. It's entirely possible to add middleware that executes
after this point in order to handle requests that were unroutable. With #396,
this becomes even more apparent, as we place a NotFoundHandler after the
dispatch middleware; you could place something else there, such as bridge
middleware that will execute a legacy application (ZF1, ZF2, Symfony 1 or 2,
etc.), providing a way for you to gradually migrate an existing application to
Expressive.

If an exception is not the desired way to fix this, then we should really always inject the RouteResult into the request, and change dispatchMiddleware() to check isFailure() rather than just assuming the result being in the request means it was a success.

I like this idea.

HOWEVER...

It'd be a breaking change at this point. The reason is thus: currently, if
routing fails, no route result is injected, so middleware currently does the
following:

if (! ($result = $request->getAttribute(RouteResult::class, false))) {
    return $next($request, $response);
}

If we always inject, that now has to become:

$result = $request->getAttribute(RouteResult::class, false);
if (! $result || $result->isFailure()) {
    return $next($request, $response);
}

If that change is not made, the code will continue to work, but it could lead to
odd edge cases, as the various getMatched*() methods now return boolean false
instead of what you might expect. This is particularly problematic in the case
of getMatchedMiddleware(), as you might expect to be able to call it
immediately.

---

@moderndeveloperllc —

Can you further explain the logic of letting non-routable URLs pass through the routing and dispatching? It is necessary to just have a 404-handling middleware right after ApplicationFactory::ROUTING_MIDDLEWARE in the routing pipeline?

Dispatch must occur after routing, but dispatch only works if a route result
is present (or, with the change proposed above, successful). However, as noted
above, one use case for allowing non-routable requests to continue through the
pipeline is to allow other handlers to act when routing fails.

If the "not found" middleware were to be between routing and dispatch, what
happens in such cases? Custom middleware would still need to call $next,
meaning the dispatch middleware still executes.

---

Based on the above observations, my plan will be to add documentation indicating
that the routing middleware DOES NOT inject a route result when routing fails,
and that you can thus check for that using:

if (! ($result = $request->getAttribute(RouteResult::class, false))) {
    return $next($request, $response);
}

[moderndeveloperllc]

my plan will be to add documentation indicating
that the routing middleware DOES NOT inject a route result when routing fails

Great! The docs looked great in the other commit.

If the "not found" middleware were to be between routing and dispatch, what
happens in such cases? Custom middleware would still need to call $next,
meaning the dispatch middleware still executes.

@weierophinney I can see how this should be the default, but is there anything wrong with throwing an Exception to be caught by ErrorHandler (Stratigility 1.3/2.0 implementation)? I'm using an ErrorResponseGenerator on that for all my error responses

/** @var RouteResult $route */
$route = $request->getAttribute(RouteResult::class);
if (!$route instanceof RouteResult) {
    throw new BadRouteException('Route Not Found', StatusCodeInterface::STATUS_NOT_FOUND);
}

I'm doing this because I have four middleware between routing and dispatch that depend on the route being set. I would rather throw early than short circuit in all those classes so it falls through to the dispatch and NotFoundHandler.

[weierophinney]

@moderndeveloperllc once we have #396 and 1.1 in place, that'll be an excellent approach. Until then, doing so triggers either error middleware or the final handler, both of which are being deprecated.

[moderndeveloperllc]

@weierophinney Thanks. I'm already doing this as I've moved to the http-interop style of doing middleware (via the __invoke() --> process() hybrid pattern). I just wanted to make sure I wasn't coding down a dead end.