-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: page cache saves Response data before running after filters #6282
Merged
kenjis
merged 9 commits into
codeigniter4:develop
from
kenjis:fix-cachePage-secureheaders
Jul 30, 2022
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
ca73c0f
docs: add/fix PHPDocs
kenjis 6e733b0
refactor: do not return void return method
kenjis f19169d
fix: page cache saves Response before running after filters
kenjis 9b5a124
refactor: initialize static::$cacheTTL
kenjis 2814d57
docs: add changelog
kenjis f47842f
docs: fix @var
kenjis 91f6f1c
docs: add upgrade guide
kenjis 7875a2c
docs: fix typo
kenjis e0964ed
docs: fix by proofreading
kenjis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,6 @@ | |
use CodeIgniter\HTTP\IncomingRequest; | ||
use CodeIgniter\HTTP\RedirectResponse; | ||
use CodeIgniter\HTTP\Request; | ||
use CodeIgniter\HTTP\RequestInterface; | ||
use CodeIgniter\HTTP\ResponseInterface; | ||
use CodeIgniter\HTTP\URI; | ||
use CodeIgniter\Router\Exceptions\RedirectException; | ||
|
@@ -55,7 +54,7 @@ class CodeIgniter | |
/** | ||
* App startup time. | ||
* | ||
* @var mixed | ||
* @var float|null | ||
*/ | ||
protected $startTime; | ||
|
||
|
@@ -298,17 +297,18 @@ protected function initializeKint() | |
* tries to route the response, loads the controller and generally | ||
* makes all of the pieces work together. | ||
* | ||
* @throws Exception | ||
* @throws RedirectException | ||
* | ||
* @return bool|mixed|RequestInterface|ResponseInterface|void | ||
* @return ResponseInterface|void | ||
*/ | ||
public function run(?RouteCollectionInterface $routes = null, bool $returnResponse = false) | ||
{ | ||
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`.'); | ||
} | ||
|
||
static::$cacheTTL = 0; | ||
|
||
$this->startBenchmark(); | ||
|
||
$this->getRequestObject(); | ||
|
@@ -321,7 +321,9 @@ 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'); | ||
|
||
return $this->sendResponse(); | ||
$this->sendResponse(); | ||
|
||
return; | ||
} | ||
|
||
Events::trigger('pre_system'); | ||
|
@@ -409,7 +411,7 @@ private function isWeb(): bool | |
* @throws PageNotFoundException | ||
* @throws RedirectException | ||
* | ||
* @return mixed|RequestInterface|ResponseInterface | ||
* @return ResponseInterface | ||
*/ | ||
protected function handleRequest(?RouteCollectionInterface $routes, Cache $cacheConfig, bool $returnResponse = false) | ||
{ | ||
|
@@ -475,6 +477,9 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache | |
// so it can be used with the output. | ||
$this->gatherOutput($cacheConfig, $returned); | ||
|
||
// After filter debug toolbar requires 'total_execution'. | ||
$this->totalTime = $this->benchmark->getElapsedTime('total_execution'); | ||
|
||
// Never run filters when running through Spark cli | ||
if (! $this->isSparked()) { | ||
$filters->setResponse($this->response); | ||
|
@@ -496,6 +501,17 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache | |
$this->response = $response; | ||
} | ||
|
||
// Cache it without the performance metrics replaced | ||
// so that we can have live speed updates along the way. | ||
// Must be run after filters to preserve the Response headers. | ||
if (static::$cacheTTL > 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you certain that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind, I see this was just moved from below! |
||
$this->cachePage($cacheConfig); | ||
} | ||
|
||
// Update the performance metrics | ||
$output = $this->displayPerformanceMetrics($this->response->getBody()); | ||
$this->response->setBody($output); | ||
|
||
// 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)); | ||
|
@@ -641,7 +657,7 @@ protected function forceSecureAccess($duration = 31_536_000) | |
* | ||
* @throws Exception | ||
* | ||
* @return bool|ResponseInterface | ||
* @return false|ResponseInterface | ||
*/ | ||
public function displayCache(Cache $config) | ||
{ | ||
|
@@ -664,7 +680,8 @@ public function displayCache(Cache $config) | |
$this->response->setHeader($name, $value); | ||
} | ||
|
||
$output = $this->displayPerformanceMetrics($output); | ||
$this->totalTime = $this->benchmark->getElapsedTime('total_execution'); | ||
$output = $this->displayPerformanceMetrics($output); | ||
$this->response->setBody($output); | ||
|
||
return $this->response; | ||
|
@@ -734,8 +751,6 @@ protected function generateCacheName(Cache $config): string | |
*/ | ||
public function displayPerformanceMetrics(string $output): string | ||
{ | ||
$this->totalTime = $this->benchmark->getElapsedTime('total_execution'); | ||
|
||
return str_replace('{elapsed_time}', (string) $this->totalTime, $output); | ||
} | ||
|
||
|
@@ -956,7 +971,10 @@ protected function display404errors(PageNotFoundException $e) | |
* Gathers the script output from the buffer, replaces some execution | ||
* time tag in the output and displays the debug toolbar, if required. | ||
* | ||
* @param Cache|null $cacheConfig Deprecated. No longer used. | ||
* @param ResponseInterface|string|null $returned | ||
* | ||
* @deprecated $cacheConfig is deprecated. | ||
*/ | ||
protected function gatherOutput(?Cache $cacheConfig = null, $returned = null) | ||
{ | ||
|
@@ -992,14 +1010,6 @@ protected function gatherOutput(?Cache $cacheConfig = null, $returned = null) | |
$this->output .= $returned; | ||
} | ||
|
||
// Cache it without the performance metrics replaced | ||
// so that we can have live speed updates along the way. | ||
if (static::$cacheTTL > 0) { | ||
$this->cachePage($cacheConfig); | ||
} | ||
|
||
$this->output = $this->displayPerformanceMetrics($this->output); | ||
|
||
$this->response->setBody($this->output); | ||
} | ||
|
||
|
@@ -1069,6 +1079,8 @@ public function spoofRequestMethod() | |
/** | ||
* Sends the output of this request back to the client. | ||
* This is what they've been waiting for! | ||
* | ||
* @return void | ||
*/ | ||
protected function sendResponse() | ||
{ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ upgrading from. | |
.. toctree:: | ||
:titlesonly: | ||
|
||
upgrade_422 | ||
upgrade_421 | ||
upgrade_420 | ||
upgrade_418 | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it means that
cacheTTL
value is reset in every run?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
$cacheTTL will be set by a Controller.