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 <[email protected]>

Author: andrewnicols

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

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;
 

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<string, array{string, string, Closure(ConfigureRoutes):void}> */

test/GenerateUri/FromProcessedConfigurationTest.php

@@ -129,7 +129,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));

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,34 @@ public function addRoute(string $httpMethod, array $routeData, mixed $handler, a
             }
         };
     }
+
+    private static function dummyNestedDataGenerator(): DataGenerator
+    {
+        return new class implements DataGenerator
+        {
+            /** @var list<array{string, string, mixed, array<string, bool|float|int|string>}> */
+            public array $routes = [];
+
+            /** @inheritDoc */
+            public function getData(): array
+            {
+                return [[], []];
+            }
+
+            /** @inheritDoc */
+            public function addRoute(string $httpMethod, array $routeData, mixed $handler, array $extraParameters = []): void
+            {
+                // assert(count($routeData) === 1 && is_string($routeData[0]));
+
+                $route = [$httpMethod];
+
+                foreach ($routeData as $data) {
+                    $route[] = $data;
+                }
+                $route[] = $handler;
+                $route[] = $extraParameters;
+                $this->routes[] = $route;
+            }
+        };
+    }
 }