Registry expiration as opposed to removing it too soon

Created on 3 April 2019, over 5 years ago
Updated 12 April 2024, 8 months ago

I hit #2881045: Add/remove sessionless requests only → which for my use case I really need that not happening. The proposed solution there was a bit arbitrary. I know it's a setting, but it addressed only the session part and although it might work most of the time, there might be use cases where the session is there but you still might be a cacheable response.

I also sensed that there's never a proper expiry of the url from the registry, which might make sense to have it.

I am proposing a different approach with the following modifications:

- Add an expire column to the registry as well as a configuration for it. It defaults to 0, same behavior as before. Patch includes update hooks for both the config and the table.
- Do not clear the registry when max age is < 1 and when the response is 200: Leave it as not to registered, but not remove it.
- Adds a cron entry to remove expired entries, if configured.

✨ Feature request
Status

Needs work

Version

1.0

Component

Code

Created by

🇦🇷Argentina hanoii 🇦🇷UTC-3

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 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
  • 🇳🇿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.

Production build 0.71.5 2024