Global Styles: Update gutenberg_get_global_stylesheet to use WP_Object_Cache
#45679
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,88 +54,3 @@ function ( $item ) { | |
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,3 +45,108 @@ function wp_theme_clean_theme_json_cached_data() { | |
| WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the stylesheet resulting of merging core, theme, and user data. | ||
| * | ||
| * @param array $types Types of styles to load. Optional. | ||
| * It accepts 'variables', 'styles', 'presets' as values. | ||
| * If empty, it'll load all for themes with theme.json support | ||
| * and only [ 'variables', 'presets' ] for themes without theme.json support. | ||
| * | ||
| * @return string Stylesheet. | ||
| */ | ||
| function gutenberg_get_global_stylesheet( $types = array() ) { | ||
| // Return cached value if it can be used and exists. | ||
| // It's cached by theme to make sure that theme switching clears the cache. | ||
| $can_use_cached = ( | ||
| ( empty( $types ) ) && | ||
|
There was a problem hiding this comment. |
||
| ( ! defined( 'WP_DEBUG' ) || ! WP_DEBUG ) && | ||
| ( ! defined( 'SCRIPT_DEBUG' ) || ! SCRIPT_DEBUG ) && | ||
|
There was a problem hiding this comment. 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 😅 There was a problem hiding this comment. Let's remove this, as it hides caching behaviour changes on local dev, with it not great for unit testing and local profiling. There was a problem hiding this comment. 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 There was a problem hiding this comment.
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. There was a problem hiding this comment. In a845e85 I simplified the conditions that allows the cache to be used by removing the I kept the I also kept the Furthermore, there is now a filter that can override the default cache behavior when needed such as in the unit tests. There was a problem hiding this comment. (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 cacheHowever, 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:
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. |
||
| ( ! defined( 'REST_REQUEST' ) || ! REST_REQUEST ) && | ||
| ! is_admin() | ||
| ); | ||
| $cache_key = 'gutenberg_get_global_stylesheet'; | ||
mmtr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| $cache_group = 'theme_json'; | ||
| if ( $can_use_cached ) { | ||
| $cached = wp_cache_get( $cache_key, $cache_group ); | ||
| if ( $cached ) { | ||
| return $cached; | ||
| } | ||
| } | ||
| $tree = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data(); | ||
| $supports_theme_json = wp_theme_has_theme_json(); | ||
| if ( empty( $types ) && ! $supports_theme_json ) { | ||
| $types = array( 'variables', 'presets', 'base-layout-styles' ); | ||
| } elseif ( empty( $types ) ) { | ||
| $types = array( 'variables', 'styles', 'presets' ); | ||
| } | ||
|
|
||
| /* | ||
| * If variables are part of the stylesheet, | ||
| * we add them. | ||
| * | ||
| * This is so themes without a theme.json still work as before 5.9: | ||
| * they can override the default presets. | ||
| * See https://core.trac.wordpress.org/ticket/54782 | ||
| */ | ||
| $styles_variables = ''; | ||
| if ( in_array( 'variables', $types, true ) ) { | ||
| /* | ||
| * We only use the default, theme, and custom origins. | ||
| * This is because styles for blocks origin are added | ||
| * at a later phase (render cycle) so we only render the ones in use. | ||
| * @see wp_add_global_styles_for_blocks | ||
| */ | ||
| $origins = array( 'default', 'theme', 'custom' ); | ||
| $styles_variables = $tree->get_stylesheet( array( 'variables' ), $origins ); | ||
| $types = array_diff( $types, array( 'variables' ) ); | ||
| } | ||
|
|
||
| /* | ||
| * For the remaining types (presets, styles), we do consider origins: | ||
| * | ||
| * - themes without theme.json: only the classes for the presets defined by core | ||
| * - themes with theme.json: the presets and styles classes, both from core and the theme | ||
| */ | ||
| $styles_rest = ''; | ||
| if ( ! empty( $types ) ) { | ||
| /* | ||
| * We only use the default, theme, and custom origins. | ||
| * This is because styles for blocks origin are added | ||
| * at a later phase (render cycle) so we only render the ones in use. | ||
| * @see wp_add_global_styles_for_blocks | ||
| */ | ||
| $origins = array( 'default', 'theme', 'custom' ); | ||
| if ( ! $supports_theme_json ) { | ||
| $origins = array( 'default' ); | ||
| } | ||
| $styles_rest = $tree->get_stylesheet( $types, $origins ); | ||
| } | ||
| $stylesheet = $styles_variables . $styles_rest; | ||
| if ( $can_use_cached ) { | ||
| wp_cache_set( $cache_key, $stylesheet, $cache_group ); | ||
| } | ||
| return $stylesheet; | ||
| } | ||
|
|
||
| /** | ||
| * Invalidate the cached stylesheet. | ||
| */ | ||
| function gutenberg_get_global_stylesheet_clean_cache() { | ||
| wp_cache_delete( 'gutenberg_get_global_stylesheet', 'theme_json' ); | ||
| } | ||
|
|
||
| function _gutenberg_get_global_stylesheet_clean_cache_upon_upgrading( $upgrader, $options ) { | ||
| if ( 'update' !== $options['action'] ) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| 'core' === $options['type'] || | ||
| 'plugin' === $options['type'] || | ||
| ( 'theme' === $options['type'] && array_key_exists( get_stylesheet(), $options['themes'] ) ) | ||
| ) { | ||
| gutenberg_clean_cached_stylesheet(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| <?php | ||
| /** | ||
| * Tests wp_get_global_stylesheet(). | ||
| * | ||
| * @package Gutenberg | ||
| */ | ||
|
|
||
| class WP_Get_Global_Stylesheet_Test extends WP_UnitTestCase { | ||
|
|
||
| public function set_up() { | ||
| parent::set_up(); | ||
| $this->theme_root = realpath( __DIR__ . '/data/themedir1' ); | ||
|
|
||
| $this->orig_theme_dir = $GLOBALS['wp_theme_directories']; | ||
|
|
||
| // /themes is necessary as theme.php functions assume /themes is the root if there is only one root. | ||
| $GLOBALS['wp_theme_directories'] = array( WP_CONTENT_DIR . '/themes', $this->theme_root ); | ||
|
|
||
| add_filter( 'theme_root', array( $this, 'filter_set_theme_root' ) ); | ||
| add_filter( 'stylesheet_root', array( $this, 'filter_set_theme_root' ) ); | ||
| add_filter( 'template_root', array( $this, 'filter_set_theme_root' ) ); | ||
| $this->queries = array(); | ||
| // Clear caches. | ||
| wp_clean_themes_cache(); | ||
| unset( $GLOBALS['wp_themes'] ); | ||
| } | ||
|
|
||
| public function tear_down() { | ||
| $GLOBALS['wp_theme_directories'] = $this->orig_theme_dir; | ||
| wp_clean_themes_cache(); | ||
| unset( $GLOBALS['wp_themes'] ); | ||
| parent::tear_down(); | ||
| } | ||
|
|
||
| public function filter_set_theme_root() { | ||
| return $this->theme_root; | ||
| } | ||
|
|
||
| public function test_global_styles_changes_invalidates_cache() { | ||
| switch_theme( 'block-theme' ); | ||
|
|
||
| $user_cpt = WP_Theme_JSON_Resolver_Gutenberg::get_user_data_from_wp_global_styles( wp_get_theme(), true ); | ||
|
|
||
| $config = json_decode( $user_cpt['post_content'], true ); | ||
| $config['styles']['color']['background'] = 'hotpink'; | ||
| $user_cpt['post_content'] = wp_json_encode( $config ); | ||
|
|
||
| wp_update_post( $user_cpt, true, false ); | ||
|
|
||
| $styles = gutenberg_get_global_stylesheet(); | ||
mmtr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| $this->assertStringContainsString( '.button{background-color: hotpink;}', $styles ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is changing:
empty( $types ).WP_DEBUGcheck.SCRIPT_DEBUG.WP_DEBUGshould be enough.is_admin. I don't know why we started addingis_adminin a few places (in this function, in the SVG filters cache, and webfonts). From the cache management point of view, the way it works now is that admin users won't use the cache, no matter the other conditions. I don't see a reason why, other than perhaps circumventing the issues introduced by the transient (styles were not immediately applied in the front-end). I think it's safe to remove.REST_REQUEST. I don't know why this logic was present and why we cannot cache REST requests. After some searching, I found the PR that first introduced this logic. The SVG and webfonts PRs came later, so they probably followed what they saw. I asked there for more context.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a response from Ari: he says the rest request check was probably for the editor (it was many moons ago, so it's hard for anyone to remember exactly). Going to investigate this a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only know see places in which we use a REST Request with this data:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I will test it and report back 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just tested these changes and the mobile endpoint works correctly ✅ Thanks for the ping!