Add cache group to block pattern cache

[spacedmonkey]

In [56978] block patterns were cache in object cache. However, no cache group was provided. Example.

        private function get_pattern_cache() {
            if ( ! $this->exists() ) {
               return false;
            }
            $pattern_data = wp_cache_get( 'wp_theme_patterns_' . $this->stylesheet );

This makes it hard to control this cache. A cache group should be provided to bring inline with other caches.

Question: should this cache group be a global cache group.

[spacedmonkey]

For your consideration. @joemcgill @flixos90 @hellofromTonya

[joemcgill]

I agree that we should address this. Originally, block patterns were going to be cached to a transient, but no expiration strategy was set, nor are transients easily clearable. This concern was raised in #59633.

As a compromise, this was moved to the global cache group so that sites with an object cache would still get the benefits of persistent caching, while we considered a better wholistic caching strategy for expensive operations related to getting and parsing data from theme files in block themes (see: #59719).

Generally, it would make sense to cache all of these files to the themes cache group, but that group is non-persistent by default, unless someone has opted in to group being persistent by filtering the wp_cache_themes_persistently value.

Once a wholistic strategy is determined in #59719, we can apply that strategy to the pattern cache methods in WP_Theme.

Moving this to the 6.5 milestone.

[flixos90]

Agreed, this is somewhat blocked by #59719. We should make sure not to choose this cache group in isolation.

Let's aim to come to a decision on that broader direction ticket early in the new year so that we can start addressing the various tickets in 6.5.

[flixos90]

Assigning this in relation to https://core.trac.wordpress.org/ticket/59719#comment:13. Probably best to implement the suggested changes here first before proceeding with #59600, just to validate this actually works the way we want on the existing functionality.

[joemcgill]

Thanks @flixos90. Based on https://core.trac.wordpress.org/ticket/59719#comment:13, let's both add a new theme_files cache group and use this as an opportunity to explore the idea of backing these caches up with a transient when a persistent object cache is not present.

A new PR should:

1. Introduce a new theme_files cache group via wp_cache_add_global_groups()
2. Apply the new cache group when setting the cached block pattern data in WP_Theme::set_pattern_cache
3. Set the cache to a site transient, using set_site_transient() in WP_Theme::set_pattern_cache with an expiration time (e.g. HOUR_IN_SECONDS) if wp_using_ext_object_cache() returns false. In single sites, this will automatically fall back to a local transient.
4. The expiration time should only be used for transients and not the persistent cache, and the value should be filterable. We should design the filter so the same hook can be extended to block templates and template parts, so the filter should pass additional context that identifies that the data the expiration applies to in this case is block patterns. Ex. apply_filters( 'wp_block_theme_files_cache_ttl', HOUR_IN_SECONDS, 'patterns' ) so that these TTL values can be independently controlled.
5. Handle deleting the transient in WP_Theme::delete_pattern_cache when wp_using_ext_object_cache() returns false.

Note that WP_Theme::delete_pattern_cache is already called from WP_Theme::cache_delete and during switch_theme(), but additional cache invalidation may need to be considered for things like editing theme files directly in the legacy theme editor (/wp-admin/theme-editor.php). Additionally, WP_Theme::set_pattern_cache is already only called if WP_DEVELOPMENT_MODE is not set to theme or all, and that should remain the responsibility of the code calling WP_Theme::set_pattern_cache, not in the implementation of that helper method.

@flixos90 and @spacedmonkey, feel free to add anything that I missed.

[flixos90]

Thanks @joemcgill, LGTM.

Regarding cache invalidation when adding patterns in the theme file editor, I'm thinking we may want to (at least initially) leave this out of the PR here, as it ties in more closely with #59662 which is already on my list for next week. IMO it would make sense to use that ticket to tackle that part.

[spacedmonkey]

Replying to joemcgill:

2. Apply the new cache group when setting the cached block pattern data in WP_Theme::set_pattern_cache
3. Set the cache to a site transient, using set_site_transient() in WP_Theme::set_pattern_cache with an expiration time (e.g. HOUR_IN_SECONDS) if wp_using_ext_object_cache() returns false. In single sites, this will automatically fall back to a local transient.
4. The expiration time should only be used for transients and not the persistent cache, and the value should be filterable. We should design the filter so the same hook can be extended to block templates and template parts, so the filter should pass additional context that identifies that the data the expiration applies to in this case is block patterns. Ex. apply_filters( 'wp_block_theme_files_cache_ttl', HOUR_IN_SECONDS, 'patterns' ) so that these TTL values can be independently controlled.
5. Handle deleting the transient in WP_Theme::delete_pattern_cache when wp_using_ext_object_cache() returns false.

All of this feel out of scope of what was being ask for in this ticket. This can, which I am personally unsure about, should be handled in a different ticket. Let’s keep this ticket to adding a cache group, or a larger ticket about improving caching here, which is a different discussion. Please read https://core.trac.wordpress.org/ticket/59719#comment:15. Until we have solved cache invalidation, we shouldn’t cache anything about caching here.

We should also consider, just making the cache group theme_files a non persistent group if development mode is enabled. This makes logic much cleaner.

[joemcgill]

Replying to spacedmonkey:

All of this feel out of scope of what was being ask for in this ticket.

I agree. 1 and 2 above are the minimum that needs to be done to close this ticket. However, I think it's worth trying to back data in this cache group with transients (3–5 above) at the same time, based on the conversation in #59719. Since there isn't a rush to create the new group only, I'd prefer to see us iterate on the full idea and scale back if needed rather than juggling various follow-up tickets during this release cycle.

We should also consider, just making the cache group theme_files a non persistent group if development mode is enabled. This makes logic much cleaner.

Good suggestion. That is worth exploring as part of this change.

Trac ticket: https://core.trac.wordpress.org/ticket/60120

[spacedmonkey]

I created a ​PR based on the discussion in this ticket. Ready for review.

CC @flixos90 @joemcgill

[flixos90]

@spacedmonkey @joemcgill
I'm not sure about the value of conditionally having the new group non-persistent. In all other places in core we avoid caching entirely based on WP_DEVELOPMENT_MODE. I think changing that warrants a bigger discussion.

I agree with @joemcgill that we need to consider this as part of the broader solution, though I also think it's fair to go with what you suggested @spacedmonkey and address only the minimum requirements that this ticket originally requested as part of the PR.

Given you previously suggested to limit the scope here, I'd say let's limit the PR to purely introducing the theme_files cache group, which was the only original purpose of this ticket.

We can then address the broader approach with a single PR against #59719.

@felixarntz I have revert some of the changed based on your feedback.

[spacedmonkey]

Replying to flixos90:

Given you previously suggested to limit the scope here, I'd say let's limit the PR to purely introducing the theme_files cache group, which was the only original purpose of this ticket.

I have actioned your feedback.

[spacedmonkey]

@joemcgill I think the PR is ready to commit. Can you take another look?

[joemcgill]

Thanks @spacedmonkey. This does look ready to commit. Left some feedback on the PR, but not a blocker.

[spacedmonkey]

In 57608:

Themes: Add 'theme_files' cache group to block pattern cache operations.

Use 'theme_files' cache group for block pattern caches. Previously, block pattern cache data was not stored in a cache group and used the default group. This new cache group, is setup as a global cache group, meaning that sites using multisite, will have a single cache for block pattern data per theme. This change also no longer invalidate block pattern caches in multisite instances, meaning block pattern caches can be shared between sites on a network, meaning less repeated data in the object cache.

Props spacedmonkey, flixos90, joemcgill.
Fixes #60120.

@spacedmonkey I think this was merged in 57608 so I'm closing, but feel free to reopen if that's incorrect.