-
Notifications
You must be signed in to change notification settings - Fork 445
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
base: master
Are you sure you want to change the base?
Conversation
(cherry picked from commit 81d5460)
@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 |
Why not both? Similar to PHP's 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 And it doesn't stop here: the tests are all written in a way that only explicitly modifies the given |
29d9dd7
to
3346f11
Compare
Ok, I've added |
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. |
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 |
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:
vs.