Skip to content

Update wp_theme_has_theme_json to use WP_Object_Cache #45543

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

Merged
oandregal merged 15 commits into from
Nov 17, 2022
Merged

Update wp_theme_has_theme_json to use WP_Object_Cache #45543

Changes from 1 commit
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
Prev Previous commit
Next Next commit
Add a cache group
  • Loading branch information
@oandregal
oandregal committed Nov 16, 2022
commit 6003a43f44d8b7e4b35788463d8d5063fe6c3d54
7 changes: 4 additions & 3 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 @@ -15,9 +15,10 @@
* @return boolean
*/
function wp_theme_has_theme_json() {
Copy link
Member

@spacedmonkey spacedmonkey Nov 10, 2022

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

@oandregal oandregal Nov 16, 2022

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

@felixarntz felixarntz Nov 16, 2022

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

@spacedmonkey spacedmonkey Nov 17, 2022

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';
$cache_key = 'wp_theme_has_theme_json';
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.

$cache_found = false;
$theme_has_support = wp_cache_get( $cache_key, '', false, $cache_found );
$theme_has_support = wp_cache_get( $cache_key, $cache_group, false, $cache_found );
if ( $cache_found ) {
return $theme_has_support;
}
Expand All @@ -30,7 +31,7 @@ function wp_theme_has_theme_json() {
$theme_has_support = is_readable( get_template_directory() . '/theme.json' );
}

wp_cache_set( $cache_key, $theme_has_support );
wp_cache_set( $cache_key, $theme_has_support, $cache_group );
Copy link
Member

@felixarntz felixarntz Nov 16, 2022

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

@oandregal oandregal Nov 17, 2022

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 $theme_has_support;
}
Expand All @@ -41,7 +42,7 @@ function wp_theme_has_theme_json() {
* Clean theme.json related cached data.
*/
function wp_theme_clean_theme_json_cached_data() {
Copy link
Member Author

@oandregal oandregal Nov 4, 2022

Choose a reason for hiding this comment

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

wp_theme_clean_theme_json_cached_data is now called in the following filters: switch_theme, start_previewing_theme. Do we need any other?

What if a theme is updated and removes or adds a theme.json? How can we flush the cache at that point?

Copy link
Member

@spacedmonkey spacedmonkey Nov 4, 2022

Choose a reason for hiding this comment

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

Cleaning the cache on delete_theme sounds like a good idea.

Copy link
Member

@felixarntz felixarntz Nov 4, 2022

Choose a reason for hiding this comment

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

@oandregal +1, we should definitely delete the cache when the theme is updated.

If you're referring to adding/modifying theme.json during development of a theme for example, I think for that we can't properly handle cache, so we may want to wrap the cache usage here in a condition to only use cache if ! WP_DEBUG.

Copy link
Member Author

@oandregal oandregal Nov 9, 2022

Choose a reason for hiding this comment

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

to only use cache if ! WP_DEBUG

Nice, I'm going to implement this.

I've seen something similar in wp_get_global_stylesheet (see), though it also includes SCRIPT_DEBUG and other stuff. I'm not really sure why it was added. It sounds more sensible to just use WP_DEBUG.

Copy link
Member Author

@oandregal oandregal Nov 9, 2022

Choose a reason for hiding this comment

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

Cleaned the cache upon upgrading the theme at 13510c7

Did not added the same for the deleted_theme action because users cannot delete the active theme (manually or via the wp-cli).

wp_cache_delete( 'wp_theme_has_theme_json' );
wp_cache_delete( 'wp_theme_has_theme_json', 'theme_json' );
WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data();
}
}