-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global Styles: Update gutenberg_get_global_stylesheet
to use WP_Object_Cache
#45679
Changes from all commits
fad27ea
0a23b43
714819b
4f36f72
0f80d71
a845e85
6014519
5acfd40
4eaeb71
e8a32cb
6931aa8
a2b9d6f
2acbafd
30361b5
b38b5b8
0a371d4
18cd94f
8e2ffb8
18f1443
4f1a1b8
795045a
c9df6b4
88c1e33
9dcdde7
9321aca
8f642a4
2b16326
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,4 +32,29 @@ public static function theme_has_support() { | |
return wp_theme_has_theme_json(); | ||
} | ||
|
||
/** | ||
* Private method to clean the cached data 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. | ||
*/ | ||
public static function _clean_cached_data_upon_upgrading( $upgrader, $options ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let stop adding static methods to this class. They are not in part of the WordPress coding standards and are harder to test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've done a quick search for this pattern and there are multiple cases where this pattern is used: 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. |
||
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() ] ) ) ) | ||
) { | ||
static::clean_cached_data(); | ||
} | ||
} | ||
|
||
} |
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' => 'administrator@example.com', | ||
) | ||
); | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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_DEBUG
check.SCRIPT_DEBUG
.WP_DEBUG
should be enough.is_admin
. I don't know why we started addingis_admin
in 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!