Skip to content
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.

Sign up for GitHub

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

Make theme.json object caches non persistent #46150

Merged
merged 9 commits into from
Nov 30, 2022
Merged

Make theme.json object caches non persistent #46150

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5a9921d
Make theme_json group non persistent
oandregal Nov 29, 2022
6802de1
Sort hooks for readibility
oandregal Nov 29, 2022
b93314e
No need to delete cache keys upon upgrading if it is non persistent
oandregal Nov 29, 2022
2f5b23a
No need to delete cache keys upon plugin de/activation if it is non p…
oandregal Nov 29, 2022
6616e9c
No need to delete cache keys upon post saving if it is non persistent
oandregal Nov 29, 2022
cf99597
Remove filters for clearing WP_Theme_JSON_Resolver cache
oandregal Nov 29, 2022
83af27a
Deprecate old methods to clear the cache
oandregal Nov 29, 2022
0dd6a78
Remove method as it was introduced in this same release
oandregal Nov 29, 2022
f654f6f
Use plugins_loaded instead of init as hook
oandregal Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions lib/compat/wordpress-6.2/class-wp-theme-json-resolver-6-2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,31 +32,6 @@ 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 ) {
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();
}
}

/**
* Returns the data merged from multiple origins.
*
Expand Down
21 changes: 6 additions & 15 deletions lib/compat/wordpress-6.2/default-filters.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,9 @@
* @package gutenberg
*/

add_action( 'switch_theme', 'wp_theme_has_theme_json_clean_cache' );
add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
add_action( 'start_previewing_theme', 'wp_theme_has_theme_json_clean_cache' );
add_action( 'start_previewing_theme', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
add_action( 'upgrader_process_complete', '_wp_theme_has_theme_json_clean_cache_upon_upgrading_active_theme', 10, 2 );
add_action( 'save_post_wp_global_styles', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
add_action( 'activated_plugin', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
add_action( 'deactivated_plugin', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
add_action( 'upgrader_process_complete', array( 'WP_Theme_JSON_Resolver_Gutenberg', '_clean_cached_data_upon_upgrading', 10, 2 ) );
oandregal marked this conversation as resolved.
Show resolved Hide resolved
add_action( 'save_post_wp_global_styles', 'gutenberg_get_global_stylesheet_clean_cache' );
add_action( 'switch_theme', 'gutenberg_get_global_stylesheet_clean_cache' );
add_action( 'start_previewing_theme', 'gutenberg_get_global_stylesheet_clean_cache' );
add_action( 'activated_plugin', 'gutenberg_get_global_stylesheet_clean_cache' );
add_action( 'deactivated_plugin', 'gutenberg_get_global_stylesheet_clean_cache' );
add_action( 'upgrader_process_complete', '_gutenberg_get_global_stylesheet_clean_cache_upon_upgrading', 10, 2 );
/**
* When backporting to core, the existing filters hooked to WP_Theme_JSON_Resolver::clean_cached_data()
* need to be removed.
*/
add_action( 'start_previewing_theme', '_gutenberg_clean_theme_json_caches' );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only left the start_previewing_theme and switch_theme events as they are necessary for certain flows (preview themes, theme directory) that may access the theme data before they are dispatched.

These new events were introduced in 14.7 (stylesheet PR) when we aimed to make this persistent and can be removed: activated_plugin, deactivated_plugin, upgrader_process_complete.

With this change, all caches need the same invalidation logic, so I've consolidated it into a single place as per feedback at #45372 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we merge #45955, it will mean that no cache invalidation will be needed for start_previewing_theme / switch_theme, as the act of changing / filter the stylesheet option, will cache the cache key.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It requires more changes than that: data cached by wp_theme_has_theme_json, gutenberg_get_global_styles, and WP_Theme_JSON_Resolver all needs to be cached by stylesheet. If/When we do that, we can remove these hooks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching themes, mean we do not have cache to clear, as each theme would have it's own cache.

Copy link
Member

@aristath aristath Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching themes, mean we do not have cache to clear, as each theme would have it's own cache.

I agree. However... clearing the cache won't be a bad thing since we'll be saving some memory 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that to be able to remove these hooks, we need we make changes to those three places. It cannot be done in this PR or #45955 that only addresses wp_theme_has_theme_json as far as I could see.

add_action( 'switch_theme', '_gutenberg_clean_theme_json_caches' );
94 changes: 44 additions & 50 deletions lib/compat/wordpress-6.2/get-global-styles-and-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,32 +51,11 @@ function wp_theme_has_theme_json() {
if ( ! function_exists( 'wp_theme_has_theme_json_clean_cache' ) ) {
/**
* Function to clean the cache used by wp_theme_has_theme_json method.
*/
function wp_theme_has_theme_json_clean_cache() {
wp_cache_delete( 'wp_theme_has_theme_json', 'theme_json' );
}
}

if ( ! function_exists( '_wp_theme_has_theme_json_clean_cache_upon_upgrading_active_theme' ) ) {
/**
* Private function to clean the cache used by wp_theme_has_theme_json method.
*
* It is hooked into the `upgrader_process_complete` action.
*
* @see default-filters.php
*
* @param WP_Upgrader $upgrader Instance of WP_Upgrader class.
* @param array $options Metadata that identifies the data that is updated.
* Not to backport to core. Delete it instead.
*/
function _wp_theme_has_theme_json_clean_cache_upon_upgrading_active_theme( $upgrader, $options ) {
// The cache only needs cleaning when the active theme was updated.
if (
'update' === $options['action'] &&
'theme' === $options['type'] &&
( isset( $options['themes'][ get_stylesheet() ] ) || isset( $options['themes'][ get_template() ] ) )
) {
wp_theme_has_theme_json_clean_cache();
}
function wp_theme_has_theme_json_clean_cache() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was introduced in 14.5 at #45543 so I'd rather deprecate it to be safe.

_deprecated_function( __METHOD__, '14.7' );
}
}

Expand Down Expand Up @@ -159,34 +138,11 @@ function gutenberg_get_global_stylesheet( $types = array() ) {

/**
* Clean the cache used by the `gutenberg_get_global_stylesheet` function.
*/
function gutenberg_get_global_stylesheet_clean_cache() {
Copy link
Member Author

@oandregal oandregal Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was introduced at #45679 and is slated for the 14.7 release. I've marked this PR for that release as well, so we can remove it. If this does not make it to the 14.7 we should deprecate it instead.

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.
* Not to backport to core. Delete it instead.
*/
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();
}
function gutenberg_get_global_stylesheet_clean_cache() {
_deprecated_function( __METHOD__, '14.7' );
}

/**
Expand Down Expand Up @@ -227,3 +183,41 @@ function gutenberg_get_global_settings( $path = array(), $context = array() ) {
$settings = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( $origin )->get_settings();
return _wp_array_get( $settings, $path, $settings );
}

/**
* Private function to clean the caches used by gutenberg_get_global_settings method.
*
* @access private
*/
function _gutenberg_clean_theme_json_caches() {
wp_cache_delete( 'wp_theme_has_theme_json', 'theme_json' );
wp_cache_delete( 'gutenberg_get_global_stylesheet', 'theme_json' );
wp_cache_delete( 'gutenberg_get_global_settings_theme', 'theme_json' );
WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data();
}

/**
* Tell the cache mechanisms not to persist theme.json data across requests.
* The data stored under this cache group:
*
* - wp_theme_has_theme_json
* - gutenberg_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
*/
function _gutenberg_add_non_persistent_theme_json_cache_group() {
wp_cache_add_non_persistent_groups( 'theme_json' );
}
add_action( 'init', '_gutenberg_add_non_persistent_theme_json_cache_group' );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what hook would be best to set this group to non-persistent. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe plugins_loaded

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 At that point, any plugin that implement persistent cache should be loaded, hence calling this method should be effective. Sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f654f6f