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.

Sign up for GitHub

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

Jump to bottom

Allow for custom ConfigureRoutes implementations #278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow for custom ConfigureRoutes implementations #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 37 additions & 17 deletions src/FastRoute.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ final class FastRoute
* @param class-string<RouteParser> $routeParser
* @param class-string<DataGenerator> $dataGenerator
* @param class-string<Dispatcher> $dispatcher
* @param class-string<ConfigureRoutes> $routesConfiguration
* @param class-string<ConfigureRoutes> $configureRoutes
* @param class-string<GenerateUri> $uriGenerator
* @param Cache|class-string<Cache>|null $cacheDriver
* @param non-empty-string|null $cacheKey
Expand All @@ -30,7 +30,7 @@ private function __construct(
private readonly string $routeParser,
private readonly string $dataGenerator,
private readonly string $dispatcher,
private readonly string $routesConfiguration,
private readonly string $configureRoutes,
private readonly string $uriGenerator,
private readonly Cache|string|null $cacheDriver,
private readonly ?string $cacheKey,
Expand Down Expand Up @@ -62,7 +62,7 @@ public function disableCache(): self
$this->routeParser,
$this->dataGenerator,
$this->dispatcher,
$this->routesConfiguration,
$this->configureRoutes,
$this->uriGenerator,
null,
null,
Expand All @@ -80,7 +80,7 @@ public function withCache(Cache|string $driver, string $cacheKey): self
$this->routeParser,
$this->dataGenerator,
$this->dispatcher,
$this->routesConfiguration,
$this->configureRoutes,
$this->uriGenerator,
$driver,
$cacheKey,
Expand Down Expand Up @@ -118,7 +118,24 @@ public function useCustomDispatcher(string $dataGenerator, string $dispatcher):
$this->routeParser,
$dataGenerator,
$dispatcher,
$this->routesConfiguration,
$this->configureRoutes,
$this->uriGenerator,
$this->cacheDriver,
$this->cacheKey,
);
}

/**
* @param class-string<ConfigureRoutes> $configureRoutes
*/
public function useCustomConfigureRoutes(string $configureRoutes): self
Copy link
Collaborator

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

Copy link
Author

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.

Copy link
Collaborator

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.

Copy link
Author

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.

{
return new self(
$this->routeDefinitionCallback,
$this->routeParser,
$this->dataGenerator,
$this->dispatcher,
$configureRoutes,
$this->uriGenerator,
$this->cacheDriver,
$this->cacheKey,
Expand All @@ -133,29 +150,24 @@ public function withUriGenerator(string $uriGenerator): self
$this->routeParser,
$this->dataGenerator,
$this->dispatcher,
$this->routesConfiguration,
$this->configureRoutes,
$uriGenerator,
$this->cacheDriver,
$this->cacheKey,
);
}

/** @return ProcessedData */
private function buildConfiguration(): array
public function processedConfiguration(): array
Copy link
Collaborator

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

Copy link
Author

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?

Copy link
Collaborator

@lcobucci lcobucci Mar 10, 2024 •
edited
Loading

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.

{
if ($this->processedConfiguration !== null) {
return $this->processedConfiguration;
}

$loader = function (): array {
$configuredRoutes = new $this->routesConfiguration(
new $this->routeParser(),
new $this->dataGenerator(),
);

($this->routeDefinitionCallback)($configuredRoutes);

return $configuredRoutes->processedRoutes();
$configureRoutes = $this->configureRoutes();
($this->routeDefinitionCallback)($configureRoutes);
return $configureRoutes->processedRoutes();
};

if ($this->cacheDriver === null) {
Expand All @@ -171,13 +183,21 @@ private function buildConfiguration(): array
return $this->processedConfiguration = $cache->get($this->cacheKey, $loader);
}

public function configureRoutes(): ConfigureRoutes
{
return new $this->configureRoutes(
new $this->routeParser(),
new $this->dataGenerator(),
);
}

public function dispatcher(): Dispatcher
{
return new $this->dispatcher($this->buildConfiguration());
return new $this->dispatcher($this->processedConfiguration());
}

public function uriGenerator(): GenerateUri
{
return new $this->uriGenerator($this->buildConfiguration()[2]);
return new $this->uriGenerator($this->processedConfiguration()[2]);
}
}
1 change: 0 additions & 1 deletion src/RouteCollector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
* @phpstan-import-type ExtraParameters from DataGenerator
* @phpstan-import-type RoutesForUriGeneration from GenerateUri
* @phpstan-import-type ParsedRoutes from RouteParser
* @final
Copy link
Collaborator

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

Copy link
Author

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.

Copy link
Collaborator

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.

Copy link
Author

@pmjones pmjones Apr 6, 2024 •
edited
Loading

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.

*/
class RouteCollector implements ConfigureRoutes
{
Expand Down
10 changes: 10 additions & 0 deletions test/FakeRouteCollector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php
declare(strict_types=1);

namespace FastRoute\Test;

use FastRoute\RouteCollector;

class FakeRouteCollector extends RouteCollector
Copy link
Collaborator

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.

{
}
11 changes: 11 additions & 0 deletions test/FastRouteTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use FastRoute\Dispatcher;
use FastRoute\FastRoute;
use FastRoute\GenerateUri;
use FastRoute\RouteCollector;
use PHPUnit\Framework\Attributes as PHPUnit;
use PHPUnit\Framework\TestCase;
use RuntimeException;
Expand Down Expand Up @@ -110,6 +111,16 @@ public function defaultUriGeneratorMustBeProvided(): void
self::assertInstanceOf(GenerateUri\FromProcessedConfiguration::class, $uriGenerator);
}

#[PHPUnit\Test]
public function configureRoutesCanBeOverridden(): void
{
$configureRoutes = FastRoute::recommendedSettings(self::routes(...), 'test')
->useCustomConfigureRoutes(FakeRouteCollector::class)
->configureRoutes();

self::assertInstanceOf(FakeRouteCollector::class, $configureRoutes);
}

#[PHPUnit\Test]
public function uriGeneratorCanBeOverridden(): void
{
Expand Down