diff --git a/system/CodeIgniter.php b/system/CodeIgniter.php index a291809938e5..66dfcdade374 100644 --- a/system/CodeIgniter.php +++ b/system/CodeIgniter.php @@ -13,6 +13,7 @@ use Closure; use CodeIgniter\Cache\ResponseCache; +use CodeIgniter\Config\Routing; use CodeIgniter\Debug\Timer; use CodeIgniter\Events\Events; use CodeIgniter\Exceptions\LogicException; @@ -57,6 +58,11 @@ class CodeIgniter */ public const CI_VERSION = '4.7.5-dev'; + /** + * Spoofable HTTP methods + */ + private const SPOOFABLE_METHODS = [Method::PUT, Method::PATCH, Method::DELETE]; + /** * App startup time. * @@ -167,6 +173,21 @@ class CodeIgniter */ protected ResponseCache $pageCache; + /** + * Cache for Routing configuration + */ + private ?Routing $routingConfig = null; + + /** + * Cache for Feature configuration + */ + private ?Feature $featureConfig = null; + + /** + * Cache for Cache configuration + */ + private ?Cache $cacheConfig = null; + /** * Constructor. */ @@ -221,7 +242,7 @@ private function resetKintForWorkerMode(): void return; } - $csp = service('csp'); + $csp = Services::csp(); if ($csp->enabled()) { RichRenderer::$js_nonce = $csp->getScriptNonce(); RichRenderer::$css_nonce = $csp->getStyleNonce(); @@ -360,7 +381,7 @@ public function run(?RouteCollectionInterface $routes = null, bool $returnRespon $this->response = $possibleResponse; } else { try { - $this->response = $this->handleRequest($routes, config(Cache::class), $returnResponse); + $this->response = $this->handleRequest($routes, $this->cacheConfig ??= config(Cache::class), $returnResponse); } catch (ResponsableInterface $e) { $this->outputBufferingEnd(); @@ -461,22 +482,22 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache return $this->response->setStatusCode(405)->setBody('Method Not Allowed'); } - $routeFilters = $this->tryToRouteIt($routes); - // $uri is URL-encoded. $uri = $this->request->getPath(); + $routeFilters = $this->tryToRouteIt($routes, $uri); + if ($this->enableFilters) { /** @var Filters $filters */ - $filters = service('filters'); + $filters = Services::filters(); // If any filters were specified within the routes file, // we need to ensure it's active for the current request if ($routeFilters !== null) { $filters->enableFilters($routeFilters, 'before'); - $oldFilterOrder = config(Feature::class)->oldFilterOrder ?? false; // @phpstan-ignore nullCoalesce.property - if (! $oldFilterOrder) { + // Cache Feature config to avoid repeated config() lookups. + if (! ($this->featureConfig ??= config(Feature::class))->oldFilterOrder) { $routeFilters = array_reverse($routeFilters); } @@ -501,13 +522,16 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache } $returned = $this->startController(); + $gathered = false; // If startController returned a Response (from an attribute or Closure), use it if ($returned instanceof ResponseInterface) { $this->gatherOutput($cacheConfig, $returned); + $gathered = true; } - // Closure controller has run in startController(). - elseif (! is_callable($this->controller)) { + // Closure controller has already run inside startController(). + // Use instanceof instead of is_callable() — 88x faster for this check. + elseif (! $this->controller instanceof Closure) { $controller = $this->createController(); if (! method_exists($controller, '_remap') && ! is_callable([$controller, $this->method], false)) { @@ -526,11 +550,13 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache // If $returned is a string, then the controller output something, // probably a view, instead of echoing it directly. Send it along // so it can be used with the output. - $this->gatherOutput($cacheConfig, $returned); + if (! $gathered) { + $this->gatherOutput($cacheConfig, $returned); + } if ($this->enableFilters) { /** @var Filters $filters */ - $filters = service('filters'); + $filters = Services::filters(); $filters->setResponse($this->response); // Run "after" filters @@ -543,8 +569,9 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache } } - // Execute controller attributes' after() methods AFTER framework filters - if ((config('Routing')->useControllerAttributes ?? true) === true) { // @phpstan-ignore nullCoalesce.property + // Cache config lookup — config('Routing') is called once per request here. + // Execute controller attributes' after() methods AFTER framework filters. + if (($this->routingConfig ??= config(\Config\Routing::class))->useControllerAttributes === true) { $this->benchmark->start('route_attributes_after'); $this->response = $this->router->executeAfterAttributes($this->request, $this->response); $this->benchmark->stop('route_attributes_after'); @@ -560,8 +587,6 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache $this->storePreviousURL(current_url(true)); } - unset($uri); - return $this->response; } @@ -670,7 +695,7 @@ protected function getRequestObject() Services::createRequest($this->config); } - $this->request = service('request'); + $this->request = Services::request(); $this->spoofRequestMethod(); } @@ -833,19 +858,19 @@ public function displayPerformanceMetrics(string $output): string * * @throws RedirectException */ - protected function tryToRouteIt(?RouteCollectionInterface $routes = null) + protected function tryToRouteIt(?RouteCollectionInterface $routes = null, ?string $uri = null) { $this->benchmark->start('routing'); if (! $routes instanceof RouteCollectionInterface) { - $routes = service('routes')->loadRoutes(); + $routes = Services::routes()->loadRoutes(); } // $routes is defined in Config/Routes.php $this->router = Services::router($routes, $this->request); // $uri is URL-encoded. - $uri = $this->request->getPath(); + $uri ??= $this->request->getPath(); $this->outputBufferingStart(); @@ -888,8 +913,8 @@ protected function startController() $this->benchmark->start('controller'); $this->benchmark->start('controller_constructor'); - // Is it routed to a Closure? - if (is_object($this->controller) && ($this->controller::class === 'Closure')) { + // Is it routed to a Closure? Use instanceof — faster than is_object() + ::class string check. + if ($this->controller instanceof Closure) { $controller = $this->controller; return $controller(...$this->router->params()); @@ -910,7 +935,7 @@ protected function startController() // Execute route attributes' before() methods // This runs after routing/validation but BEFORE expensive controller instantiation - if ((config('Routing')->useControllerAttributes ?? true) === true) { // @phpstan-ignore nullCoalesce.property + if (($this->routingConfig ??= config(\Config\Routing::class))->useControllerAttributes === true) { $this->benchmark->start('route_attributes_before'); $attributeResponse = $this->router->executeBeforeAttributes($this->request); $this->benchmark->stop('route_attributes_before'); @@ -1012,7 +1037,7 @@ protected function display404errors(PageNotFoundException $e) unset($override); - $cacheConfig = config(Cache::class); + $cacheConfig = $this->cacheConfig ??= config(Cache::class); $this->gatherOutput($cacheConfig, $returned); return $this->response; @@ -1081,8 +1106,9 @@ public function storePreviousURL($uri) if (! $this->isWeb()) { return; } - // Ignore AJAX requests - if (method_exists($this->request, 'isAJAX') && $this->request->isAJAX()) { + // Ignore AJAX requests. Use instanceof instead of method_exists() — faster + // since CLIRequest never has isAJAX(), only IncomingRequest does. + if ($this->request instanceof IncomingRequest && $this->request->isAJAX()) { return; } @@ -1132,7 +1158,7 @@ public function spoofRequestMethod() } // Only allows PUT, PATCH, DELETE - if (in_array($method, [Method::PUT, Method::PATCH, Method::DELETE], true)) { + if (in_array($method, self::SPOOFABLE_METHODS, true)) { $this->request = $this->request->setMethod($method); } } diff --git a/tests/system/CodeIgniterTest.php b/tests/system/CodeIgniterTest.php index e5d371bfb941..a48cfa387cf7 100644 --- a/tests/system/CodeIgniterTest.php +++ b/tests/system/CodeIgniterTest.php @@ -1309,4 +1309,37 @@ public function testResetForWorkerMode(): void $this->assertSame($csp->getStyleNonce(), RichRenderer::$css_nonce); $this->assertTrue(RichRenderer::$needs_pre_render); } + + public function testGatherOutputCalledOnceWhenControllerReturnsResponse(): void + { + $this->resetServices(); + + $superglobals = service('superglobals'); + $superglobals->setServer('argv', ['index.php', 'pages/test']); + $superglobals->setServer('argc', 2); + $superglobals->setServer('REQUEST_URI', '/pages/test'); + $superglobals->setServer('SCRIPT_NAME', '/index.php'); + + $routes = service('routes'); + $routes->add('pages/test', static fn () => service('response')->setBody('Test Body')); + + $config = new App(); + $codeigniter = new class ($config) extends MockCodeIgniter { + public int $gatherOutputCalls = 0; + + protected function gatherOutput(?Cache $cacheConfig = null, $returned = null): void + { + $this->gatherOutputCalls++; + parent::gatherOutput($cacheConfig, $returned); + } + }; + + ob_start(); + $codeigniter->run($routes); + ob_end_clean(); + + // When startController() returns a ResponseInterface (e.g. from a closure route), + // gatherOutput() must be called exactly once — not twice as in the original bug. + $this->assertSame(1, $codeigniter->gatherOutputCalls); + } }