MatchResult is not predictable

[DASPRiD]

Currently, we do have a MatchResultInterface, but that only defines whether the match was a success or not. But for anything further, the user has to make instanceof calls against it, which is a little dirty.

While the match result architecture was build really flexible, it doesn't make much sense to have more return types than those which we have already defined. Now the question would be how we could model a generic match result, which fulfils the following requirements:

• On success, contain the matched route name and the matched parameters
• On generic failure, no further data are required
• On method-not-allowed failure, contain the allowed methods
• On scheme-not-allowed failure, contain the alternate (canonical) URI

I'm open to any suggestions!

[weierophinney]

For predictability and ease of usage, it's best to have methods return a single result type. As such, based on the criteria you outlined above, I'd likely define MatchResultInterface as follows:

interface MatchResultInterface
{
    /**
     * @return bool
     */
    public function isSuccess();

    /**
     * @return bool
     */
    public function isMethodFailure();

    /**
     * @return bool
     */
    public function isSchemeFailure();

    /**
     * @param return string Route name
     * @throws DomainException If no route was matched.
     */
    public function getRoute();

    /**
     * @param return array Matched routing parameters, if any.
     * @throws DomainException If no route was matched.
     */
    public function getParams();

    /**
     * @param return array Allowed HTTP methods for 
     * @throws DomainException If no route was matched.
     */
    public function getAllowedMethods();

    /**
     * @param return string|\Psr\Http\Message\UriInterface Canonical URI
     * @throws DomainException If no route was matched.
     */
    public function getCanonicalUri();
}

Some notes:

• getRoute() should likely return the route name in the event of both method-not-allowed and scheme-not-allowed failures; a route was matched, but additional criteria was not met. (Same argument for getParams().)
• I'd argue that getAllowedMethods() should also be populated for any match.
• Likewise, getCanonicalUri().

The above allows the consumer to interact with the result in meaningful and predictable ways:

if ($result->isSuccess()) {
    foreach ($result->getParams() as $key => $value) {
        $request = $request->withAttribute($key, $value);
    }

    return $next($request, $response);
}

if ($result->isMethodFailure()) {
    return $response->withStatus(405)
        ->withHeader('Allow', implode(',', $result->getAllowedMethods()));
}

if ($result->isSchemeFailure()) {
    return $response->withStatus(301)
        ->withHeader('Location', $result->getCanonicalUri());
}

// oops! Big failure! Handle that differently!

This benefits from code-completion in IDEs, and makes learning usage reasonable (one class to look at, single behavior methods).

[DASPRiD]

@weierophinney Thanks a lot for your input, that does make sense. Just a few minor remarks:

• getRoute() should probably be named getRouteName()?
• getCanonicalUri() sounds quite generic to me and doesn't seem to reflect what it's actually for, maybe there's a better name for it?
• Lastly, only related to your usage example: as per RFC 7538, the scheme redirect should likely be done with HTTP code 308, which means a permanent redirect while keeping the request method and body.

[weierophinney]

• re: getRouteName(); agreed.
• re: getCanonicalUri(), I was just using your own verbiage. :smile: It depends on how it's used, really. For success matches, or method-not-allowed, it would be the URI that resulted in the request. For scheme-not-allowed, it would be the URI with the updated scheme. "Canonical" fits both cases, I think.
• Re: RFC 7538; D'oh! Forgot about that one! Yes, of course.

[DASPRiD]

@weierophinney Sounds all good to me then. Only question left would be if we actually need an interface or if we just make a single implementation.

[danizord]

the user has to make instanceof calls against it, which is a little dirty.

Why instanceof checks are dirty and is*Failure() are not?

To be honest I'd prefer to have a type for each special case - just following the open-closed principle.

[weierophinney]

@danizord — the primary reason against having multiple types is that consumers must branch based on implementation, which is contrary to SOLID design; it requires that I know every single possible return type possible in order to work with the result. This makes both consumption and testing far more difficult; what should I mock, exactly? and how many different types should I mock against to ensure my consuming code is robust? What happens if in the future, additional types are introduced? Having a single, known type with specific, known behaviors is predictable and reduces error.

@DASPRiD — with Expressive, we went with a concrete result type, no interface. As you note, since this is a value object created and returned by the router itself, there's no real reason for an interface, as it encapsulates all possible permutations of behavior.