- First commit to issue fork.
- ðŸ‡ðŸ‡ºHungary szato
Rebased MR and added 2 extra commits:
- 30cb67ca - Added 1 hour; 3,6,9,12 hours; 1 day; 1 weeks.
- 12a436fb - Phpcs fixes.
- Status changed to Needs work
8 months ago 2:54am 25 March 2024 - 🇳🇿New Zealand ericgsmith
I think this is something that the module should be doing and the approach looks sensible to me - thank you.
I looks like an unintended change was introduced that broke the tests - I think we will also need more test coverage that the expiration is working.
- 🇳🇿New Zealand ericgsmith
Have fixed the existing test and taken a closer look.
I think we need to be mindful here for pages using the internal page cache module.
The behaviour for page_cache is permanent by default:
// The response passes all of the above checks, so cache it. Page cache // entries default to Cache::PERMANENT since they will be expired via cache // tags locally. Because of this, page cache ignores max age.
While some setups will have the internal page cache disabled and rely only on an external page cache, not all will.
If page cache is enabled, it looks like the url registry item is removed after expiration, but the as the page cache entry will remain and continue to be returned by Drupal this never gets added back to the registry because of:
// When page_cache is enabled, skip HITs to prevent running code twice. if ($cached = $response->headers->get('X-Drupal-Cache')) { if ($cached === 'HIT') { return FALSE; } }
I haven't thought too much on this - perhaps we can check if page cache is enabled here and return TRUE? We don't want to introduce more work for sites only using external cache.