Skip to content

perf: add fast-path for static routes in Router::checkRoutes() #10705

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.

Sign up for GitHub

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

Open
gr8man wants to merge 1 commit into
base: develop
Choose a base branch
from
+197 −82
Open

perf: add fast-path for static routes in Router::checkRoutes() #10705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
178 changes: 96 additions & 82 deletions system/Router/Router.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}', '(?<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}', '(?<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;
Expand Down
101 changes: 101 additions & 0 deletions tests/system/Router/RouterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Loading