-
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
feat: HTTP method-aware web page caching #8364
Conversation
da37c9c
to
d182491
Compare
system/Filters/PageCache.php
Outdated
@@ -65,6 +66,10 @@ public function after(RequestInterface $request, ResponseInterface $response, $a | |||
{ | |||
assert($request instanceof CLIRequest || $request instanceof IncomingRequest); | |||
|
|||
if ($this->isNotCacheableMethod($request)) { |
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.
So what's cachable? GET and HEAD? If that's all, and HEAD isn't something we really need to worry about caching, wouldn't it be simpler to just check if $request->getMethod() !== 'get'
?
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.
ResponseCache also caches the headers, so in theory, it can be useful for HEAD... however, as I mentioned in my comment, I would make it configurable and stick to the GET by default.
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.
So what's cachable?
GET, HEAD, POST, PATCH, CLI.
But a dev must put $this->cachePage($n);
in controller.
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.
But a dev must put $this->cachePage($n); in controller.
Ok, that changes a lot.
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.
I dropped the check.
To begin with, page caching must be specified by developers in controllers.
There is no need for the filter to dare to check the HTTP method to limit the cache functionality.
Besides, if a developer wants to determine to enable caching by HTTP method, they can customize the filter.
Enabling caching for POST requests by default is rather controversial. I would be in favor of adding a config for this. While I can imagine a scenario where we want to cache the POST response, the most common action for POST requests (which change the state of the application) will be to respond with new content every time. |
Ah, strictly speaking, the current this PR has a breaking change? |
d182491
to
c6a9112
Compare
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.
LGTM
c6a9112
to
0c01fbc
Compare
I forgot to document. Added. |
2715695
to
5f7c7d5
Compare
LGTM |
Description
Closes #8363
Enhancement to https://codeigniter4.github.io/CodeIgniter4/general/caching.html
The current issue:
If we use RESTful Resource Handling, operation Create and List have the same URI.
https://codeigniter4.github.io/CodeIgniter4/incoming/restful.html#presenter-controller-comparison
If we enable page caching to List (GET), Create (POST) will return the cached List response.
Checklist: