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

Update wp_theme_has_theme_json to use WP_Object_Cache #45543

Merged
merged 15 commits into from
Nov 17, 2022
Merged

Update wp_theme_has_theme_json to use WP_Object_Cache #45543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0739837
Add object caching to wp_theme_has_theme_json
oandregal Nov 4, 2022
6003a43
Add a cache group
oandregal Nov 9, 2022
f58e23b
Store int instead of boolean in the cache
oandregal Nov 9, 2022
20b2d8c
Do not use the cache if WP_DEBUG is true
oandregal Nov 9, 2022
e5b1b41
Update clean cache method to have a single purpose
oandregal Nov 9, 2022
a5e6bdf
Clear cache upon upgrading the active theme
oandregal Nov 9, 2022
57d2df9
Add PHPDoc
oandregal Nov 10, 2022
5473642
Fix lint issues
oandregal Nov 10, 2022
f197534
Remove WP_DEBUG behavior
oandregal Nov 16, 2022
0aa8ce3
Brinch back cache retrieval
oandregal Nov 16, 2022
ee310d7
Update lib/compat/wordpress-6.2/get-global-styles-and-settings.php
oandregal Nov 16, 2022
ffee4e1
Fix lint issue
oandregal Nov 16, 2022
961528f
Update lib/compat/wordpress-6.2/get-global-styles-and-settings.php
oandregal Nov 17, 2022
af3ecb9
Update lib/compat/wordpress-6.2/get-global-styles-and-settings.php
oandregal Nov 17, 2022
5268077
Update lib/compat/wordpress-6.2/get-global-styles-and-settings.php
oandregal Nov 17, 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
11 changes: 3 additions & 8 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,11 +17,6 @@
* @package gutenberg
*/

/**
* Note for backport: we should also remove the existing filters:
*
* > add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver', 'clean_cached_data' ) );
Copy link
Member Author

@oandregal oandregal Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We should not remove these filters when backporting to core, so I've removed the note.

Each public method has its own conditions for invalidating the cache (this one does not require invalidation when a plugin is updated, unlike the settings or styles), so we should have atomic clean operations instead of a single cache clean for them all.

* > add_action( 'start_previewing_theme', array( 'WP_Theme_JSON_Resolver', 'clean_cached_data' ) );
*/
add_action( 'switch_theme', 'wp_theme_clean_theme_json_cached_data' );
add_action( 'start_previewing_theme', 'wp_theme_clean_theme_json_cached_data' );
add_action( 'switch_theme', 'wp_theme_has_theme_json_clean_cache' );
add_action( 'start_previewing_theme', 'wp_theme_has_theme_json_clean_cache' );
add_action( 'upgrader_process_complete', '_wp_theme_has_theme_json_clean_cache_upon_upgrading_active_theme' );
Copy link
Member Author

Choose a reason for hiding this comment

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

Follow-up #45881

66 changes: 48 additions & 18 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 @@ -9,39 +9,69 @@
/**
* Whether a theme or its parent have a theme.json file.
*
* @param boolean $clear_cache Whether the cache should be cleared and theme support recomputed. Default is false.
* The result would be cached via the WP_Object_Cache.
* It can be cleared by calling wp_theme_has_theme_json_clean_cache().
*
* @return boolean
*/
function wp_theme_has_theme_json( $clear_cache = false ) {
static $theme_has_support = null;
function wp_theme_has_theme_json() {
Copy link
Member

Choose a reason for hiding this comment

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

This function should accept a theme as a param. So we can check if none active themes have theme json. Example.
https://github.com/WordPress/wordpress-develop/blob/551c3d6619b7e88742911ca2dd3aed77a71cb0dc/src/wp-includes/rest-api/endpoints/class-wp-rest-themes-controller.php#L368-L374

Copy link
Member Author

Choose a reason for hiding this comment

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

That could be useful, although I'm reluctant to do it in this PR. All the current use cases we have don't need it, so it adds complexity (manage invalidation of stylesheet per theme) without real needs. When/If we need that, we should implement it. It's a good follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with both of you here, but for now I agree more with @oandregal that this can be added in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Here and here are examples where we need to know if another theme that is not the current theme has theme supports json.

We can add this later, but this is something that is required.

$cache_group = 'theme_json';
oandregal marked this conversation as resolved.
Show resolved Hide resolved
$cache_key = 'wp_theme_has_theme_json';
oandregal marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member Author

@oandregal oandregal Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wanted to note here that this deliberately uses the function name as the cache key for a reason: this is expected to be unique in the PHP namespace.

Additionally, unlike other functions, this is prefixed by wp_ and not gutenberg_ as well. If there is ever a need to make this method work differently in the plugin and in core (which requires renaming it to gutenberg_), we should also rename the cache key. Otherwise, the plugin will use the cache contents from core if there is any.

$theme_has_support = wp_cache_get( $cache_key, $cache_group );
oandregal marked this conversation as resolved.
Show resolved Hide resolved

if ( true === $clear_cache ) {
$theme_has_support = null;
}

if ( null !== $theme_has_support ) {
return $theme_has_support;
/**
* $theme_has_support is stored as a int in the cache.
*
* The reason not to store it as a boolean is to avoid working
* with the $found parameter which apparently had some issues in some implementations
* https://developer.wordpress.org/reference/functions/wp_cache_get/
*/
if ( 0 === $theme_has_support || 1 === $theme_has_support ) {
return (bool) $theme_has_support;
}

// Has the own theme a theme.json?
$theme_has_support = is_readable( get_stylesheet_directory() . '/theme.json' );
$theme_has_support = is_readable( get_stylesheet_directory() . '/theme.json' ) ? 1 : 0;
oandregal marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Related #45831


// Look up the parent if the child does not have a theme.json.
if ( ! $theme_has_support ) {
$theme_has_support = is_readable( get_template_directory() . '/theme.json' );
if ( 0 === $theme_has_support ) {
$theme_has_support = is_readable( get_template_directory() . '/theme.json' ) ? 1 : 0;
}

return $theme_has_support;
wp_cache_set( $cache_key, $theme_has_support, $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.

Nit-pick, but we could simplify the code here to

// Check whether the theme or its parent theme have a theme.json.
$theme_has_support = is_readable( get_stylesheet_directory() . '/theme.json' ) || is_readable( get_template_directory() . '/theme.json' );
wp_cache_set( $cache_key, (int) $theme_has_support, $cache_group );

This code will do the same as what you have here. If the first check is true, the second one won't be called so there won't be any wasteful is_readable calls in either approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing I like from the current implementation is that it is really obvious that it stores an integer instead of a boolean. That's a bit more obscured in the concise proposal, and I worry that means it'll become fragile (aka, someone from the future could remove the integer casting inadvertently).


return (bool) $theme_has_support;
}
}

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_clean_theme_json_cached_data' ) ) {
if ( ! function_exists( '_wp_theme_has_theme_json_clean_cache_upon_upgrading_active_theme' ) ) {
/**
* Clean theme.json related cached data.
* Private function to clean the cache used by wp_theme_has_theme_json method.
*
oandregal marked this conversation as resolved.
Show resolved Hide resolved
* 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.
*/
function wp_theme_clean_theme_json_cached_data() {
wp_theme_has_theme_json( true );
WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data();
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();
}
}
}