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 |
|---|---|---|
|
|
@@ -79,3 +79,122 @@ function _wp_theme_has_theme_json_clean_cache_upon_upgrading_active_theme( $upgr | |
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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() ) { | ||
| /** | ||
| * Filters whether the cached global stylesheet can be used. | ||
| * | ||
| * @param boolean $can_use_cached Whether the cached global stylesheet can be used. | ||
| */ | ||
| $can_use_cached = apply_filters( | ||
| 'wp_get_global_stylesheet_can_use_cache', | ||
|
There was a problem hiding this comment. 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 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. There was a problem hiding this comment. The use case for the filter is documented in this other conversation (it's unrelated to development/debug) #45679 (comment) There was a problem hiding this comment. @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? There was a problem hiding this comment. I think we should add this filter later, I am not sure about it either. There was a problem hiding this comment.
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 Without a filter that short-circuits the cache, a plugin that hooks into 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. There was a problem hiding this comment. 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
That's the why, the product need. I hope this provides a clear context. Now, discussing implementations to support this product need:
Is there another alternative we should consider to be able to support this use case? There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. Thanks for the feedback @felixarntz. I see now how requiring plugin developers to disable the cache to be able to hook into the |
||
| ( empty( $types ) ) && | ||
|
There was a problem hiding this comment. |
||
| // Ignore cache when `WP_DEBUG` is enabled, so it doesn't interfere with the theme developers workflow. | ||
| ( ! defined( 'WP_DEBUG' ) || ! WP_DEBUG ) | ||
mmtr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ); | ||
| $cache_key = 'gutenberg_get_global_stylesheet'; | ||
| $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; | ||
| } | ||
|
|
||
| /** | ||
| * Clean the cache used by the `gutenberg_get_global_stylesheet` function. | ||
| */ | ||
| function gutenberg_get_global_stylesheet_clean_cache() { | ||
| wp_cache_delete( 'gutenberg_get_global_stylesheet', 'theme_json' ); | ||
| } | ||
|
|
||
| /** | ||
| * Private function to clean the cache used by the `gutenberg_get_global_stylesheet` function after an upgrade. | ||
| * | ||
| * It is hooked into the `upgrader_process_complete` action. | ||
| * | ||
| * @see default-filters.php | ||
| * | ||
| * @param WP_Upgrader $upgrader WP_Upgrader instance. | ||
| * @param array $options Array of bulk item update data. | ||
| */ | ||
| function _gutenberg_get_global_stylesheet_clean_cache_upon_upgrading( $upgrader, $options ) { | ||
| if ( 'update' !== $options['action'] ) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| 'core' === $options['type'] || | ||
| 'plugin' === $options['type'] || | ||
| // Clean cache only if the active theme was updated. | ||
| ( 'theme' === $options['type'] && ( isset( $options['themes'][ get_stylesheet() ] ) || isset( $options['themes'][ get_template() ] ) ) ) | ||
| ) { | ||
| gutenberg_get_global_stylesheet_clean_cache(); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| <?php | ||
| /** | ||
| * Tests wp_get_global_stylesheet(). | ||
| * | ||
| * @package Gutenberg | ||
| */ | ||
|
|
||
| class WP_Get_Global_Stylesheet_Test extends WP_UnitTestCase { | ||
|
|
||
| /** | ||
| * Administrator ID. | ||
| * | ||
| * @var int | ||
| */ | ||
| protected static $administrator_id; | ||
|
|
||
| /** | ||
| * Theme root directory. | ||
| * | ||
| * @var string | ||
| */ | ||
| private $theme_root; | ||
|
|
||
| /** | ||
| * Original theme directory. | ||
| * | ||
| * @var string | ||
| */ | ||
| private $orig_theme_dir; | ||
|
|
||
| public static function set_up_before_class() { | ||
| parent::set_up_before_class(); | ||
|
|
||
| self::$administrator_id = self::factory()->user->create( | ||
| array( | ||
| 'role' => 'administrator', | ||
| 'user_email' => '[email protected]', | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public function set_up() { | ||
| parent::set_up(); | ||
|
|
||
| $this->orig_theme_dir = $GLOBALS['wp_theme_directories']; | ||
| $this->theme_root = realpath( DIR_TESTDATA . '/themedir1' ); | ||
|
|
||
| // /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 ); | ||
|
|
||
| // Set up the new 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' ) ); | ||
|
|
||
| // Clear caches. | ||
| wp_clean_themes_cache(); | ||
| unset( $GLOBALS['wp_themes'] ); | ||
| } | ||
|
|
||
| public function tear_down() { | ||
| $GLOBALS['wp_theme_directories'] = $this->orig_theme_dir; | ||
|
|
||
| // Clear up the filters to modify the theme root. | ||
| remove_filter( 'theme_root', array( $this, 'filter_set_theme_root' ) ); | ||
| remove_filter( 'stylesheet_root', array( $this, 'filter_set_theme_root' ) ); | ||
| remove_filter( 'template_root', array( $this, 'filter_set_theme_root' ) ); | ||
|
|
||
| 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_user_cpt_change_invalidates_cached_stylesheet() { | ||
| add_filter( 'wp_get_global_stylesheet_can_use_cache', '__return_true' ); | ||
| switch_theme( 'block-theme' ); | ||
| wp_set_current_user( self::$administrator_id ); | ||
|
There was a problem hiding this comment. Documenting something Miguel fixed for posterity: we need to make the petition as an admin, otherwise |
||
|
|
||
| $styles = gutenberg_get_global_stylesheet(); | ||
| $this->assertStringNotContainsString( 'background-color: hotpink;', $styles ); | ||
|
|
||
| $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( 'background-color: hotpink;', $styles ); | ||
| remove_filter( 'wp_get_global_stylesheet_can_use_cache', '__return_true' ); | ||
| } | ||
| } | ||
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!