Skip to content

refactor: simplify and optimize handleRequest method in CodeIgniter #10369

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
+66 −25
Open

refactor: simplify and optimize handleRequest method in CodeIgniter #10369

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
58 changes: 33 additions & 25 deletions system/CodeIgniter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
use Config\Cache;
use Config\Feature;
use Config\Kint as KintConfig;
use Config\Routing;
use Config\Services;
use Exception;
use Kint;
Expand All @@ -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.
*
Expand Down Expand Up @@ -221,7 +227,7 @@ private function resetKintForWorkerMode(): void
return;
}

$csp = service('csp');
$csp = Services::csp();
Comment on lines -224 to +230

@michalsn michalsn Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert everywhere. We prefer the use of service() as it's optimized.

if ($csp->enabled()) {
RichRenderer::$js_nonce = $csp->getScriptNonce();
RichRenderer::$css_nonce = $csp->getStyleNonce();
Expand Down Expand Up @@ -461,22 +467,21 @@ 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) {
if (! config(Feature::class)->oldFilterOrder) {
$routeFilters = array_reverse($routeFilters);
}

Expand All @@ -501,13 +506,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;
}
Comment thread
michalsn marked this conversation as resolved.
// 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)) {
Expand All @@ -526,11 +534,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
Expand All @@ -543,8 +553,8 @@ 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
// Execute controller attributes' after() methods AFTER framework filters.
if (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');
Expand All @@ -560,8 +570,6 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
$this->storePreviousURL(current_url(true));
}

unset($uri);

return $this->response;
}

Expand Down Expand Up @@ -670,7 +678,7 @@ protected function getRequestObject()
Services::createRequest($this->config);
}

$this->request = service('request');
$this->request = Services::request();

$this->spoofRequestMethod();
}
Expand Down Expand Up @@ -833,19 +841,19 @@ public function displayPerformanceMetrics(string $output): string
*
* @throws RedirectException
*/
protected function tryToRouteIt(?RouteCollectionInterface $routes = null)
protected function tryToRouteIt(?RouteCollectionInterface $routes = null, ?string $uri = null)
Comment on lines -836 to +844

@michalsn michalsn Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a BC break, without real gain. This is just one getPath() call. Please revert.

{
$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();

Expand Down Expand Up @@ -888,8 +896,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) {
Comment on lines -891 to +900

@michalsn michalsn Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's likely true, but please remove the added part of the comment. I see no reason why we should have this in the code.

$controller = $this->controller;

return $controller(...$this->router->params());
Expand All @@ -909,8 +917,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 (config(Routing::class)->useControllerAttributes === true) {
$this->benchmark->start('route_attributes_before');
$attributeResponse = $this->router->executeBeforeAttributes($this->request);
$this->benchmark->stop('route_attributes_before');
Expand Down Expand Up @@ -1081,8 +1088,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()) {

@michalsn michalsn Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This narrows the previous behavior from "any request with isAJAX()" to only IncomingRequest.

I think I'm okay with this, but I would like to hear what others think.

If this stays, please remove the added comment, as it's not relevant to include.

return;
}

Expand Down Expand Up @@ -1132,7 +1140,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)) {
Comment on lines -1135 to +1143

@michalsn michalsn Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this gives us any benefits here. This constant will be used only once.

$this->request = $this->request->setMethod($method);
}
}
Expand Down
33 changes: 33 additions & 0 deletions tests/system/CodeIgniterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Loading