refactor: simplify handleRequest method in CodeIgniter

Author: gr8man

system/CodeIgniter.php

@@ -463,16 +463,66 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
 
         $routeFilters = $this->tryToRouteIt($routes);
 
-        // $uri is URL-encoded.
+        // Run "before" filters
+        $possibleResponse = $this->applyFilters('before', $routeFilters);
+
+        // If a ResponseInterface instance is returned then send it back to the client and stop
+        if ($possibleResponse instanceof ResponseInterface) {
+            $this->outputBufferingEnd();
+
+            return $possibleResponse;
+        }
+
+        $returned = $this->startController();
+
+        $this->serveResponse($cacheConfig, $returned);
+
+        // Run "after" filters
+        $this->applyFilters('after');
+
+        // Execute controller attributes' after() methods AFTER framework filters
+        if ((config('Routing')->useControllerAttributes ?? true) === true) { // @phpstan-ignore nullCoalesce.property
+            $this->benchmark->start('route_attributes_after');
+            $this->response = $this->router->executeAfterAttributes($this->request, $this->response);
+            $this->benchmark->stop('route_attributes_after');
+        }
+
+        // Skip unnecessary processing for special Responses.
+        if (
+            ! $this->response instanceof DownloadResponse
+            && ! $this->response instanceof RedirectResponse
+        ) {
+            // Save our current URI as the previous URI in the session
+            // for safer, more accurate use with `previous_url()` helper function.
+            $this->storePreviousURL(current_url(true));
+        }
+
+        return $this->response;
+    }
+
+    /**
+     * Runs filters.
+     *
+     * @param string                   $position     'before' or 'after'
+     * @param list<string>|string|null $routeFilters
+     */
+    protected function applyFilters(string $position, $routeFilters = null): ?ResponseInterface
+    {
+        if (! $this->enableFilters) {
+            return null;
+        }
+
         $uri = $this->request->getPath();
 
-        if ($this->enableFilters) {
-            /** @var Filters $filters */
-            $filters = service('filters');
+        /** @var Filters $filters */
+        $filters = service('filters');
 
-            // If any filters were specified within the routes file,
-            // we need to ensure it's active for the current request
+        if ($position === 'before') {
             if ($routeFilters !== null) {
+                if (is_string($routeFilters)) {
+                    $routeFilters = [$routeFilters];
+                }
+
                 $filters->enableFilters($routeFilters, 'before');
 
                 $oldFilterOrder = config(Feature::class)->oldFilterOrder ?? false; // @phpstan-ignore nullCoalesce.property
@@ -483,31 +533,51 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
                 $filters->enableFilters($routeFilters, 'after');
             }
 
-            // Run "before" filters
             $this->benchmark->start('before_filters');
             $possibleResponse = $filters->run($uri, 'before');
             $this->benchmark->stop('before_filters');
 
-            // If a ResponseInterface instance is returned then send it back to the client and stop
             if ($possibleResponse instanceof ResponseInterface) {
-                $this->outputBufferingEnd();
-
                 return $possibleResponse;
             }
 
             if ($possibleResponse instanceof IncomingRequest || $possibleResponse instanceof CLIRequest) {
                 $this->request = $possibleResponse;
             }
+
+            return null;
         }
 
-        $returned = $this->startController();
+        // after position
+        $filters->setResponse($this->response);
+
+        $this->benchmark->start('after_filters');
+        $response = $filters->run($uri, 'after');
+        $this->benchmark->stop('after_filters');
 
+        if ($response instanceof ResponseInterface) {
+            $this->response = $response;
+        }
+
+        return null;
+    }
+
+    /**
+     * Start the controller, run it, and gather output.
+     *
+     * @param mixed $returned
+     */
+    protected function serveResponse(Cache $cacheConfig, $returned): void
+    {
         // If startController returned a Response (from an attribute or Closure), use it
         if ($returned instanceof ResponseInterface) {
-            $this->gatherOutput($cacheConfig, $returned);
+            $this->handleCache($cacheConfig, $returned);
+
+            return;
         }
+
         // Closure controller has run in startController().
-        elseif (! is_callable($this->controller)) {
+        if (! is_callable($this->controller)) {
             $controller = $this->createController();
 
             if (! method_exists($controller, '_remap') && ! is_callable([$controller, $this->method], false)) {
@@ -526,43 +596,17 @@ 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 ($this->enableFilters) {
-            /** @var Filters $filters */
-            $filters = service('filters');
-            $filters->setResponse($this->response);
-
-            // Run "after" filters
-            $this->benchmark->start('after_filters');
-            $response = $filters->run($uri, 'after');
-            $this->benchmark->stop('after_filters');
-
-            if ($response instanceof ResponseInterface) {
-                $this->response = $response;
-            }
-        }
-
-        // Execute controller attributes' after() methods AFTER framework filters
-        if ((config('Routing')->useControllerAttributes ?? true) === true) { // @phpstan-ignore nullCoalesce.property
-            $this->benchmark->start('route_attributes_after');
-            $this->response = $this->router->executeAfterAttributes($this->request, $this->response);
-            $this->benchmark->stop('route_attributes_after');
-        }
-
-        // Skip unnecessary processing for special Responses.
-        if (
-            ! $this->response instanceof DownloadResponse
-            && ! $this->response instanceof RedirectResponse
-        ) {
-            // Save our current URI as the previous URI in the session
-            // for safer, more accurate use with `previous_url()` helper function.
-            $this->storePreviousURL(current_url(true));
-        }
-
-        unset($uri);
+        $this->handleCache($cacheConfig, $returned);
+    }
 
-        return $this->response;
+    /**
+     * Gathers the script output and handles caching.
+     *
+     * @param mixed $returned
+     */
+    protected function handleCache(Cache $cacheConfig, $returned): void
+    {
+        $this->gatherOutput($cacheConfig, $returned);
     }
 
     /**

tests/system/CodeIgniterTest.php

@@ -1309,4 +1309,35 @@ public function testResetForWorkerMode(): void
         $this->assertSame($csp->getStyleNonce(), RichRenderer::$css_nonce);
         $this->assertTrue(RichRenderer::$needs_pre_render);
     }
+
+    public function testServeResponseCallsHandleCacheOnceForResponse(): 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 $handleCacheCalls = 0;
+
+            protected function handleCache(Cache $cacheConfig, $returned): void
+            {
+                $this->handleCacheCalls++;
+                parent::handleCache($cacheConfig, $returned);
+            }
+        };
+
+        ob_start();
+        $codeigniter->run($routes);
+        ob_end_clean();
+
+        $this->assertSame(1, $codeigniter->handleCacheCalls);
+    }
 }