From 33637cde2f57b49a8befc16adb7f590022daccfa Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 9 Dec 2024 23:45:00 +0800 Subject: [PATCH] Fix path support after unlimited optional placeholders When using a route which contains both an unlimited optional placeholder, and another optional placeholder afterwards, the incorrect values are collected. For example, given the following route: ``` /go/to/[{location:.*}[/info/{subpage}]] ``` The following behaviour is currently observed: - route: `/go/to/[{location:.*}[/info/{subpage}]]` - url: `/go/to/australia/perth/info/about` - location: `'australia/perth/info/about'` - subpage: `''` Note that the `location` contains `/info/about` and the `subpage` is empty. This is inconsistent with the behaviour where an unlimited value is _not_ used: - route: `/go/to/[{location}[/info/{subpage}]]` - url: `/go/to/australia/info/about` - location: `'australia'` - subpage: `'about'` In the case of the unlimited optional path, the expected behaviour is: The correct value would be: - route: `/go/to/[{location:.*}[/info/{subpage}]]` - url: `/go/to/australia/perth/info/about` - location: `'australia/perth'` - subpage: `'about'` This commit change updates the route dispatcher to reverse the order of the routes when adding routes to the router, which brings the unlimited path placeholder format inline with limited path placeholders. Signed-off-by: Andrew Nicols --- .../FromProcessedConfiguration.php | 3 +- src/RouteCollector.php | 2 +- test/Dispatcher/DispatcherTestCase.php | 12 +++++ .../FromProcessedConfigurationTest.php | 3 +- test/RouteCollectorTest.php | 51 +++++++++++++++++++ 5 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/GenerateUri/FromProcessedConfiguration.php b/src/GenerateUri/FromProcessedConfiguration.php index aae03b1..381987c 100644 --- a/src/GenerateUri/FromProcessedConfiguration.php +++ b/src/GenerateUri/FromProcessedConfiguration.php @@ -8,6 +8,7 @@ use function array_key_exists; use function array_keys; +use function array_reverse; use function assert; use function count; use function is_string; @@ -34,7 +35,7 @@ public function forRoute(string $name, array $substitutions = []): string $missingParameters = []; - foreach ($this->processedConfiguration[$name] as $parsedRoute) { + foreach (array_reverse($this->processedConfiguration[$name]) as $parsedRoute) { $missingParameters = $this->missingParameters($parsedRoute, $substitutions); // Only attempt to generate the path if we have the necessary info diff --git a/src/RouteCollector.php b/src/RouteCollector.php index 9de94b6..3cd9bb9 100644 --- a/src/RouteCollector.php +++ b/src/RouteCollector.php @@ -31,7 +31,7 @@ public function __construct( public function addRoute(string|array $httpMethod, string $route, mixed $handler, array $extraParameters = []): void { $route = $this->currentGroupPrefix . $route; - $parsedRoutes = $this->routeParser->parse($route); + $parsedRoutes = array_reverse($this->routeParser->parse($route)); $extraParameters = [self::ROUTE_REGEX => $route] + $extraParameters; diff --git a/test/Dispatcher/DispatcherTestCase.php b/test/Dispatcher/DispatcherTestCase.php index ba45688..dfbd86b 100644 --- a/test/Dispatcher/DispatcherTestCase.php +++ b/test/Dispatcher/DispatcherTestCase.php @@ -329,6 +329,18 @@ public static function provideFoundDispatchCases(): iterable }; yield 'options method is supported' => ['OPTIONS', '/about', $callback, 'handler0', [], ['_route' => '/about']]; + + $callback = static function (ConfigureRoutes $r): void { + $r->addRoute('GET', '/about[/{aboutwhat}[/location]]', 'handler0'); + }; + + yield 'Paths can be placed after an optional placeholder' => ['GET', '/about/some/location', $callback, 'handler0', ['aboutwhat' => 'some'], ['_route' => '/about[/{aboutwhat}[/location]]']]; + + $callback = static function (ConfigureRoutes $r): void { + $r->addRoute('GET', '/about[/{aboutwhat:.*}[/location]]', 'handler0'); + }; + + yield 'Paths can be placed after an unlimited optional placeholder' => ['GET', '/about/the/nested/location', $callback, 'handler0', ['aboutwhat' => 'the/nested'], ['_route' => '/about[/{aboutwhat:.*}[/location]]']]; } /** @return iterable */ diff --git a/test/GenerateUri/FromProcessedConfigurationTest.php b/test/GenerateUri/FromProcessedConfigurationTest.php index 704b8ab..63fbb0f 100644 --- a/test/GenerateUri/FromProcessedConfigurationTest.php +++ b/test/GenerateUri/FromProcessedConfigurationTest.php @@ -9,7 +9,6 @@ use PHPUnit\Framework\TestCase; use function array_map; -use function array_reverse; /** @phpstan-import-type ParsedRoutes from RouteParser */ final class FromProcessedConfigurationTest extends TestCase @@ -129,7 +128,7 @@ public function unicodeParametersAreAlsoAccepted(): void private static function routeGeneratorFor(array $routeMap): GenerateUri { $parseRoutes = static function (string $route): array { - return array_reverse((new RouteParser\Std())->parse($route)); + return (new RouteParser\Std())->parse($route); }; return new GenerateUri\FromProcessedConfiguration(array_map($parseRoutes, $routeMap)); diff --git a/test/RouteCollectorTest.php b/test/RouteCollectorTest.php index 4dd52ce..5d36022 100644 --- a/test/RouteCollectorTest.php +++ b/test/RouteCollectorTest.php @@ -118,6 +118,26 @@ public function routesCanBeGrouped(): void self::assertSame($expected, $dataGenerator->routes); } + #[PHPUnit\Test] + public function optionalRoutesCanBeUsed(): void + { + $dataGenerator = self::dummyNestedDataGenerator(); + $r = new RouteCollector(new Std(), $dataGenerator); + + $r->any('/any[/{optional}]', 'optional'); + $r->get('/any[/{optional}/hello]', 'optional'); + + $expected = [ + ['*', ['/any/', ['optional', '[^/]+']], 'optional', ['_route' => '/any[/{optional}]']], + ['*', ['/any'], 'optional', ['_route' => '/any[/{optional}]']], + ['GET', ['/any/', ['optional', '[^/]+'], '/hello'], 'optional', ['_route' => '/any[/{optional}/hello]']], + ['GET', ['/any'], 'optional', ['_route' => '/any[/{optional}/hello]']], + ]; + + self::assertObjectHasProperty('routes', $dataGenerator); + self::assertSame($expected, $dataGenerator->routes); + } + #[PHPUnit\Test] public function namedRoutesShouldBeRegistered(): void { @@ -180,4 +200,35 @@ public function addRoute(string $httpMethod, array $routeData, mixed $handler, a } }; } + + private static function dummyNestedDataGenerator(): DataGenerator + { + return new class implements DataGenerator + { + /** @var list, mixed, array}> */ + public array $routes = []; + + /** @inheritDoc */ + public function getData(): array + { + return [[], []]; + } + + /** @inheritDoc */ + public function addRoute(string $httpMethod, array $routeData, mixed $handler, array $extraParameters = []): void + { + $route = [$httpMethod]; + + $subroute = []; + foreach ($routeData as $data) { + $subroute[] = $data; + } + $route[] = $subroute; + + $route[] = $handler; + $route[] = $extraParameters; + $this->routes[] = $route; + } + }; + } }