Hello,
Thank you very much for the module.
Max-age is set in the cache header but the Expires header is not set so the page_cache module set the cache permanently anyway.
Maybe it is the same problem as #2916705: Page cache isn't invalidated
I will upload a patch.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | cache_control_override-expires_header-2962699-2-x.7.patch | 2 KB | feng-shui |
| #5 | cache_control_override-expires_header-2962699-5.patch | 2.18 KB | mark_fullmer |
| #5 | no-block-override.png | 73.92 KB | mark_fullmer |
| #5 | with-block-override.png | 74.92 KB | mark_fullmer |
Comments
Comment #2
grimreaperHere is the patch.
Thanks for the review.
Comment #3
grimreaperSame patch but made against the 8.x-1.0-alpha1 tag.
Comment #4
grimreaperSorry, I had not tested the dependency injection. Here are two new patches that do it properly.
Comment #5
mark_fullmerThe previous patches weren't in the right format to be apply-able; I've recreated the same changes in the attached patch. I've also verified the fix, though I'll forego RTBC'ing this, since I'm adding a patch now.
For future reference, a quick manual verification of this (in the absence of automated tests) is below:
A content block placed on a page will yield the following:

.... while a page without that block (assuming a global TTL set to "60" will show:

Importantly, I think this "Expires" behavior was not the original intent of the module (rather, it was intended for reverse proxies & edge caches; however, I think there are valid use cases for letting the "Expires" value bubble up to the Internal Page Cache's cache_page bin, as this change accomplishes (e.g., a widget that is responsible for retrieving external content every X minutes).
Comment #6
leon kessler commentedI don't see why this patch should not be incorporated into the project. Yes most people will be using this module with an external cache/CDN, and probably have the internal page cache module uninstalled.
However, for those that need to use the page_cache module, it's either this patch, or patching Drupal core with https://www.drupal.org/project/drupal/issues/2352009#comment-14064154 (which is the latest patch on that issue at this current time).
I don't believe there is any harm in setting the correct Expires header, but I suppose there could be some unexpected consequences for someones setup (although you could argue if this does cause an issue for someone, it's a symptom of a bug in their infrastructure, rather than with the Drupal instance itself).
Comment #7
feng-shui commentedRe-roll against 2.x.
Comment #8
prudloff commentedWe should probably decide which patch to keep between #2962699: Expires header not set and #2916705: Page cache isn't invalidated.
Comment #9
prudloff commentedClosing as a duplicate of #2916705: Page cache isn't invalidated, the two patches basically do the same thing and the other one has a more recent patch.
Comment #10
prudloff commented