Global Styles: Update gutenberg_get_global_stylesheet to use WP_Object_Cache

[mmtr]

Part of #45171
Fixes #43914

What?

This PR updates gutenberg_get_global_stylesheet, so the runtime cache is WP_Object_Cache and not a transient. Furthermore, the stylesheet is cached indefinitely now (previously it was cached for a minute) and invalidated once it's needed to regenerate it (e.g. after the Global Styles CPT changes, after a theme switch, after a theme update).

Why?

See https://core.trac.wordpress.org/ticket/56910:

These caches should be set forever and correctly invalidated. This will improve performance and mean that front end users who hit a page will not suffer poor performance.

How?

• Replace transient_ functions with wp_cache_ functions.
• Hook into the relevant actions to invalidate the cache.

Testing Instructions

• Use a block theme, such as TT3 or TT2
• Go to the site editor
• Switch to a different style variation (Click global styles > Browse styles > Pick one). Save the changes.
• Click view > View site.
• Make sure the new styles are applied immediately.

[codesandbox]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[tyxla]

Those will be truthy in our phpunit setup, so a test will not be able to use the cached data in the current state in a test. We could, of course, add a filter to circumvent that 😅

[spacedmonkey]

Let's remove this, as it hides caching behaviour changes on local dev, with it not great for unit testing and local profiling.

[oandregal]

If we don't have a mechanism to invalidate the cache in development mode, what would you recommend to provide a good developer experience (changes to theme.json are immediately applied)?

[mmtr]

We could, of course, add a filter to circumvent that 😅

Yeah, I discussed this more deeply with @oandregal and probably will give that a try, so we can keep the cache off by default in some scenarios (e.g. during theme development). There might be also legitimate scenarios where plugins want to disable this cache as well (e.g. if they want to change the styles in real time using some of the Global Styles filters), so they could benefit from this filter as well.

[mmtr]

In a845e85 I simplified the conditions that allows the cache to be used by removing the ! REST_REQUEST and ! is_admin() checks, so from now on we should be caching more often.

I kept the ! WP_DEBUG and ! SCRIPT_DEBUG checks since theme developers presumably have these constants turned on when developing a theme and we don't want the cache to interfere with their work.

I also kept the empty( $types ) check, since otherwise we would need to cache the stylesheet for every possible value in $types.

Furthermore, there is now a filter that can override the default cache behavior when needed such as in the unit tests.

[oandregal]

(I want to document some of the rationale for the filter)

The use case the filter addresses is allowing consumers to control cache based on external conditions. Sometimes, it's not enough to clear the cache upon all the events this is hooked into.

Initially, my thinking was that Miguel and I discussed that this could be enough (asking consumers to clear the cache under those conditions):

// In some 3rd party code, such as a plugin:
if ( $some_conditions_are_met ) {
	gutenberg_get_global_stylesheet_clean_cache();
	// So it gets recalculated based on
	// the results of the filters for theme.json data.
}

// ...

// In core code:
gutenberg_get_global_stylesheet(); // will repopulate the cache

However, this code is vulnerable to race conditions.

A simple example would be when the object cache is implemented as a persistent cache across request that is shared among many front-end servers. Other examples exist, but this is easy to visualize. A reproducible race condition would be:

• server A cleans the cache (in the plugin code)
• server B sets the cache (in the core code)
• server A goes to find the value (in the core code) and it comes from the cache, so it's not recalculated as expected

By allowing the plugin code to modify the core behaviour (by means of the filter):

function gutenberg_get_global_stylesheet( $types = array() ) {
	$can_use_cached = apply_filters( 'wp_get_global_stylesheet_can_use_cache', $default );
	// ...

the cache will never be used if the consumer does not want to, avoiding race conditions.

[spacedmonkey]

Let's consider - WordPress/wordpress-develop#3624

[mmtr]

Let's consider - WordPress/wordpress-develop#3624

@spacedmonkey Can you expand on that? As far as I can see the changes from that PR (which improves the caching of the WP_Theme_JSON_Resolver class) are not strictly related to these changes (which improves the caching of the gutenberg_get_global_stylesheet/wp_get_global_stylesheet function), but maybe I missing something else!

[oandregal]

Idea for a potential follow-up: this code currently only caches the whole stylesheet, meaning the one that is enqueued in the front-end (see). I wonder whether it makes sense to cache the others for the editor as well (see) and what effect would it have in the loading times of the editor.

[oandregal]

Thanks for all your work here, Miguel!

I'm pre-approving this PR, though I'd still like to hear from mobile folks in relation to the removal of REST request for caching before merging.

[felixarntz]

@mmtr This looks solid to me, except for one higher level consideration.

[felixarntz]

What would be the use-case for having this controlled by a filter? If this is just to allow turning it off during development, I would suggest we remove this and rethink it, as this consideration does not only apply to the global stylesheet but also anything related to theme.json.

See my other comment - maybe a filter is actually the best approach, but then it should probably be a more general filter? Anyway, my main point is we should discuss this holistically, not across several PRs where we're adding different ideas in code. I think an issue to discuss first would be best for this.

At a minimum, we should align on one thing throughout those PRs, even if we want to remain open to changing it later. Right now I'm afraid the approaches are getting mixed up, which would be a problem.

[oandregal]

The use case for the filter is documented in this other conversation (it's unrelated to development/debug) #45679 (comment)

[felixarntz]

@oandregal Thanks for the context. What I'm not understanding yet though, why is this specifically a problem here? Wouldn't the same concerns apply to pretty much any cache usage in WordPress? How is this one different?

[spacedmonkey]

I think we should add this filter later, I am not sure about it either.

[mmtr]

What would be the use-case for having this controlled by a filter?

One example is that this filter would allow plugins to preview styles in the frontend without saving them.

Plugins have the ability to change the styles using one of the wp_theme_json_data_* filters, but those filters are not applied if the global stylesheet is cached since WP_Theme_JSON_Resolver_Gutenberg::get_merged_data() is never executed.

Without a filter that short-circuits the cache, a plugin that hooks into wp_theme_json_data_user to show how styles would look like in the frontend won't be able to do so if the actual styles (the ones showed to visitors) are already cached.

The plugin could of course invalidate the cache before the styles are previewed, but then those previewed-not-actual-styles would be cached, and the plugin would need to invalidate the cache again before a visitor access the site. This would also be prone to race conditions errors as noted by @oandregal.

With a filter, this would be much easier to handle.

Anyway, happy to remove the filter for now and leave it for a follow-up.

[oandregal]

I may have been too concise on the rationale, let me rephrase/provide more context.

This is the product need we want to support: 3rd parties are able to hook into the theme.json data filters, which control the output of the gutenberg_get_global_stylesheet method. Those filters only run if the value is recalculated, not taken from the cache. There are valid uses cases that are not covered by the current cache invalidation, hence we need a mechanism for 3rd parties to force the values to be recalculated. Some of these use cases are:

• a hosting company may want to recalculate the stylesheet based on a user profile.
• an agency may want to control the styles of a site based on seasonal events (e.g.: black friday).
• a plugin may want to pre-visualize styles in the front-end that were not saved.

That's the why, the product need. I hope this provides a clear context.

Now, discussing implementations to support this product need:

• I pushed against the filter initially (see). I though asking consumers to clean the cache would be enough.
• Upon further inspection, I realized that the code was vulnerable to race conditions (see), an issue that the filter approach does not have.

Is there another alternative we should consider to be able to support this use case?

[felixarntz]

I think being able to add filters on something that goes to cache is the root flaw here that we're trying to work around. Adding a filter to bypass cache is not solving this problem, it would just be a workaround.

I would advocate for leaving out the filter and work on solving the real problem. There must be ways to migrate the filter to be run on the data Post-Cache rather than pre-cache.

[oandregal]

I've talked about this with Felix in private. I understand Felix's point of view, though I have a different perspective. In the interest of making progress, let's leave the filter out of this PR and re-evaluate later, should we need.

[mmtr]

Thanks for the feedback @felixarntz. I see now how requiring plugin developers to disable the cache to be able to hook into the wp_theme_json_data_* filters shouldn't be a required pattern, so I removed the filter for now in 2b16326.

[oandregal]

Changes are fine from a mobile point of view #45679 (comment)

I'd like to merge this tomorrow morning, unless folks want to propose any alternative to implement the use case the filter covers, see #45679 (comment)

[spacedmonkey]

Let stop adding static methods to this class. They are not in part of the WordPress coding standards and are harder to test.

[oandregal]

I've done a quick search for this pattern and there are multiple cases where this pattern is used:

• core upgrader
• language pack upgrader
• community events
• privacy policy
• WP_Theme

I won't list them all, but these should serve to highlight that there are different sensibilities around this. Given this code is architected this way, and it is a very common pattern, I don't think we should do it differently.

[oandregal]

All feedback has been addressed, including removing the filter for now. Thanks everyone to help shape this, it's an important change. Let's merge and iterate.

[andrewserong]

Hi folks! 👋

I'm currently attempting to help investigate a particularly difficult to reproduce issue over in #45713, which (at least superficially) appears to be potentially related to caching of global styles and folks upgrading to the latest WP version (base layout styles as of 6.1 are generated as part of the core theme.json file / processing). Looking at the logic change here, I think there's a potential issue of changes to the core (or Gutenberg) theme.json file not resulting in the cache being cleared.

I imagine we will also need to ensure the cache is cleared when the core or GB theme.json files change, and whenever any of the code that processes those files change. For example, if we adjust any calculations in the Theme JSON class, the cache would need to be cleared, even if the base theme.json file hasn't changed.

What do you think?

[oandregal]

@andrewserong I've looked at the core reports and tried to reproduce with block and classic theme (including some themes people reported were broken). Tried with WP_DEBUG on and off. Had no luck reproducing.

I've commented over there as well #45713 (comment)

I imagine we will also need to ensure the cache is cleared when the core or GB theme.json files change, and whenever any of the code that processes those files change. For example, if we adjust any calculations in the Theme JSON class, the cache would need to be cleared, even if the base theme.json file hasn't changed.

Yeah. The state previous to this PR was unfortunate: the stylesheet would be cached for a 1 minute, no matter the context or what events were dispatched. That's precisely what this PR fixes. This is the full list of events upon which the cache is flushed:

• user data for GS is saved: modifies the styles provided by the user
• theme switch: modifies the styles provided by the theme
• preview theme: needs the styles provided by the theme
• plugin activated/deactivated: can hook into the theme.json data filters, etc.
• core, plugins, or the current theme is updated: code updates may modify how styles work

There's also the condition to not cache when WP_DEBUG is on, to make sure changes to theme.json while developing are immediately reflected in the styles.

Do you think is there any other event that should clear the cache?

[andrewserong]

I've looked at the core reports and tried to reproduce with block and classic theme (including some themes people reported were broken). Tried with WP_DEBUG on and off. Had no luck reproducing.

Thank you for giving it a test, and for the extra comments, much appreciated!

Do you think is there any other event that should clear the cache?

That looks like a pretty thorough list to me — this is probably a bit edge-casey, but there are a couple more potential cases that could be difficult to predict or catch:

• Plugins that change behaviour based on user settings within the plugin itself, outside of an update cycle. Many caching plugins offer configuration options and conditionals that change the behaviour of the caching outside of the plugin update cycle. I'm not sure if there is a good way to catch for that, though, and it looks like this case was sort of already touched on by the discussion here: Global Styles: Update gutenberg_get_global_stylesheet to use WP_Object_Cache #45679 (comment)
• Sites running WordPress trunk or Gutenberg trunk where an update or upgrade hasn't yet occurred. This is probably both a situation that is less critical to fix (since it's less likely production sites will be running trunk where changes occur without a version bump), however could also be a challenging one to debug if we wind up encountering any issues there.

Please forgive my naive question here, but is the expectation with object cache that the external implementation will flush the cache regularly, so that's why we don't need to worry about not having an expiry timeout? From a quick check of popular plugins, I see W3 Total Cache has a default cache lifetime of 180 seconds, or 1 hour if persisting object cache to disk, which both sound pretty good to me, so I think this likely mitigates the latter concern above 👍

[oandregal]

Plugins that change behaviour based on user settings within the plugin itself, outside of an update cycle. Many caching plugins offer configuration options and conditionals that change the behaviour of the caching outside of the plugin update cycle.

Agreed, this is the last bit to figure out. This is something that has been discussed in a few places:

• Global Styles: Update gutenberg_get_global_stylesheet to use WP_Object_Cache #45679 (comment)
• Add WP_Object_Cache to the gutenberg_get_global_settings method #45372 (comment)
• Control whether theme (e.g. theme.json) cache usage is enabled through a dedicated function #45912

I've shared three options so far:

1. Consumers can always call the function to clean the cache, should they need. This can be done now by the consumer.
2. We may want to introduce a specific filter to short-circuit the cache, as proposed in this same PR. That was punted to a follow-up, and it's now being discussed in the issue.
3. We could consider cleaning the cache upon any option is added/updated/deleted. That would cover any dynamic behavior that depends on them. The performance will still be great, given that reading a site is many times more frequent than changing an option.

There are some places in core I need to look at to understand our choices better. For example, WP_Theme, uses a filter.

Sites running WordPress trunk or Gutenberg trunk where an update or upgrade hasn't yet occurred. This is probably both a situation that is less critical to fix (since it's less likely production sites will be running trunk where changes occur without a version bump), however could also be a challenging one to debug if we wind up encountering any issues there.

What do you mean by this? Sites that, for example, are controlled by git (not using the WordPress update mechanism)? If so, this is similar to someone updating a plugin via FTP, as Jonny shared here. The answer is that this code will work perfectly in the next request if they don't use a persistent cache plugin (the stylesheet is recalculated for each request). If they do have a persistent cache plugin, they can always clear the cache manually.

Please forgive my naive question here, but is the expectation with object cache that the external implementation will flush the cache regularly, so that's why we don't need to worry about not having an expiry timeout?

That's a great question. I don't know about the specific implementations. Though I argued that setting an expiration would be a good last resort, and was advised not to. Happy to revisit if this is problematic.

[andrewserong]

I've shared three options so far:

Thanks for the detailed follow-up, good to know these issues are being teased apart here!

What do you mean by this? Sites that, for example, are controlled by git (not using the WordPress update mechanism)? ...
The answer is that this code will work perfectly in the next request if they don't use a persistent cache plugin (the stylesheet is recalculated for each request). If they do have a persistent cache plugin, they can always clear the cache manually.

Yes, that's pretty much what I had in mind, and that answer covers my question nicely, thanks! 🙂

Though I #45543 (comment) that setting an expiration would be a good last resort, and was advised not to.

Good to know I wasn't alone in thinking of a fallback expiry timeout. It sounds like the advice from the others is that the external implementation is responsible for the timeliness of cache flushing, which makes sense to me.

Happy to revisit if this is problematic.

Agreed — let's revisit if we encounter any issues with it 👍

Overall it sounds like there's been some really good progress in updates to the caching here, nice work, all!

[oandregal]

This is being backported at WordPress/wordpress-develop#3712