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
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
180 changes: 98 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,125 @@ protected function checkRoutes(string $uri): bool

$matchedKey = $routeKey;

// Are we dealing with a locale?
if (str_contains($routeKey, '{locale}')) {
$routeKey = str_replace('{locale}', '[^/]+', $routeKey);
}
// Route keys without PCRE metacharacters (including the pattern
// delimiter `#`) can be compared as plain strings, avoiding a
// PCRE call for static routes.
$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
55 changes: 55 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,59 @@ 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());
}
}
Loading