refactor: simplify handleRequest method in CodeIgniter

Author: gr8man

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);
         }
     }

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);
+    }
 }