Return value too ambiguous

[XedinUnknown]

Hi!

Your library is great. So great in fact that Zend Expressive has an adapter for it. So, thank you very much for this.

What I find wrong with it is the resolution value, i.e. the return value of dispatch() (which is already ambiguous because the route is not dispatched, but rather resolved; but anyhow). And by that I mean the weird array: both the type of the returned value, and the non-uniform structure. It complicates logic that consumes dispatch(), and makes type documentation or hinting impossible.

Proposal

IMO, a method must do what it is made to do, or fail. And by fail, I mean throw. So, dispatch() could be changed to either resolve the URL+method combination, or throw an exception. The successful result could be a callable - the callable that is given to the Route as the handler. In the case of error, two exception types may be added to mean the inability to resolve, e.g.

• CouldNotResolveException - No route could be found for the URI+method combination.
• MethodNotAllowedException extends CouldNotResolveException - The "endpoint" was found, but no method matched. This can have a getAllowedMethods(): array<string> method, which would return the array of allowed methods.

The above would allow a structured approach to route resolution, delegation of error handling, stricter type handling. In my case, I used ellipse/dispatcher. I could then much more reliably expect a callable that returns a RequestHandlerInterface.

Further

Optionally, since the above changes would lead to a BC break, you could go even further and change things around a bit. For example, have routes be value objects that expose their URL, method, and handler, and outsource the matching to something else.

[bwoebi]

While I agree on the idea of using value objects, I do not think that using exceptions is the correct way to handle the no-match case.

This is a low-level library providing base routing functionality, not a high-level wrapper who considers a non-match exceptional. A non-match is, from the perspective of this library a very valid outcome and thus shouldn't throw.

[XedinUnknown]

I believe that any function should do what it is meant to do, or throw. The purpose of a router is to resolve a set of parameters to a route. If it cannot be done, then it is a failure, which warrants an exception.

From a different point of view, exceptions are very convenient here to represent a result that is not standard. It is very difficult to normalize the current result of routing (for some reason called "dispatching"), and this is probably why the author has used an array: the possible variations of result cannot be normalized to a set of standard parameters. So, from this point of view, exceptions could provide an excellent way to mean results that are not routes.

[XedinUnknown]

By the way, the argument about this lib being low-level, and therefore shouldn't throw, is a non-argument IMHO. My personal opinion is that things which are low-level should throw even more, because this is the way to pass the details of the problem up the frame stack. But even if not, I wouldn't say that low-level throwing is something unusual or undesirable: take a look at thecodingmachine/safe.

[XedinUnknown]

Also, whether or not this library is low-level is debatable, because it really depends on how/why it is consumed. I can easily imagine an application that consists only of routing with a few handlers, in which case this will be the highest level available in the application.

[nikic]

I generally agree with @bwoebi. It's not really a question of low-level vs. high-level. It's a matter of exceptions being used only for exceptional circumstances. For a routing library, a 404 condition is very much expected. You may think of it this way: If you were not to catch the exception, would that still constitute a reasonable use of the method? If the answer is no, then it likely should not throw.

In addition to that, exceptions involve gathering a stack trace and as such are exceptionally (:P) slow.

[nikic]

The optimal solution for this particular case would be to return an ADT enum and pattern match over it. Of course that's not something that PHP supports :)

[XedinUnknown]

If you were not to catch the exception, would that still constitute a reasonable use of the method?

I think that the use of exceptions is reasonable in itself, a long as they mean failure. Failure to find a route is a failure nonetheless.

Of course this is a matter of opinion. But I really like this kind of use for exceptions, because it allows the program to continue on its happy path, without necessarily having to deal with the problem at the point of consumption. For example, ValidatorInterface#validate() will throw if the argument is invalid, staying true to the name of the method. I have heard the opinion that it should return a true or false, but this has 2 problems: the method should then be called isValid(), and a failure would not communicate the details or reasons. I think this is a similar case. If the method is called getRoute() (I think this is more fitting than dispatch(), because it doesn't dispatch anything, but simply routes), it should return an e.g. RouteInterface, or fail (throw). This will keep the application simple, and the flow straight-forward:

$uri = $_SERVER['REQUEST_URI'];
$method = $_SERVER['REQUEST_METHOD'];
$route = $router->getRoute($uri, $method);
// maybe
$route->dispatch($args);

I think that from this example it is apparent that the failure to route does indeed represent an exceptional scenario, because the flow of logic must change.

[burzum]

Can't we simply use a result object and a few constants? And I agree with @XedinUnknown, the dispatch() method should be renamed to something better. So what about this?

$result = $dispatcher->resolve($httpMethod, $uri);
if ($result->routeMatched()) {
    $route = $result->route(); // get the route object
} else {
    if ($result->error() === Result::NOT_FOUND) {
        /*...*/
    }
    if ($result->error() === Result::NOT_ALLOWED) {
        /*...*/
    }
}

If somebody wants more convenience to reduce the typing additional methods could be added that return bool: routeWasNotFound(), isNotAllowed().

So instead of if ($result->error() === Result::NOT_FOUND) { just if ($result->routeWasNotFound()).

And it would be even possible to do this in a backward compatible way by implementing array access or an toArray() method to turn the result object into an array.

I would prefer an enum over a constant but since php doesn't have proper enums... When php 8 arrives the result object could also implement __toBool() or whatever the name was, I've seen a RFC for that recently.

Another way could be to return a route object or null if no matching route was found, but this would require a much deeper refactor of the code base.

[XedinUnknown]

That really is an "ugly fix". Use of objects in this way is no different to using an array. And if the method is called dispatchVariableRoute(), then it must dispatch a variable route, or fail. And failure is represented in PHP by exceptions, meaning it should throw. In general, exceptions mean a deviation from the happy path. This allows you to write your code as you normally would if everything works, and handle things that don't work wherever that is convenient/practical.

[burzum]

@XedinUnknown an exception doesn't mean failure. The outcome of the routing is deterministic: You either get a match or not there is nothing exceptional to handle here. Using an exception won't save you a single line of code as you need to wrap it with try/catch, same LOC as if / else. Also exceptions come with a cost because they have to collect the trace.

Exceptions should be used for situation where a certain method or function could not execute normally. For example, when it encounters broken input or when a resource (e.g. a file) is unavailable. Use exceptions to signal the caller that you faced an error which you are unwilling or unable to handle.

To find or to not find a route is pretty normal here and expected, nothing exceptional that is unclear or we're unable to handle from the point of view of the router.

The most logic result for a router would be simply to return a route object or null. But there is is legacy stuff, so just returning null won't work without a huge BC breaker. I personally wouldn't care but obviously other people.

[XedinUnknown]

@burzum, I feel you. Thing is, if a method gets a route, then it should be called getRoute(). Then, it should either return the route, or fail with an exception. In this case, the method is dispatch(), and therefore it should either dispatch, or, if unable to dispatch, throw an exception - because if you expect it to dispatch, the inability to dispatch is an exceptional scenario.

[lcobucci]

We've implemented in #267 results objects to handle this (inspired by @burzum's work, thanks).

I didn't rename "dispatch" because I see it as an unnecessary BC-break for now.
We can review that for v3 👍