Make ConfigureRoutes fluent

[codemasher]

Ok, I'm trying this again :)

Now that we have the static return type, we can write pretty fluent interfaces without worrying about messy return types, yay! And while this may be opinionated, it clearly adds value as it allows to write cleaner code:

$dispatcher = FastRoute\simpleDispatcher(function(FastRoute\ConfigureRoutes $routeCollector) {

    $routeCollector->get('/users', 'get_all_users_handler');
    $routeCollector->get('/user/{id:\d+}', 'get_user_handler');
    $routeCollector->get('/articles/{id:\d+}[/{title}]', 'get_article_handler');

    // ...
});

vs.

$dispatcher = FastRoute\simpleDispatcher(function(FastRoute\ConfigureRoutes $routeCollector) {

    $routeCollector
        ->get('/users', 'get_all_users_handler')
        ->get('/user/{id:\d+}', 'get_user_handler')
        ->get('/articles/{id:\d+}[/{title}]', 'get_article_handler')
    ;

    // ...
});

[lcobucci]

@codemasher thanks for your patch!

I'm generally okay with chaining, though I'd prefer to ensure we have a new instance instead of mutating the object...

However, enforcing immutability would likely have some performance impact.

I'll have a look at this later this week and come back to you

[codemasher]

Why not both? Similar to PHP's DateTime and its immutable counterpart? You could just extend RouteCollector into RouteCollectorImmutable and overwrite addRoute() and addGroup().

Update:

class RouteCollectorImmutable extends RouteCollector
{

    public function addRoute(string|array $httpMethod, string $route, mixed $handler, array $extraParameters = []): static
    {
        $route = $this->currentGroupPrefix . $route;
        $parsedRoutes = $this->routeParser->parse($route);

        $extraParameters = [self::ROUTE_REGEX => $route] + $extraParameters;

        $clone = clone $this;

        foreach ((array) $httpMethod as $method) {
            foreach ($parsedRoutes as $parsedRoute) {
                $clone->dataGenerator->addRoute($method, $parsedRoute, $handler, $extraParameters);
            }
        }

        if (array_key_exists(self::ROUTE_NAME, $extraParameters)) {
            $clone->registerNamedRoute($extraParameters[self::ROUTE_NAME], $parsedRoutes);
        }

        return $clone;
    }

    public function addGroup(string $prefix, callable $callback): static
    {
        $clone = clone $this;

        $previousGroupPrefix = $clone->currentGroupPrefix;
        $clone->currentGroupPrefix = $previousGroupPrefix . $prefix;
        $callback($clone);
        $clone->currentGroupPrefix = $previousGroupPrefix;

        return $clone;
    }

}

Another update:

This is getting messier that I thought. Since clone only produces a shallow copy of the object instance, the internal properties still reference to the original instances. So you'd have to clone the DataGenerator instance too, otherwise it would write to the one in the original instance of the route collector. However, this property is currently marked as readonly and re-initialising of readonly properties is only possible as of PHP 8.3 (IIRC?).

And it doesn't stop here: the tests are all written in a way that only explicitly modifies the given DataGenerator instance (hello side effects) and don't properly test on the parsed array via ConfigureRoutes::processedRoutes() or DataGenerator::getData() but rather a bogus public property in the test dummy. I think there needs some general clean-up to be done...

[codemasher]

Ok, I've added RouteCollectorImmutable. Nothing concrete yet and might need further clean-up... just for consideration :)

[lcobucci]

To me, the DateTimeImmutable was introduced to correct a mistake in the PHP API, but now we have to live with 2 implementations.

I believe the library must provide a single default way to handle this. Having a mutable and an immutable implementation will make people ask which should be used and why.

Let's keep this patch only for the fluent interface, then we can craft a new to modify things to be immutable if/when needed.

[codemasher]

Personally I'm not a fan of (pseudo-) immutability as it creates mess and unnecessary overhead (in PSR-7 for example, to the point where it becomes unmaintainable). Especially in the case of this class, which explicitly modifies the internal state of another object (similar to PSR-7 StreamInterface), enforcing immutability doesn't make much sense to me.

[Crell]

I also don't see a need for an immutable version here, but I am all in favor of making the methods fluent. Just returning $this is fine.