From ab63068d3331c69cbe56a566474e3ca3eabaab9a Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 30 Aug 2023 11:12:21 +0900 Subject: [PATCH 1/4] test: add tests for reverse routing --- .../system/Helpers/URLHelper/MiscUrlTest.php | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/system/Helpers/URLHelper/MiscUrlTest.php b/tests/system/Helpers/URLHelper/MiscUrlTest.php index ecb59e1ef2b5..85466b151a2e 100644 --- a/tests/system/Helpers/URLHelper/MiscUrlTest.php +++ b/tests/system/Helpers/URLHelper/MiscUrlTest.php @@ -906,6 +906,43 @@ 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 */ From ec489c3a2c5280c30c34059787aaf94a3bd0fd1c Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 30 Aug 2023 11:13:18 +0900 Subject: [PATCH 2/4] fix: reverse routing causes ErrorException ErrorException : preg_match(): Compilation failed: missing closing parenthesis --- system/Router/RouteCollection.php | 42 ++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/system/Router/RouteCollection.php b/system/Router/RouteCollection.php index 36b8e4d52340..54f7e1d15c7b 100644 --- a/system/Router/RouteCollection.php +++ b/system/Router/RouteCollection.php @@ -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); } } @@ -1172,7 +1174,8 @@ public function reverseRoute(string $search, ...$params) // all routes to find a match. foreach ($this->routes as $collection) { foreach ($collection as $routeKey => $route) { - $to = $route['handler']; + $to = $route['handler']; + $from = $route['from']; // ignore closures if (! is_string($to)) { @@ -1196,7 +1199,7 @@ public function reverseRoute(string $search, ...$params) continue; } - return $this->buildReverseRoute($routeKey, $params); + return $this->buildReverseRoute($from, $params); } } @@ -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? @@ -1336,25 +1339,34 @@ 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 . '".' ); } + + $placeholderName = substr($placeholder, 2, -1); + if (isset($this->placeholders[$placeholderName])) { + $pattern = $this->placeholders[$placeholderName]; + } else { + // The $placeholder is not a placeholder. It is a regex. + $pattern = $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, '/'); } /** From a9e44f03f5403134a10abf05be0b63f59a95c56a Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 30 Aug 2023 11:28:15 +0900 Subject: [PATCH 3/4] refactor: remove uneeded variable and if statement --- system/Router/RouteCollection.php | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/system/Router/RouteCollection.php b/system/Router/RouteCollection.php index 54f7e1d15c7b..8383280114b5 100644 --- a/system/Router/RouteCollection.php +++ b/system/Router/RouteCollection.php @@ -1173,7 +1173,7 @@ 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) { + foreach ($collection as $route) { $to = $route['handler']; $from = $route['from']; @@ -1346,13 +1346,10 @@ protected function buildReverseRoute(string $from, array $params): string ); } + // Remove `(:` and `)` when $placeholder is a placeholder. $placeholderName = substr($placeholder, 2, -1); - if (isset($this->placeholders[$placeholderName])) { - $pattern = $this->placeholders[$placeholderName]; - } else { - // The $placeholder is not a placeholder. It is a regex. - $pattern = $placeholder; - } + // 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(); From 50cb4b4573c4d89dcbadd35bae1ae7a381c6c954 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 30 Aug 2023 11:31:27 +0900 Subject: [PATCH 4/4] test: update assertion --- tests/system/Helpers/URLHelper/MiscUrlTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/system/Helpers/URLHelper/MiscUrlTest.php b/tests/system/Helpers/URLHelper/MiscUrlTest.php index 85466b151a2e..4767dbf0cbfd 100644 --- a/tests/system/Helpers/URLHelper/MiscUrlTest.php +++ b/tests/system/Helpers/URLHelper/MiscUrlTest.php @@ -949,7 +949,7 @@ public function testUrlToWithRouteWithNestedParentheses(): void 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 {