-
Notifications
You must be signed in to change notification settings - Fork 3.2k
wp_get_global_stylesheet: substitute 1-minute transient by non-persistent object cache
#3712
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
Changes from 1 commit
c23cc4c
f84f94c
8707830
d39b5e4
95fd03a
6db5380
9c82e0b
a0ae741
45536da
af0cb51
5477120
d9c0909
8d3fcb4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -83,18 +83,12 @@ function wp_get_global_styles( $path = array(), $context = array() ) { | |||
| * @return string Stylesheet. | ||||
| */ | ||||
| function wp_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 ) ) && | ||||
| ( ! defined( 'WP_DEBUG' ) || ! WP_DEBUG ) && | ||||
| ( ! defined( 'SCRIPT_DEBUG' ) || ! SCRIPT_DEBUG ) && | ||||
| ( ! defined( 'REST_REQUEST' ) || ! REST_REQUEST ) && | ||||
| ! is_admin() | ||||
| ); | ||||
| $transient_name = 'global_styles_' . get_stylesheet(); | ||||
| // Ignore cache when `WP_DEBUG` is enabled, so it doesn't interfere with the theme developers workflow. | ||||
| $can_use_cached = empty( $types ) && ! WP_DEBUG; | ||||
| $cache_key = 'wp_get_global_stylesheet'; | ||||
felixarntz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||
| $cache_group = 'theme_json'; | ||||
| if ( $can_use_cached ) { | ||||
| $cached = get_transient( $transient_name ); | ||||
| $cached = wp_cache_get( $cache_key, $cache_group ); | ||||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| if ( $cached ) { | ||||
| return $cached; | ||||
| } | ||||
|
|
@@ -148,15 +142,10 @@ function wp_get_global_stylesheet( $types = array() ) { | |||
| } | ||||
| $styles_rest = $tree->get_stylesheet( $types, $origins ); | ||||
| } | ||||
|
|
||||
| $stylesheet = $styles_variables . $styles_rest; | ||||
|
|
||||
| if ( $can_use_cached ) { | ||||
| // Cache for a minute. | ||||
| // This cache doesn't need to be any longer, we only want to avoid spikes on high-traffic sites. | ||||
| set_transient( $transient_name, $stylesheet, MINUTE_IN_SECONDS ); | ||||
| wp_cache_set( $cache_key, $stylesheet, $cache_group ); | ||||
| } | ||||
|
|
||||
| return $stylesheet; | ||||
| } | ||||
|
|
||||
|
|
@@ -283,3 +272,31 @@ function wp_theme_has_theme_json() { | |||
| function wp_clean_theme_json_cache() { | ||||
| WP_Theme_JSON_Resolver::clean_cached_data(); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Tell the cache mechanisms not to persist theme.json data across requests. | ||||
| * The data stored under this cache group: | ||||
| * | ||||
| * - wp_get_global_stylesheet | ||||
| * | ||||
| * There is some hooks consumers can use to modify parts | ||||
| * of the theme.json logic. | ||||
| * See https://make.wordpress.org/core/2022/10/10/filters-for-theme-json-data/ | ||||
| * | ||||
| * The rationale to make this cache group non persistent is to make sure derived data | ||||
| * from theme.json is always fresh from the potential modifications done via hooks | ||||
| * that can use dynamic data (modify the stylesheet depending on some option, | ||||
| * or settings depending on user permissions, etc.). | ||||
| * | ||||
| * A different alternative considered was to invalidate the cache upon certain | ||||
| * events such as options add/update/delete, user meta, etc. | ||||
| * It was judged not enough, hence this approach. | ||||
| * See https://github.com/WordPress/gutenberg/pull/45372 | ||||
| * | ||||
| * @since 6.1.2 | ||||
felixarntz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
hellofromtonya marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||
| * @access private | ||||
| */ | ||||
| function _wp_add_non_persistent_theme_json_cache_group() { | ||||
| wp_cache_add_non_persistent_groups( 'theme_json' ); | ||||
|
||||
| wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) ); |
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 see wp_cache_add_non_persistent_groups is used in a few places:
I'm not sure where is best. Why should we use load.php instead of having a dedicated place?
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 further. It looks like there is no a single place for this kind of thing. If so, I'd rather try to collocate wp_cache_add_non_persistent_groups( 'theme_json' ) with the actual code that uses the cache.
The rationale for doing so is that if we move this to a different part of the codebase it becomes "out of sight, out of mind": we need to resort to human memory to remember, which is far from ideal.
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.
Also, for cross-reference and extra clarity. I'm not sure where this should be hooked. Using plugins_loaded was the way we came up with at WordPress/gutenberg#46150 (comment) Is there a better place?
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.
Hmm, I wonder if it's better to move
add_action( 'plugins_loaded', '_wp_add_non_persistent_theme_json_cache_group' );
to src/wp-includes/default-filters.php.
System-wide hooks/actions are supposed to go into that file.
At the same time, _wp_add_non_persistent_theme_json_cache_group() can stay in global-styles-and-settings.php.
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.
Moved the hook to default-filters.php in ff7fd1e I had missed that, thanks for bringing it up!
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'm not sure where this should be hooked. Using plugins_loaded was the way we came up with at WordPress/gutenberg#46150 (comment) Is there a better place?
Mostly depends on whether plugins may need it right away (while loading).
The other question is: does it make sense for this to be removable by plugins? As far as I understand it, no.
As @spacedmonkey points out above the similar wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) ); is hard-coded and happens earlier. Thinking it makes sense for wp_cache_add_non_persistent_groups( 'theme_json' ); to be there too.
Then the hook and the helper function _wp_add_non_persistent_theme_json_cache_group() won't be needed and the only change to the code would be to add theme_json to the first one:
wp_cache_add_non_persistent_groups( array( 'counts', 'plugins', 'theme_json' ) );
Or perhaps even better would be to add the separate wp_cache_add_non_persistent_groups( 'theme_json' ); at the bottom of wp_start_object_cache() and add the nice explanation from the docblock as a comment.
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.
The other question is: does it make sense for this to be removable by plugins? As far as I understand it, no.
That's a good point. 🤔
Is there a way for it to happen earlier but still be collocated in this file?
Alternatively, if it is moved elsewhere, how can we make sure people reading this code has clarity about it being non-persistent? I can only think of adding comments in every function that uses that group.
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.
Is there a way for it to happen earlier but still be collocated in this file?
Don't think so unless there is a function that is called on including that file (which is generally a bad idea). Also not sure if that is particularly important. Thinking that the best option here is to add it at the bottom of wp_start_object_cache() which seems to be the best place for such settings, and add the explanation as inline comment.
Many modern IDEs (probably all) would show cross references. Having a good explanation of what is being set and why, as in the current docblock, would be more than enough imho. It is easy to find.
can only think of adding comments in every function that uses that group
Not sure that's needed every time the cache group is used. If you're worried that somebody may miss it, perhaps adding an inline comment (pointing to where it is set) the first time it is used in global-styles-and-settings.php would make it even easier.
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.
Done at 45969f1
Uh oh!
There was an error while loading. Please reload this page.