Global Styles: Add compat filter for toggling stylesheet cache

[tyxla]

What?

This PR intends to add the filter for disabling the global styles stylesheet cache that we're suggesting in https://core.trac.wordpress.org/ticket/55780 (see WordPress/wordpress-develop#2732) to the compatibility functions.

This should land only if https://core.trac.wordpress.org/ticket/55780 lands.

Any idea who's a good person to ping for a review of this PR?

Why?

Necessary for backwards compatibility. See the ticket above for a rationale on why the filter is necessary.

How?

Introducing a simple filter that defaults to true that will allow disabling of the cache if necessary.

Testing Instructions

• Add add_filter( 'global_styles_enable_cache', __return_false ); somewhere, in a plugin or mu-plugin.
• Load the frontend of a site.
• Verify that global_styles_* transients are not set.

[annezazu]

@oandregal might you be able to take a look here? You came to mind to ping for this but, if that's off based, just let me know. If you can, would be great to ping someone else who you think is better suited.

[oandregal]

👋 Always happy to help if I can.

@tyxla I wanted to surface #45171 There's a section about performance there, and one of the things I'd like to do is to improve the way this code uses the caches. It's been suboptimal and has caused issues. I'm hoping it can be used as the overview/track issue to coordinate efforts among people (editor folks, performance folks, etc.).

What would you think of a different approach: instead of short-circuiting what core does via a filter, would you think is it worth improving the current state by stop using transients and using the object cache instead? Would that help?

[oandregal]

Listed this in the overview task.

[tyxla]

@tyxla I wanted to surface #45171 There's a section about performance there, and one of the things I'd like to do is to improve the way this code uses the caches. It's been suboptimal and has caused issues. I'm hoping it can be used as the overview/track issue to coordinate efforts among people (editor folks, performance folks, etc.).

That sounds great, thanks for wrangling it, @oandregal!

What would you think of a different approach: instead of short-circuiting what core does via a filter, would you think is it worth improving the current state by stop using transients and using the object cache instead? Would that help?

Yes, it would be, because we generally have better control with object caching, because we can decide what groups of cache and keys within that group to exclude. I'm happy to help with that effort and convert this PR to change the filter to use object cache instead.

However, I'm concerned about backwards compatibility - what happens with the transients? Are we simply removing them forever? Won't this be technically a breaking change and something that's usually avoided in the WordPress core?

What do you think?

[oandregal]

However, I'm concerned about backwards compatibility - what happens with the filters? Are we simply removing them forever? Won't this be technically a breaking change and something that's usually avoided in the WordPress core?

Which filters would be affected by updating the transient to the object cache?

[tyxla]

Which filters would be affected by updating the transient to the object cache?

I meant transients, not filters - edited my message.

[oandregal]

Ah, I see. My understanding is that by updating transients to object cache:

• sites with no object cache plugins will still have runtime, non-persistent cache
• sites with object cache plugins will work as before

I don't find this would be considered a breaking change, rather fixing an issue for hosts and also for users by removing a hard-coded expiration for a more flexible mechanism (I've seen issues where people report that front-end styles are not updating upon user changes until a while).

Happy to review. Can we do it in small steps? First for one method, then for the other? I want to make sure this change gets proper testing.

[mmtr]

instead of short-circuiting what core does via a filter, would you think is it worth improving the current state by stop using transients and using the object cache instead?

I think a filter to short-circuit the cache could be useful under some cases. Imagine there is a plugin that hooks into the wp_theme_json_data_user filter to change the user styles based on a dynamic check:

add_filter( 'wp_theme_json_data_user', function( $theme_json ) {
  return my_plugin_check() ? $theme_json->update_with( my_plugin_get_styles() ) : $theme_json;
} );

With the filter, the plugin could easily disable the cache to make sure the wp_theme_json_data_user filter runs (otherwise the cached styles are retrieved which doesn't trigger the wp_theme_json_data_user filter).

add_filter( 'global_styles_enable_cache', function() { 
  return ! my_plugin_check();
} )

The alternative would be to do something like this:

add_action( 'init', function() {
    if ( ! my_plugin_check() ) {
      return;
    }
    add_filter( 'pre_transient_gutenberg_global_styles_' . get_stylesheet(), '__return_null' );
    add_filter( 'pre_set_transient_gutenberg_global_styles_' . get_stylesheet(), '__return_false' );
} );

Hooking into the transient filters (or changing the object cache if we move away from transients) to achieve the same result feels a bit fragile because it's tied to the Gutenberg implementation which might change without further notice.

[tyxla]

I agree with you both. I wonder if it makes sense to have both the filter and to change the transients to object cache then? Won't that get the best of both worlds? WDYT?

[mmtr]

I think a filter to short-circuit the cache could be useful under some cases

I further discussed this with @oandregal privately and he'd like to avoid the filter. He thinks that plugins should be responsible for invalidating the cache when necessary, rather than preventing the cache from being used with a filter.

He agrees though that plugins shouldn't know the cache key to invalidate the cache, so Core/Gutenberg should provide a friendly function to do so.

[oandregal]

Miguel has prepared #45679 I think is very promising and should cover all cache invalidation we need. We only need to fix the automated tests.

[tyxla]

Awesome, thank you both! Closing in favor of #45679 then.