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.

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

fix: reverse routing causes ErrorException #7880

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 25 additions & 16 deletions system/Router/RouteCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -1151,11 +1151,13 @@ public function environment(string $env, Closure $callback): RouteCollectionInte
public function reverseRoute(string $search, ...$params)
{
// Named routes get higher priority.
foreach ($this->routesNames as $collection) {
foreach ($this->routesNames as $verb => $collection) {
if (array_key_exists($search, $collection)) {
$routeKey = $collection[$search];

return $this->buildReverseRoute($routeKey, $params);
$from = $this->routes[$verb][$routeKey]['from'];

return $this->buildReverseRoute($from, $params);
}
}

Expand All @@ -1171,8 +1173,9 @@ public function reverseRoute(string $search, ...$params)
// If it's not a named route, then loop over
// all routes to find a match.
foreach ($this->routes as $collection) {
foreach ($collection as $routeKey => $route) {
$to = $route['handler'];
foreach ($collection as $route) {
$to = $route['handler'];
$from = $route['from'];

// ignore closures
if (! is_string($to)) {
Expand All @@ -1196,7 +1199,7 @@ public function reverseRoute(string $search, ...$params)
continue;
}

return $this->buildReverseRoute($routeKey, $params);
return $this->buildReverseRoute($from, $params);
}
}

Expand Down Expand Up @@ -1311,21 +1314,21 @@ protected function fillRouteParams(string $from, ?array $params = null): string
* @param array $params One or more parameters to be passed to the route.
* The last parameter allows you to set the locale.
*/
protected function buildReverseRoute(string $routeKey, array $params): string
protected function buildReverseRoute(string $from, array $params): string
{
$locale = null;

// Find all of our back-references in the original route
preg_match_all('/\(([^)]+)\)/', $routeKey, $matches);
preg_match_all('/\(([^)]+)\)/', $from, $matches);

if (empty($matches[0])) {
if (strpos($routeKey, '{locale}') !== false) {
if (strpos($from, '{locale}') !== false) {
$locale = $params[0] ?? null;
}

$routeKey = $this->replaceLocale($routeKey, $locale);
$from = $this->replaceLocale($from, $locale);

return '/' . ltrim($routeKey, '/');
return '/' . ltrim($from, '/');
}

// Locale is passed?
Expand All @@ -1336,25 +1339,31 @@ protected function buildReverseRoute(string $routeKey, array $params): string

// Build our resulting string, inserting the $params in
// the appropriate places.
foreach ($matches[0] as $index => $pattern) {
foreach ($matches[0] as $index => $placeholder) {
if (! isset($params[$index])) {
throw new InvalidArgumentException(
'Missing argument for "' . $pattern . '" in route "' . $routeKey . '".'
'Missing argument for "' . $placeholder . '" in route "' . $from . '".'
);
}

// Remove `(:` and `)` when $placeholder is a placeholder.
$placeholderName = substr($placeholder, 2, -1);
// or maybe $placeholder is not a placeholder, but a regex.
$pattern = $this->placeholders[$placeholderName] ?? $placeholder;

if (! preg_match('#^' . $pattern . '$#u', $params[$index])) {
throw RouterException::forInvalidParameterType();
}

// Ensure that the param we're inserting matches
// the expected param type.
$pos = strpos($routeKey, $pattern);
$routeKey = substr_replace($routeKey, $params[$index], $pos, strlen($pattern));
$pos = strpos($from, $placeholder);
$from = substr_replace($from, $params[$index], $pos, strlen($placeholder));
}

$routeKey = $this->replaceLocale($routeKey, $locale);
$from = $this->replaceLocale($from, $locale);

return '/' . ltrim($routeKey, '/');
return '/' . ltrim($from, '/');
}

/**
Expand Down
39 changes: 38 additions & 1 deletion tests/system/Helpers/URLHelper/MiscUrlTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -906,13 +906,50 @@ public function testUrlToWithSupportedLocaleInRoute(): void
);
}

public function testUrlToWithNamedRouteWithNestedParentheses(): void
{
Services::createRequest(new App());
$routes = service('routes');

// The route will be:
// docs/(master|\d+\.(?:\d+|x))/([a-z0-9-]+)
$routes->addPlaceholder([
'version' => 'master|\d+\.(?:\d+|x)',
'page' => '[a-z0-9-]+',
]);
$routes->get('docs/(:version)/(:page)', static function () {
echo 'Test the documentation segment';
}, ['as' => 'docs.version']);

$this->assertSame(
'http://example.com/index.php/docs/10.9/install',
url_to('docs.version', '10.9', 'install')
);
}

public function testUrlToWithRouteWithNestedParentheses(): void
{
Services::createRequest(new App());
$routes = service('routes');

// The route will be:
// images/(^.*\.(?:jpg|png)$)
$routes->addPlaceholder('imgFileExt', '^.*\.(?:jpg|png)$');
$routes->get('images/(:imgFileExt)', 'Images::getFile/$1');

$this->assertSame(
'http://example.com/index.php/images/test.jpg',
url_to('Images::getFile', 'test.jpg')
);
}

/**
* @see https://github.com/codeigniter4/CodeIgniter4/issues/7651
*/
public function testUrlToMissingArgument(): void
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('Missing argument for "([a-zA-Z]+)" in route "([a-zA-Z]+)/login".');
$this->expectExceptionMessage('Missing argument for "(:alpha)" in route "(:alpha)/login".');

$routes = Services::routes();
$routes->group('(:alpha)', static function ($routes): void {
Expand Down