Skip to content

fix: CodeIgniter::run() doesn't respect $returnResponse #6737

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

Merged
merged 7 commits into from
Oct 26, 2022
Merged

fix: CodeIgniter::run() doesn't respect $returnResponse #6737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a988128
test: replace Services::request() with Services::incomingrequest()
kenjis Oct 22, 2022
6d40429
docs: fix typos
kenjis Oct 22, 2022
ec97332
fix: CodeIgniter::run() doesn't respect $returnResponse parameter
kenjis Oct 22, 2022
ac66762
test: add tests
kenjis Oct 22, 2022
585dbd0
docs: add changelog
kenjis Oct 22, 2022
59c076f
refactor: reverse if condition
kenjis Oct 23, 2022
b795d0f
fix: behavior so that it does not change
kenjis Oct 24, 2022
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
37 changes: 34 additions & 3 deletions system/CodeIgniter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ class CodeIgniter
*/
protected ?string $context = null;

/**
* Whether to return Response object or send response.
*/
protected bool $returnResponse = false;

/**
* Constructor.
*/
Expand Down Expand Up @@ -291,6 +296,8 @@ protected function initializeKint()
*/
public function run(?RouteCollectionInterface $routes = null, bool $returnResponse = false)
{
$this->returnResponse = $returnResponse;

if ($this->context === null) {
throw new LogicException('Context must be set before run() is called. If you are upgrading from 4.1.x, you need to merge `public/index.php` and `spark` file from `vendor/codeigniter4/framework`.');
}
Expand All @@ -309,6 +316,10 @@ public function run(?RouteCollectionInterface $routes = null, bool $returnRespon
if ($this->request instanceof IncomingRequest && strtolower($this->request->getMethod()) === 'cli') {
$this->response->setStatusCode(405)->setBody('Method Not Allowed');

if ($this->returnResponse) {
return $this->response;
}

$this->sendResponse();

return;
Expand Down Expand Up @@ -345,13 +356,22 @@ public function run(?RouteCollectionInterface $routes = null, bool $returnRespon
// If the route is a 'redirect' route, it throws
// the exception with the $to as the message
$this->response->redirect(base_url($e->getMessage()), 'auto', $e->getCode());

if ($this->returnResponse) {
return $this->response;
}

$this->sendResponse();

$this->callExit(EXIT_SUCCESS);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not related to this PR, but is this callExit() necessary?

Copy link
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 - maybe just to enforce a "never return" condition?

Copy link
Member

@paulbalandan paulbalandan Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If you will add @phpstan-return never to protected function callExit, then phpstan correctly flags that the return; below will not be reached. So, this must be a bug?

Copy link
Member

Choose a reason for hiding this comment

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

I think the return is there because MockCodeIgniter prevents the actual exit statement. Still, this is a bad set up, and we should probably get rid of the exit altogether and just have the whole stack return cleanly.


return;
} catch (PageNotFoundException $e) {
$this->display404errors($e);
$return = $this->display404errors($e);

if ($return instanceof ResponseInterface) {
return $return;
}
}
}

Expand Down Expand Up @@ -400,9 +420,13 @@ private function isWeb(): bool
*
* @throws PageNotFoundException
* @throws RedirectException
*
* @deprecated $returnResponse is deprecated.
*/
protected function handleRequest(?RouteCollectionInterface $routes, Cache $cacheConfig, bool $returnResponse = false)
{
$this->returnResponse = $returnResponse;

$routeFilter = $this->tryToRouteIt($routes);

$uri = $this->determinePath();
Expand Down Expand Up @@ -433,7 +457,8 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache

// If a ResponseInterface instance is returned then send it back to the client and stop
if ($possibleResponse instanceof ResponseInterface) {
return $returnResponse ? $possibleResponse : $possibleResponse->pretend($this->useSafeOutput)->send();
return $this->returnResponse ? $possibleResponse
: $possibleResponse->pretend($this->useSafeOutput)->send();
}

if ($possibleResponse instanceof Request) {
Expand Down Expand Up @@ -512,7 +537,7 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache

unset($uri);

if (! $returnResponse) {
if (! $this->returnResponse) {
$this->sendResponse();
}

Expand Down Expand Up @@ -910,6 +935,8 @@ protected function runController($class)
/**
* Displays a 404 Page Not Found error. If set, will try to
* call the 404Override controller/method that was set in routing config.
*
* @return ResponseInterface|void
*/
protected function display404errors(PageNotFoundException $e)
{
Expand All @@ -934,6 +961,10 @@ protected function display404errors(PageNotFoundException $e)

$cacheConfig = new Cache();
$this->gatherOutput($cacheConfig, $returned);
if ($this->returnResponse) {
return $this->response;
}

$this->sendResponse();

return;
Expand Down
73 changes: 50 additions & 23 deletions tests/system/CodeIgniterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,16 @@ public function testRunEmptyDefaultRoute()
$this->assertStringContainsString('Welcome to CodeIgniter', $output);
}

public function testRunEmptyDefaultRouteReturnResponse()
{
$_SERVER['argv'] = ['index.php'];
$_SERVER['argc'] = 1;

$response = $this->codeigniter->useSafeOutput(true)->run(null, true);

$this->assertStringContainsString('Welcome to CodeIgniter', $response->getBody());
}

public function testRunClosureRoute()
{
$_SERVER['argv'] = ['index.php', 'pages/about'];
Expand All @@ -78,7 +88,7 @@ public function testRunClosureRoute()
$routes->add('pages/(:segment)', static function ($segment) {
echo 'You want to see "' . esc($segment) . '" page.';
});
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -97,7 +107,7 @@ public function testRun404Override()
$routes = Services::routes();
$routes->setAutoRoute(false);
$routes->set404Override('Tests\Support\Controllers\Hello::index');
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -116,7 +126,7 @@ public function testRun404OverrideControllerReturnsResponse()
$routes = Services::routes();
$routes->setAutoRoute(false);
$routes->set404Override('Tests\Support\Controllers\Popcorn::pop');
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -126,6 +136,23 @@ public function testRun404OverrideControllerReturnsResponse()
$this->assertStringContainsString('Oops', $output);
}

public function testRun404OverrideReturnResponse()
{
$_SERVER['argv'] = ['index.php', '/'];
$_SERVER['argc'] = 2;

// Inject mock router.
$routes = Services::routes();
$routes->setAutoRoute(false);
$routes->set404Override('Tests\Support\Controllers\Popcorn::pop');
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

$response = $this->codeigniter->useSafeOutput(true)->run($routes, true);

$this->assertStringContainsString('Oops', $response->getBody());
}

public function testRun404OverrideByClosure()
{
$_SERVER['argv'] = ['index.php', '/'];
Expand All @@ -137,7 +164,7 @@ public function testRun404OverrideByClosure()
$routes->set404Override(static function () {
echo '404 Override by Closure.';
});
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -157,7 +184,7 @@ public function testControllersCanReturnString()
// Inject mock router.
$routes = Services::routes();
$routes->add('pages/(:segment)', static fn ($segment) => 'You want to see "' . esc($segment) . '" page.');
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -182,7 +209,7 @@ public function testControllersCanReturnResponseObject()

return $response->setBody($string);
});
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -209,7 +236,7 @@ public function testControllersCanReturnDownloadResponseObject()

return $response->download('some.txt', 'some text', true);
});
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -228,9 +255,9 @@ public function testControllersRunFilterByClassName()

// Inject mock router.
$routes = Services::routes();
$routes->add('pages/about', static fn () => Services::request()->getBody(), ['filter' => Customfilter::class]);
$routes->add('pages/about', static fn () => Services::incomingrequest()->getBody(), ['filter' => Customfilter::class]);

$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand Down Expand Up @@ -258,7 +285,7 @@ public function testRoutesIsEmpty()
$_SERVER['argc'] = 2;

// Inject mock router.
$router = Services::router(null, Services::request(), false);
$router = Services::router(null, Services::incomingrequest(), false);
Services::injectMock('router', $router);

ob_start();
Expand Down Expand Up @@ -335,7 +362,7 @@ public function testRunRedirectionWithNamed()
}, ['as' => 'name']);
$routes->addRedirect('example', 'name');

$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -358,7 +385,7 @@ public function testRunRedirectionWithURI()
});
$routes->addRedirect('example', 'pages/uri');

$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -382,7 +409,7 @@ public function testRunRedirectionWithURINotSet()
$routes = Services::routes();
$routes->addRedirect('example', 'pages/notset');

$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -405,7 +432,7 @@ public function testRunRedirectionWithHTTPCode303()
$routes = Services::routes();
$routes->addRedirect('example', 'pages/notset', 301);

$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -422,7 +449,7 @@ public function testStoresPreviousURL()
$_SERVER['argc'] = 2;

// Inject mock router.
$router = Services::router(null, Services::request(), false);
$router = Services::router(null, Services::incomingrequest(), false);
Services::injectMock('router', $router);

ob_start();
Expand All @@ -446,7 +473,7 @@ public function testNotStoresPreviousURL()
$routes = Services::routes();
$routes->addRedirect('example', 'pages/notset', 301);

$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand All @@ -470,7 +497,7 @@ public function testNotStoresPreviousURLByCheckingContentType()

return $response->setContentType('image/jpeg', '');
});
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

ob_start();
Expand Down Expand Up @@ -537,7 +564,7 @@ public function testSpoofRequestMethodCanUsePUT()
$this->codeigniter->useSafeOutput(true)->run();
ob_get_clean();

$this->assertSame('put', Services::request()->getMethod());
$this->assertSame('put', Services::incomingrequest()->getMethod());
}

public function testSpoofRequestMethodCannotUseGET()
Expand All @@ -561,7 +588,7 @@ public function testSpoofRequestMethodCannotUseGET()
$this->codeigniter->useSafeOutput(true)->run();
ob_get_clean();

$this->assertSame('post', Services::request()->getMethod());
$this->assertSame('post', Services::incomingrequest()->getMethod());
}

/**
Expand All @@ -588,7 +615,7 @@ public function testPageCacheSendSecureHeaders()

return $response->setBody($string);
});
$router = Services::router($routes, Services::request());
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

/** @var Filters $filterConfig */
Expand Down Expand Up @@ -619,7 +646,7 @@ public function testPageCacheSendSecureHeaders()
// Clear Page cache
command('cache:clear');

// Remove stream fliters
// Remove stream filters
stream_filter_remove($outputStreamFilter);
stream_filter_remove($errorStreamFilter);
}
Expand Down Expand Up @@ -654,15 +681,15 @@ public function testPageCacheWithCacheQueryString($cacheQueryStringValue, int $e
$_SERVER['REQUEST_URI'] = '/' . $testingUrl;
$routes = Services::routes(true);
$routes->add($testingUrl, static function () {
CodeIgniter::cache(0); // Dont cache the page in the run() function because CodeIgniter class will create default $cacheConfig and overwrite settings from the dataProvider
CodeIgniter::cache(0); // Don't cache the page in the run() function because CodeIgniter class will create default $cacheConfig and overwrite settings from the dataProvider
$response = Services::response();
$string = 'This is a test page, to check cache configuration';

return $response->setBody($string);
});

// Inject router
$router = Services::router($routes, Services::request(null, false));
$router = Services::router($routes, Services::incomingrequest(null, false));
Services::injectMock('router', $router);

// Cache the page output using default caching function and $cacheConfig with value from the data provider
Expand Down
2 changes: 1 addition & 1 deletion user_guide_src/source/changelogs/v4.2.8.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ none.
Deprecations
************

none.
- The third parameter ``$returnResponse`` of ``CodeIgniter::handleRequest()`` is deprecated.

Bugs Fixed
**********
Expand Down