-
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
Allow for custom ConfigureRoutes implementations #278
base: master
Are you sure you want to change the base?
Conversation
dcc321d
to
177ac35
Compare
Also in FastRoute: - rename $configuredRoutes and $routeConfiguration to $configureRoutes for consistency - add configureRoutes() method to allow testing for the custom implementation - rename buildConfiguration() to processedConfiguration(), and make public so utilities (such as a URL dumper) can examine the routes - make RouteCollector non-final so it can be extended by consumers, e.g. to auto-add route names
177ac35
to
03cb8ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I was planning to work on this in the upcoming week 👍
Just please don't open the API more than strictly needed to have this change.
@@ -12,7 +12,6 @@ | |||
* @phpstan-import-type ExtraParameters from DataGenerator | |||
* @phpstan-import-type RoutesForUriGeneration from GenerateUri | |||
* @phpstan-import-type ParsedRoutes from RouteParser | |||
* @final |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is has been added on purpose and we'll make this class final on v3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is has been added on purpose
If I may ask, for what purpose was it added? Making it final means something like CastRouteCollector cannot capture or modify information along the way, without (essentially) copying the entire RouteCollector class to replace it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to move the focus of people extending components away from the implementation details and give me flexibility on evolving the lib.
We can introduce ways to reduce boilerplate in the downstream if that's helpful, I just prefer to favour composition over inheritance for that component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After having tried my hand at it, composition-over-inheritance is difficult to implement with RouteCollector. After creating a decorator around it and modifying addRoute() to automatically add a route name, I realized that addGroup() calls $this internally, which means FastRoute\RouteCollector will use its own instance methods, and not the decorator instance methods. For example:
use FastRoute\RouteCollector as RouteCollector;
use FastRoute\ConfigureRoutes;
class DecoRouteCollector implements ConfigureRoutes
public function __construct(
protected RouteCollector $fastRouteCollector,
) {
}
/**
* @inheritdoc
*/
public function addRoute(string|array $httpMethod, string $route, mixed $handler, array $extraParameters = []): void
{
if (is_string($handler)) {
$extraParameters[self::ROUTE_NAME] ??= $handler;
}
$this->fastRouteCollector->addRoute($httpMethod, $route, $handler, $extraParameters);
}
public function addGroup(string $prefix, callable $callback): void
{
// uses the FastRoute collector addRoute() under the hood,
// not the decorator addRoute() method.
$this->fastRouteCollector->addGroup($prefix, $callback);
}
}
I think the easiest solution here still is to make RouteCollector non-final and allow extension.
|
||
use FastRoute\RouteCollector; | ||
|
||
class FakeRouteCollector extends RouteCollector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, as we just need to implement the interface.
$uriGenerator, | ||
$this->cacheDriver, | ||
$this->cacheKey, | ||
); | ||
} | ||
|
||
/** @return ProcessedData */ | ||
private function buildConfiguration(): array | ||
public function processedConfiguration(): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meant to be encapsulated, please don't make it public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meant to be encapsulated
/me nods along
Is there a suggestion for how to inspect the processed routes, so that someone might (for example) dump a list of what paths map to what handlers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are open issues about dumping that info and I was planning to introduce something that consolidates it in a standard structure for debugging.
/** | ||
* @param class-string<ConfigureRoutes> $configureRoutes | ||
*/ | ||
public function useCustomConfigureRoutes(string $configureRoutes): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having an instance that implements should also be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That instance would be the FakeRouteCollector, referenced above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was multi-tasking and communicated poorly.
I meant to say having the ability to receive an instance. Your point on #280 makes a lot of sense, so we could postpone changes and enhance the design using closures in a separate pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could postpone changes and enhance the design using closures in a separate pr
I have posted an alternative solution that incorporates something like "real" DI instead at #282 -- let me know if you think that path (or some variation of it) is viable.
Don't worry about that, force pushing is part of my flow and I really appreciate having a clear set of commits. |
This PR adds a
useCustomConfigureRoutes()
method to FastRoute to allow for custom ConfigureRoutes implementations.Also in FastRoute:
(Note that I modified and force-pushed this PR after initial submission; my apologies for the hassle.)