From 893f1e18e167791844b63a482e8ac5f49bf3eae0 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sun, 1 Nov 2026 22:42:08 +0100 Subject: [PATCH] perf: add fast-path for static routes in Router::checkRoutes() Avoid executing PCRE for static routes by checking for PCRE metacharacters (including pattern delimiter '#') using strpbrk(). If none are present, perform a direct string comparison against the request URI. This reduces matching overhead from O(N) PCRE invocations to O(1) string checks for static routes while strictly preserving registration precedence and existing handler, Closure, redirect, and locale behavior. --- system/Router/Router.php | 178 ++++++++++++++++------------- tests/system/Router/RouterTest.php | 101 ++++++++++++++++ 2 files changed, 197 insertions(+), 82 deletions(-) diff --git a/system/Router/Router.php b/system/Router/Router.php index f0f9555ad1ac..0253a1f55930 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -450,109 +450,123 @@ protected function checkRoutes(string $uri): bool $matchedKey = $routeKey; - // Are we dealing with a locale? - if (str_contains($routeKey, '{locale}')) { - $routeKey = str_replace('{locale}', '[^/]+', $routeKey); - } + // `#` is the pattern delimiter used below, so it must be treated as a metacharacter. + $isRegex = strpbrk($routeKey, '()[]?*+{}^$\\.|#') !== false; - // Does the RegEx match? - if (preg_match('#^' . $routeKey . '$#u', $uri, $matches)) { - // Is this route supposed to redirect to another? - if ($this->collection->isRedirect($routeKey)) { - // replacing matched route groups with references: post/([0-9]+) -> post/$1 - $redirectTo = preg_replace_callback('/(\([^\(]+\))/', static function (): string { - static $i = 1; - - return '$' . $i++; - }, is_array($handler) ? key($handler) : $handler); - - throw new RedirectException( - preg_replace('#\A' . $routeKey . '\z#u', $redirectTo, $uri), - $this->collection->getRedirectCode($routeKey), - ); + if (! $isRegex) { + if ($routeKey !== $uri) { + continue; } - // Store our locale so CodeIgniter object can - // assign it to the Request. - if (str_contains($matchedKey, '{locale}')) { - preg_match( - '#^' . str_replace('{locale}', '(?[^/]+)', $matchedKey) . '$#u', - $uri, - $matched, - ); - - if ($this->collection->shouldUseSupportedLocalesOnly() - && ! in_array($matched['locale'], config(App::class)->supportedLocales, true)) { - // Throw exception to prevent the autorouter, if enabled, - // from trying to find a route - throw PageNotFoundException::forLocaleNotSupported($matched['locale']); - } - $this->detectedLocale = $matched['locale']; - unset($matched); + $matches = [$uri]; + } else { + // Are we dealing with a locale? + if (str_contains($routeKey, '{locale}')) { + $routeKey = str_replace('{locale}', '[^/]+', $routeKey); } - // Are we using Closures? If so, then we need - // to collect the params into an array - // so it can be passed to the controller method later. - if (! is_string($handler) && is_callable($handler)) { - $this->controller = $handler; + // Does the RegEx match? + if (! preg_match('#^' . $routeKey . '$#u', $uri, $matches)) { + continue; + } + } - // Remove the original string from the matches array - array_shift($matches); + // Is this route supposed to redirect to another? + if ($this->collection->isRedirect($routeKey)) { + // replacing matched route groups with references: post/([0-9]+) -> post/$1 + $redirectTo = preg_replace_callback('/(\([^\(]+\))/', static function (): string { + static $i = 1; - $this->params = $matches; + return '$' . $i++; + }, is_array($handler) ? key($handler) : $handler); - $this->setMatchedRoute($matchedKey, $handler); + throw new RedirectException( + preg_replace('#\A' . $routeKey . '\z#u', $redirectTo, $uri), + $this->collection->getRedirectCode($routeKey), + ); + } - return true; - } + // Store our locale so CodeIgniter object can + // assign it to the Request. + if (str_contains($matchedKey, '{locale}')) { + preg_match( + '#^' . str_replace('{locale}', '(?[^/]+)', $matchedKey) . '$#u', + $uri, + $matched, + ); - if (str_contains($handler, '::')) { - [$controller, $methodAndParams] = explode('::', $handler); - } else { - $controller = $handler; - $methodAndParams = ''; + if ($this->collection->shouldUseSupportedLocalesOnly() + && ! in_array($matched['locale'], config(App::class)->supportedLocales, true)) { + // Throw exception to prevent the autorouter, if enabled, + // from trying to find a route + throw PageNotFoundException::forLocaleNotSupported($matched['locale']); } - // Checks `/` in controller name - if (str_contains($controller, '/')) { - throw RouterException::forInvalidControllerName($handler); + $this->detectedLocale = $matched['locale']; + unset($matched); + } + + // Are we using Closures? If so, then we need + // to collect the params into an array + // so it can be passed to the controller method later. + if (! is_string($handler) && is_callable($handler)) { + $this->controller = $handler; + + // Remove the original string from the matches array + array_shift($matches); + + $this->params = $matches; + + $this->setMatchedRoute($matchedKey, $handler); + + return true; + } + + if (str_contains($handler, '::')) { + [$controller, $methodAndParams] = explode('::', $handler); + } else { + $controller = $handler; + $methodAndParams = ''; + } + + // Checks `/` in controller name + if (str_contains($controller, '/')) { + throw RouterException::forInvalidControllerName($handler); + } + + if (str_contains($handler, '$') && str_contains($routeKey, '(')) { + // Checks dynamic controller + if (str_contains($controller, '$')) { + throw RouterException::forDynamicController($handler); } - if (str_contains($handler, '$') && str_contains($routeKey, '(')) { - // Checks dynamic controller - if (str_contains($controller, '$')) { - throw RouterException::forDynamicController($handler); + if (config(Routing::class)->multipleSegmentsOneParam === false) { + // Using back-references + $segments = explode('/', preg_replace('#\A' . $routeKey . '\z#u', $handler, $uri)); + } else { + if (str_contains($methodAndParams, '/')) { + [$method, $handlerParams] = explode('/', $methodAndParams, 2); + $params = explode('/', $handlerParams); + $handlerSegments = array_merge([$controller . '::' . $method], $params); + } else { + $handlerSegments = [$handler]; } - if (config(Routing::class)->multipleSegmentsOneParam === false) { - // Using back-references - $segments = explode('/', preg_replace('#\A' . $routeKey . '\z#u', $handler, $uri)); - } else { - if (str_contains($methodAndParams, '/')) { - [$method, $handlerParams] = explode('/', $methodAndParams, 2); - $params = explode('/', $handlerParams); - $handlerSegments = array_merge([$controller . '::' . $method], $params); - } else { - $handlerSegments = [$handler]; - } - - $segments = []; - - foreach ($handlerSegments as $segment) { - $segments[] = $this->replaceBackReferences($segment, $matches); - } + $segments = []; + + foreach ($handlerSegments as $segment) { + $segments[] = $this->replaceBackReferences($segment, $matches); } - } else { - $segments = explode('/', $handler); } + } else { + $segments = explode('/', $handler); + } - $this->setRequest($segments); + $this->setRequest($segments); - $this->setMatchedRoute($matchedKey, $handler); + $this->setMatchedRoute($matchedKey, $handler); - return true; - } + return true; } return false; diff --git a/tests/system/Router/RouterTest.php b/tests/system/Router/RouterTest.php index 6e22c95ed4f9..9908dfc6e041 100644 --- a/tests/system/Router/RouterTest.php +++ b/tests/system/Router/RouterTest.php @@ -1031,4 +1031,105 @@ public function testLogsRouteAttributeInstantiationFailureWithContext(): void $this->assertLogContains('error', 'Failed to instantiate route attribute "' . Filter::class . '" on "\Tests\Support\Router\Controllers\InvalidAttributeController::invalidMultipleFilters()":'); $this->assertLogContains('error', 'must be of type string'); } + + public function testStaticRoutesMatchControllerClosureAndNumericKey(): void + { + $collection = clone $this->collection; + $collection->resetRoutes(); + $collection->get('about-us', 'Home::about'); + $collection->get('contact_page', static fn (): string => 'contact-page-content'); + $collection->get('123', 'Home::numericRoute'); + + $router = new Router($collection, $this->request); + + $router->handle('about-us'); + $this->assertSame('\Home', $router->controllerName()); + $this->assertSame('about', $router->methodName()); + $this->assertSame([], $router->params()); + + $router->handle('contact_page'); + $controller = $router->controllerName(); + $this->assertInstanceOf(Closure::class, $controller); + $this->assertSame('contact-page-content', $controller()); + $this->assertSame([], $router->params()); + + $router->handle('123'); + $this->assertSame('\Home', $router->controllerName()); + $this->assertSame('numericRoute', $router->methodName()); + $this->assertSame([], $router->params()); + } + + public function testStaticRouteDoesNotMatchAdditionalSegments(): void + { + $collection = clone $this->collection; + $collection->resetRoutes(); + $collection->get('about-us', 'Home::about'); + + $router = new Router($collection, $this->request); + + $this->expectException(PageNotFoundException::class); + + $router->handle('about-us/team'); + } + + public function testEarlierDynamicRouteTakesPrecedenceOverLaterStaticRoute(): void + { + $collection = clone $this->collection; + $collection->resetRoutes(); + $collection->get('items/(:segment)', 'Items::detail/$1'); + $collection->get('items/new', 'Items::new'); + + $router = new Router($collection, $this->request); + $router->handle('items/new'); + + $this->assertSame('\Items', $router->controllerName()); + $this->assertSame('detail', $router->methodName()); + $this->assertSame(['new'], $router->params()); + } + + public function testRouteWithUnescapedHashIsNotMatchedAsStaticRoute(): void + { + config(App::class)->permittedURIChars = 'a-z 0-9~%.:_\-#'; + + $collection = clone $this->collection; + $collection->resetRoutes(); + $collection->get('docs#anchor', 'Docs::anchor'); + + $router = new Router($collection, $this->request); + + // `#` is the PCRE delimiter, so the key must go through the regex branch + // instead of a plain string comparison. The unescaped delimiter makes + // the pattern invalid, so the route can't match. + $warnings = []; + set_error_handler(static function (int $errno, string $errstr) use (&$warnings): bool { + $warnings[] = $errstr; + + return true; + }, E_WARNING); + + try { + $router->handle('docs#anchor'); + $this->fail('PageNotFoundException was expected.'); + } catch (PageNotFoundException) { + $this->assertCount(1, $warnings); + $this->assertStringContainsString('preg_match()', $warnings[0]); + } finally { + restore_error_handler(); + } + } + + public function testRouteWithEscapedHashMatchesViaRegexBranch(): void + { + config(App::class)->permittedURIChars = 'a-z 0-9~%.:_\-#'; + + $collection = clone $this->collection; + $collection->resetRoutes(); + $collection->get('docs\#anchor', 'Docs::anchor'); + + $router = new Router($collection, $this->request); + $router->handle('docs#anchor'); + + $this->assertSame('\Docs', $router->controllerName()); + $this->assertSame('anchor', $router->methodName()); + } }