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

Make ConfigureRoutes fluent #288

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

codemasher
Copy link
Contributor

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')
    ;

    // ...
});

(cherry picked from commit 81d5460)
@lcobucci
Copy link
Collaborator

lcobucci commented Apr 22, 2024

@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
Copy link
Contributor Author

codemasher commented Apr 22, 2024

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
Copy link
Contributor Author

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

@lcobucci
Copy link
Collaborator

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
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

2 participants