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

Jump to bottom

Allow for passing stylesheet to wp_theme_has_theme_json. #45955

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Allow for passing stylesheet to wp_theme_has_theme_json. #45955

Show file tree
Hide file tree
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: 6 additions & 6 deletions lib/class-wp-theme-json-resolver-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,9 @@ public static function get_theme_data( $deprecated = array(), $options = array()
$options = wp_parse_args( $options, array( 'with_supports' => true ) );

if ( null === static::$theme || ! static::has_same_registered_blocks( 'theme' ) ) {
$theme_json_file = static::get_file_path_from_theme( 'theme.json' );
$wp_theme = wp_get_theme();
if ( '' !== $theme_json_file ) {
$wp_theme = wp_get_theme();
if ( wp_theme_has_theme_json( $wp_theme->get_stylesheet() ) ) {
$theme_json_file = static::get_file_path_from_theme( 'theme.json' );
$theme_json_data = static::read_json_file( $theme_json_file );
$theme_json_data = static::translate( $theme_json_data, $wp_theme->get( 'TextDomain' ) );
} else {
Expand All @@ -262,8 +262,8 @@ public static function get_theme_data( $deprecated = array(), $options = array()

if ( $wp_theme->parent() ) {
// Get parent theme.json.
$parent_theme_json_file = static::get_file_path_from_theme( 'theme.json', true );
if ( '' !== $parent_theme_json_file ) {
if ( wp_theme_has_theme_json( $wp_theme->parent()->get_stylesheet() ) ) {
$parent_theme_json_file = static::get_file_path_from_theme( 'theme.json', true );
$parent_theme_json_data = static::read_json_file( $parent_theme_json_file );
$parent_theme_json_data = static::translate( $parent_theme_json_data, $wp_theme->parent()->get( 'TextDomain' ) );
// BEGIN OF EXPERIMENTAL CODE. Not to backport to core.
Expand Down Expand Up @@ -422,7 +422,7 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post
* theme, the extra condition for whether $theme is the active theme is
* present here.
*/
if ( $theme->get_stylesheet() === get_stylesheet() && ! wp_theme_has_theme_json() ) {
if ( ! wp_theme_has_theme_json( $theme->get_stylesheet() ) ) {
return array();
}

Expand Down
38 changes: 26 additions & 12 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 @@ -12,11 +12,16 @@
* The result would be cached via the WP_Object_Cache.
* It can be cleared by calling wp_theme_has_theme_json_clean_cache().
*
* @param string $stylesheet Directory name for the theme. Optional. Defaults to current theme.
*
* @return boolean
*/
function wp_theme_has_theme_json() {
function wp_theme_has_theme_json( $stylesheet = '' ) {
$cache_group = 'theme_json';
$cache_key = 'wp_theme_has_theme_json';
if ( empty( $stylesheet ) ) {
$stylesheet = get_stylesheet();
}
$cache_key = sprintf( 'wp_theme_has_theme_json_%s', $stylesheet );
Copy link
Member

Choose a reason for hiding this comment

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

I see two issues with caching by theme:

  • Code complexity: what are the benefits of having cache keys per theme when we only need one at a time?
  • I don't see a way to clean the cache key for the previous theme upon a start_previewing_event. It gives the stylesheet of the theme being previewed, but at that point we need to clear the cache key of the previous theme, not the one being previewed. How can we do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Complexity is not really a good argument not to do something. Just becuase it is hard, does not mean it not worthwhile. I have done the hard work to make this work, let's go ahead here.

Good point about start_previewing_event. This cache clearing can be removed, as now the cache key will be different once you start previewing, which is pretty neat. I am not against removing this all together, but I kept, just in case.

Copy link
Member

Choose a reason for hiding this comment

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

Complexity is not really a good argument not to do something. Just becuase it is hard, does not mean it not worthwhile. I have done the hard work to make this work, let's go ahead here.

My point is not about doing something that is hard. It is about not doing something complex when it can be simple.

This approach is more complex in terms of code and more demanding in terms of memory (we store cache keys we don't need instead of one). The direction I shared a while ago and summarized here again is simpler and brings bigger performance gains. Would you be willing to give it a try and reevaluate when is it shipped?

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 don't understand this feedback at all. For most requests, it will only store one cache key, like it does currently. There difference here, is this structure all for multiple themes theme json to be required. This makes cache invalidation easier, not harder.

Where is the there a performance lose here?

$theme_has_support = wp_cache_get( $cache_key, $cache_group );

/**
Expand All @@ -32,15 +37,13 @@ function wp_theme_has_theme_json() {
return (bool) $theme_has_support;
}

// Has the own theme a theme.json?
$theme_has_support = is_readable( get_stylesheet_directory() . '/theme.json' );

// 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' );
$wp_theme = wp_get_theme( $stylesheet );
Copy link
Member

Choose a reason for hiding this comment

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

See conversation about performance of wp_get_theme at https://github.com/WordPress/gutenberg/pull/45831/files#r1029225791

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 allows us to get if another theme supports theme json. It is needed for here and here.

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I think this conversation about performance of wp_get_theme has blown out of proportion a little. Yes, if we can avoid calling it 5 times in a single function, we should rather call it once and put it in a variable.

However, that is a micro optimization that I don't think is critical. I haven't seen any performance testing that would show us it is truly a concern; of course instantiating an object over and over is more expensive than not instantiating the object again, but to me this seems more like a nit-pick concern that won't matter for actual performance.

In other words, it is fine to introduce a call to wp_get_theme. Just don't call it when you don't have to. In any case, I think here we don't have to (see my overarching feedback).

Copy link
Member Author

Choose a reason for hiding this comment

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

@felixarntz YES! In this case, we call it once and after that, we cache. This is called once per page load. Making it fine. Calling it 5k times per page is the problem.

if ( ! $wp_theme->exists() ) {
return false;
}

$theme_has_support = $theme_has_support ? 1 : 0;
// Has the own theme a theme.json?
$theme_has_support = is_readable( $wp_theme->get_file_path( 'theme.json' ) ) ? 1 : 0;

wp_cache_set( $cache_key, $theme_has_support, $cache_group );

Expand All @@ -53,9 +56,15 @@ function wp_theme_has_theme_json() {
* Function to clean the cache used by wp_theme_has_theme_json method.
*
* Not to backport to core. Delete it instead.
*
* @param string $stylesheet Directory name for the theme. Optional. Defaults to current theme.
*/
function wp_theme_has_theme_json_clean_cache() {
_deprecated_function( __METHOD__, '14.7' );
function wp_theme_has_theme_json_clean_cache( $stylesheet = '' ) {
if ( empty( $stylesheet ) ) {
$stylesheet = get_stylesheet();
}
$cache_key = sprintf( 'wp_theme_has_theme_json_%s', $stylesheet );
wp_cache_delete( $cache_key, 'theme_json' );
}
}

Expand Down Expand Up @@ -190,7 +199,12 @@ function gutenberg_get_global_settings( $path = array(), $context = array() ) {
* @access private
*/
function _gutenberg_clean_theme_json_caches() {
wp_cache_delete( 'wp_theme_has_theme_json', 'theme_json' );
$stylesheet = get_stylesheet();
$template = get_template();
wp_theme_has_theme_json_clean_cache( $stylesheet );
if( $stylesheet !== $template ){
wp_theme_has_theme_json_clean_cache( $template );
}
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' );
Expand Down