Skip to content
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.

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

refactor: remove deprecated properties and methods in CodeIgniter class #8050

Merged
merged 9 commits into from
Oct 20, 2023
2 changes: 1 addition & 1 deletion phpstan-baseline.php
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@
];
$ignoreErrors[] = [
'message' => '#^Construct empty\\(\\) is not allowed\\. Use more strict comparison\\.$#',
'count' => 6,
'count' => 5,
'path' => __DIR__ . '/system/CodeIgniter.php',
];
$ignoreErrors[] = [
Expand Down
83 changes: 18 additions & 65 deletions system/CodeIgniter.php
Original file line number Diff line number Diff line change
Expand Up @@ -135,25 +135,6 @@ class CodeIgniter
*/
protected static $cacheTTL = 0;

/**
* Request path to use.
*
* @var string
*
* @deprecated No longer used.
*/
protected $path;

/**
* Should the Response instance "pretend"
* to keep from setting headers/cookies/etc
*
* @var bool
*
* @deprecated No longer used.
*/
protected $useSafeOutput = false;

/**
* Context
* web: Invoked by HTTP request
Expand All @@ -171,7 +152,7 @@ class CodeIgniter
/**
* Whether to return Response object or send response.
*
* @deprecated No longer used.
* @deprecated 4.4.0 No longer used.
*/
protected bool $returnResponse = false;

Expand Down Expand Up @@ -336,6 +317,8 @@ private function configureKint(): void
* tries to route the response, loads the controller and generally
* makes all the pieces work together.
*
* @param bool $returnResponse Used for testing purposes only.
*
* @return ResponseInterface|void
*/
public function run(?RouteCollectionInterface $routes = null, bool $returnResponse = false)
Expand All @@ -355,9 +338,9 @@ public function run(?RouteCollectionInterface $routes = null, bool $returnRespon
$this->getRequestObject();
$this->getResponseObject();

$this->spoofRequestMethod();

try {
$this->forceSecureAccess();

$this->response = $this->handleRequest($routes, config(Cache::class), $returnResponse);
} catch (ResponsableInterface|DeprecatedRedirectException $e) {
$this->outputBufferingEnd();
Expand All @@ -381,22 +364,6 @@ public function run(?RouteCollectionInterface $routes = null, bool $returnRespon
$this->sendResponse();
}

/**
* Set our Response instance to "pretend" mode so that things like
* cookies and headers are not actually sent, allowing PHP 7.2+ to
* not complain when ini_set() function is used.
*
* @return $this
*
* @deprecated No longer used.
*/
public function useSafeOutput(bool $safe = true)
{
$this->useSafeOutput = $safe;

return $this;
}

/**
* Invoked via php-cli command?
*/
Expand Down Expand Up @@ -433,8 +400,6 @@ public function disableFilters(): void
*/
protected function handleRequest(?RouteCollectionInterface $routes, Cache $cacheConfig, bool $returnResponse = false)
{
$this->forceSecureAccess();

if ($this->request instanceof IncomingRequest && strtolower($this->request->getMethod()) === 'cli') {
return $this->response->setStatusCode(405)->setBody('Method Not Allowed');
}
Expand All @@ -449,7 +414,7 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache

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

$uri = $this->determinePath();
$uri = $this->request->getPath();

if ($this->enableFilters) {
// Start up the filters
Expand Down Expand Up @@ -621,6 +586,9 @@ protected function startBenchmark()
* @param CLIRequest|IncomingRequest $request
*
* @return $this
*
* @internal Used for testing purposes only.
* @testTag
*/
public function setRequest($request)
{
Expand All @@ -637,6 +605,8 @@ public function setRequest($request)
protected function getRequestObject()
{
if ($this->request instanceof Request) {
$this->spoofRequestMethod();

return;
}

Expand All @@ -647,6 +617,8 @@ protected function getRequestObject()
}

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

$this->spoofRequestMethod();
Copy link
Member

Choose a reason for hiding this comment

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

The repeat of this and the fact that spoofRequestMethod() only works on the Request object itself makes me think this should eventually be moved into IncomingRequest. This could be a constructor override or a feature of getMethod() (and maybe adding getRawMethod() or something?) or a new method like getMethodWithSpoofing(). That would also make an easy, central place to have this be configurable for security purposes: turn on/off, allow-list of methods, etc.

For this PR I think what you have is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the code, it is indeed all operations to the Request object.

public function spoofRequestMethod()
{
// Only works with POSTED forms
if (strtolower($this->request->getMethod()) !== 'post') {
return;
}
$method = $this->request->getPost('_method');
if (empty($method)) {
return;
}
// Only allows PUT, PATCH, DELETE
if (in_array(strtoupper($method), ['PUT', 'PATCH', 'DELETE'], true)) {
$this->request = $this->request->setMethod($method);
}
}

}

/**
Expand Down Expand Up @@ -808,14 +780,14 @@ protected function tryToRouteIt(?RouteCollectionInterface $routes = null)
// $routes is defined in Config/Routes.php
$this->router = Services::router($routes, $this->request);

$path = $this->determinePath();
$uri = $this->request->getPath();

$this->benchmark->stop('bootstrap');
$this->benchmark->start('routing');

$this->outputBufferingStart();

$this->controller = $this->router->handle($path);
$this->controller = $this->router->handle($uri);
$this->method = $this->router->methodName();

// If a {locale} segment was matched in the final route,
Expand All @@ -834,31 +806,12 @@ protected function tryToRouteIt(?RouteCollectionInterface $routes = null)
* on the CLI/IncomingRequest path.
*
* @return string
*/
protected function determinePath()
{
if (! empty($this->path)) {
return $this->path;
}

return method_exists($this->request, 'getPath') ? $this->request->getPath() : $this->request->getUri()->getPath();
}

/**
* Allows the request path to be set from outside the class,
* instead of relying on CLIRequest or IncomingRequest for the path.
*
* This is not used now.
*
* @return $this
*
* @deprecated No longer used.
* @deprecated 4.5.0 No longer used.
*/
public function setPath(string $path)
protected function determinePath()
{
$this->path = $path;

return $this;
return $this->request->getPath();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions tests/system/CodeIgniterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public function testRunEmptyDefaultRouteReturnResponse(): void
$_SERVER['argv'] = ['index.php'];
$_SERVER['argc'] = 1;

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

$this->assertStringContainsString('Welcome to CodeIgniter', $response->getBody());
}
Expand Down Expand Up @@ -164,7 +164,7 @@ public function testRun404OverrideReturnResponse(): void
$router = Services::router($routes, Services::incomingrequest());
Services::injectMock('router', $router);

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

$this->assertStringContainsString('Oops', $response->getBody());
}
Expand Down
11 changes: 11 additions & 0 deletions user_guide_src/source/changelogs/v4.5.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,14 @@ Model
- ``Model::idValue()``
- ``Model::classToArray()``

CodeIgniter
-----------

- ``$path``
- ``$useSafeOutput``
- ``useSafeOutput()``
- ``setPath()``

Enhancements
************

Expand Down Expand Up @@ -169,6 +177,9 @@ Changes
Deprecations
************

- **CodeIgniter:** The ``determinePath()`` method has been deprecated. No longer
used.

Bugs Fixed
**********

Expand Down
Loading