Skip to content

Commit

Permalink
Fix path support after unlimited optional placeholders
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
andrewnicols committed Dec 9, 2024
1 parent d3ada01 commit 37f2f19
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 3 deletions.
2 changes: 1 addition & 1 deletion src/GenerateUri/FromProcessedConfiguration.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public function forRoute(string $name, array $substitutions = []): string

$missingParameters = [];

foreach ($this->processedConfiguration[$name] as $parsedRoute) {
foreach (array_reverse($this->processedConfiguration[$name]) as $parsedRoute) {

Check failure on line 37 in src/GenerateUri/FromProcessedConfiguration.php

View workflow job for this annotation

GitHub Actions / Check Coding Standards (locked, 8.1, ubuntu-latest)

Function array_reverse() should not be referenced via a fallback global name, but via a use statement.
$missingParameters = $this->missingParameters($parsedRoute, $substitutions);

// Only attempt to generate the path if we have the necessary info
Expand Down
2 changes: 1 addition & 1 deletion src/RouteCollector.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
12 changes: 12 additions & 0 deletions test/Dispatcher/DispatcherTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -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}> */
Expand Down
2 changes: 1 addition & 1 deletion test/GenerateUri/FromProcessedConfigurationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
50 changes: 50 additions & 0 deletions test/RouteCollectorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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) {

Check failure on line 224 in test/RouteCollectorTest.php

View workflow job for this annotation

GitHub Actions / Check Coding Standards (locked, 8.1, ubuntu-latest)

Expected 1 line after "foreach", found 0.
$route[] = $data;
}
$route[] = $handler;
$route[] = $extraParameters;
$this->routes[] = $route;

Check failure on line 229 in test/RouteCollectorTest.php

View workflow job for this annotation

GitHub Actions / Static Analysis (locked, 8.1, ubuntu-latest)

Property class@anonymous/test/RouteCollectorTest.php:206::$routes (array<int, array{string, string, mixed, array<string, bool|float|int|string>}>) does not accept non-empty-array<int, non-empty-array<int<0, max>, mixed>>.
}
};
}
}

0 comments on commit 37f2f19

Please sign in to comment.