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

Add WP_Object_Cache to the gutenberg_get_global_settings method #45372

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Add WP_Object_Cache to the gutenberg_get_global_settings method #45372

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion 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 @@ -171,7 +171,15 @@ function gutenberg_get_global_settings( $path = array(), $context = array() ) {
$origin = 'theme';
}

$settings = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( $origin )->get_settings();
$cache_group = 'theme_json';
$cache_key = 'gutenberg_get_global_settings_' . $origin;
$settings = wp_cache_get( $cache_key, $cache_group );

if ( false === $settings || WP_DEBUG ) {
$settings = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( $origin )->get_settings();
Copy link
Member

Choose a reason for hiding this comment

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

Consider these lines
https://github.com/WordPress/wordpress-develop/blob/bb948c5f6d462dc43483f198d36e9b0948d2e5f1/src/wp-includes/class-wp-theme-json-resolver.php#L297
get_default_block_editor_settings has calls to get_options for image sizes, is_rtl related to langaguge setting of the user, get_default_block_categories that is filterable, get_allowed_mime_types filterable, and get_theme_support that can removed. How are all these cases invalidated?

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 is great feedback:

  • get_theme_support, get_default_block_categories, get_allowed_mime_types are filtered by a consumer: either themes or plugins. We clean the cache when a plugin is activated/deactivated, theme switch, etc.. so these cases are covered.
  • is_rtl: I need to look at this one. We may need to invalidate the cache upon language switches as well.

Copy link
Member

Choose a reason for hiding this comment

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

As noted, updating a plugin could change the code. Also what if someone updates their plugin by uploading it using FTP. Something I have done in the past and is very common.

Is_rtl can be related to the current user, so can not be cached ever. We need a rethink here.

Copy link
Member Author

Choose a reason for hiding this comment

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

When someone updates something via FTP, this code will work perfectly in the next request if they don't use a persistent cache plugin. If they do, they can always clear the cache.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but currently, there are no other parts of core, that require you to flush caches on FTP uploads. Changing this behavior, would be a breaking change, which is something we need avoid at all costs.

Copy link
Member

Choose a reason for hiding this comment

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

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-theme.php?rev=54236#L745

This is different theme by default are not a persistent cache group. See

Copy link
Member

Choose a reason for hiding this comment

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

core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-theme.php?rev=54236#L745

This is different theme by default are not a persistent cache group. See

Here we are dealing with site styles that are related to what a theme does. In that case, shouldn't we just follow the same approach and make sure this cache is not in a persistent cache group?

Copy link
Member

Choose a reason for hiding this comment

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

is_rtl related to langaguge setting of the user, get_default_block_categories that is filterable, get_allowed_mime_types filterable

Although get_default_block_editor_settings gets this information, it is not used at all for the purpose of computing theme. json related information, so even if filters change this, it does not affect the output of the cached functions.

Copy link
Member

Choose a reason for hiding this comment

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

Although get_default_block_editor_settings gets this information, it is not used at all for the purpose of computing theme. json related information, so even if filters change this, it does not affect the output of the cached functions.

Wow, I never followed that code path. Calling get_default_block_editor_settings is wasteful then. We should fix that ASAP. To that end I have created #46112.

Copy link
Member

Choose a reason for hiding this comment

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

In this PR, I worked on adding caching to theme_json_resolver, as had core unit tests fail. WordPress/wordpress-develop#3624.

This was because of add / removing theme supports. I don't think we should merge this until we have solution to this problem.

wp_cache_set( $cache_key, $settings, $cache_group );
}

return _wp_array_get( $settings, $path, $settings );
}

Expand All @@ -183,6 +191,7 @@ function gutenberg_get_global_settings( $path = array(), $context = array() ) {
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_custom', 'theme_json' );
wp_cache_delete( 'gutenberg_get_global_settings_theme', 'theme_json' );
WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data();
}
Expand All @@ -192,6 +201,7 @@ function _gutenberg_clean_theme_json_caches() {
* The data stored under this cache group:
*
* - wp_theme_has_theme_json
* - gutenberg_get_global_settings
* - gutenberg_get_global_stylesheet
*
* There is some hooks consumers can use to modify parts
Expand Down