Follow-up to #2476407: Use CacheableResponseInterface to determine which responses should be cached
PageCache caches 'no-store' and 'private' responses. There is no way except for a ResponsePolicy to make a response uncacheable.
That is a bug.
#2476407: Use CacheableResponseInterface to determine which responses should be cached has test coverage for PageCache and intended to test this behavior, but it does not work correctly. (Bug in tests)
At minimum do:
diff --git a/core/modules/page_cache/src/StackMiddleware/PageCache.php b/core/modules/page_cache/src/StackMiddleware/PageCache.php
index 23ddb84..5a1d99c 100644
--- a/core/modules/page_cache/src/StackMiddleware/PageCache.php
+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -220,6 +220,10 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
return $response;
}
+ if ($response->headers->hasCacheControlDirective('no-store') || $response->headers->getCacheControlDirective('private')) {
+ return $response;
+ }
+
// Use the actual timestamp from an Expires header, if available.
$date = $response->getExpires()->getTimestamp();
$expire = ($date > time()) ? $date : Cache::PERMANENT;
- Patch it
- Fix tests
- None
- no-store and private responses are no longer cached, but that should have been the case already from an Expectation point-of-view since #2476407: Use CacheableResponseInterface to determine which responses should be cached, so it is not a true API change. Just fixing behavior that should have been fixed already.
Comments
Comment #2
fabianx commentedComment #3
berdirThis isn't going to work as expected right now.
If you disable *external* caching on the performance settings page, then we send a private header, but we still cache it in the internal page cache. That's a perfectly valid reason IMHO. For example, browsers respect max-age partially and cache pages. We've seen that on some of our D8 sites, despite cache tag invalidation working perfectly with fastly, you had to force re-fresh in the browser to see the updated page. That's why we now send a very short max-age but a separate header to control caching on fastly.
This would obviously break that.
Comment #4
znerol commented@Berdir very much agree on the edge-cache vs browser-cache difference. This still seems to confuse people working at cacheability issues.
Would it be acceptable to only apply the proposed restriction to plain Symfony responses? I.e., if there is cacheability metadata available exclusively use that to determine whether or not to store it (and for how long), and for plain Symfony responses implement the logic used in HttpCache.
Comment #5
effulgentsia commentedGiven the above comments, should this now be closed as "by design" now that as of #2527126: Only send X-Drupal-Cache-Tags and -Contexts headers when developer explicitly enables them, PageCache only caches CacheableResponseInterface responses? Or is there more to do here? I like the idea of letting CacheableResponseInterface be the sole API for PageCache, and leaving response headers for external caches (proxies and browsers), but not sure if there's something about this issue that still makes sense to do.
Comment #6
xjmThanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section
<h3>Why this should be an RC target</h3>to the summary.Comment #17
catchMarking this as 'works as designed' per #3 #2593887-3: Follow-up: Do not cache private or no-store responses in PageCache.