Allow for passing stylesheet to `wp_theme_has_theme_json`. #45955

spacedmonkey · 2022-11-21T22:52:45Z

What?

Allow stylesheet to wp_theme_has_theme_json. This allows for the developers to check if none active themes support theme json. This also improves caching, as caches are no longer depending on current theme.

See #45950 #45831

Why?

How?

Testing Instructions

Screenshots or screencast

codesandbox · 2022-11-21T22:52:47Z

Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

oandregal · 2022-11-22T11:42:39Z

lib/compat/wordpress-6.2/get-global-styles-and-settings.php

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


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

This allows us to get if another theme supports theme json. It is needed for here and here.

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).

@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.

oandregal · 2022-11-22T11:50:18Z

lib/compat/wordpress-6.0/get-global-styles-and-settings.php

@@ -85,7 +86,7 @@ function gutenberg_get_global_styles_svg_filters() {
 		}
 	}

-	$supports_theme_json = wp_theme_has_theme_json();
+	$supports_theme_json = wp_theme_has_theme_json( $stylesheet );


Do we need to do this? Isn't this code querying for the active theme (hence, no need for stylesheet)?

This saves a another lookup to get_option. We have the stylesheet to hand, so why not just pass it in and save a lookup to cache.

oandregal · 2022-11-22T11:50:36Z

lib/compat/wordpress-6.1/get-global-styles-and-settings.php

 	if ( $can_use_cached ) {
 		$cached = get_transient( $transient_name );
 		if ( $cached ) {
 			return $cached;
 		}
 	}
 	$tree                = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data();
-	$supports_theme_json = wp_theme_has_theme_json();
+	$supports_theme_json = wp_theme_has_theme_json( $stylesheet );


Do we need to do this? Isn't this code querying the active theme (hence, no need for stylesheet)?

https://github.com/WordPress/gutenberg/pull/45955/files#r1029270566

oandregal · 2022-11-22T12:02:50Z

lib/compat/wordpress-6.2/get-global-styles-and-settings.php

+		if ( empty( $stylesheet ) ) {
+			$stylesheet = get_stylesheet();
+		}
+		$cache_key         = sprintf( 'wp_theme_has_theme_json_%s', $stylesheet );


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?

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.

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?

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?

oandregal · 2022-11-22T12:03:23Z

lib/compat/wordpress-6.2/get-global-styles-and-settings.php

-	function wp_theme_has_theme_json_clean_cache() {
-		wp_cache_delete( 'wp_theme_has_theme_json', 'theme_json' );
+	function _wp_theme_has_theme_json_clean_cache_start_previewing_theme( $manager ){
+		wp_theme_has_theme_json_clean_cache( $manager->get_stylesheet() );


See https://github.com/WordPress/gutenberg/pull/45955/files#r1029244275 we need to clear the cache of the previous theme, not the actual one.

oandregal · 2022-11-22T12:03:47Z

lib/compat/wordpress-6.2/get-global-styles-and-settings.php

+	 * @param WP_Theme $old_theme WP_Theme instance of the old theme.
+	 */
+	function _wp_theme_has_theme_json_clean_cache_switch_theme( $new_name, $new_theme, $old_theme ) {
+		wp_theme_has_theme_json_clean_cache( $new_theme->get_stylesheet() );


See https://github.com/WordPress/gutenberg/pull/45955/files#r1029244275

oandregal · 2022-11-22T13:08:24Z

lib/experimental/class-wp-theme-json-resolver-gutenberg.php

+		 * Bail early if the theme does not support a theme.json.
+		 *
+		 */
+		if ( ! wp_theme_has_theme_json( $theme->get_stylesheet() ) ) {


I think these inner private methods should not hold this kind of domain logic. It should be the consumer responsibility to ask for the data they need. This is how I envision improving on the current situation:

1st step is making the get_merged_data method work properly (take into account the origin) Merged data should consider origin to return early #45969

2nd step is modifying the public methods to ask for the proper origin of data they want. Example for the settings Update which origins are queried for gutenberg_get_global_settings #45971

On this we disagree. The current system of origin, is hard to understand, hard to test and VERY unclaer from a development standpoint. We should be moving away from this pattern and the code easier to use any understand.

The fundamental issue with adding caching to the resolver instead of adding it to the public methods is this: every public method has different conditions for cache invalidation. settings & styles need some, theme.json check needs some other, and patterns/custom templates need totally different conditions.

We cannot simply replicate those invalidations within the resolver. That was the core issue we ran into in the WordPress 6.1 cycle.

This method get_user_data_from_wp_global_styles take a param of WP_Theme, we need to respect that and check the requested them support theme.json.

Look at this method in core. https://github.com/WordPress/wordpress-develop/blob/9ed27339b19eb07aa01bcb7e593be2e11f928f1e/src/wp-includes/class-wp-theme-json-resolver.php#L423-L425

@felixarntz add this change. Can you chime in here.

lib/experimental/class-wp-theme-json-resolver-gutenberg.php

spacedmonkey · 2022-12-06T16:25:11Z

lib/experimental/class-wp-theme-json-resolver-gutenberg.php

-				$parent_theme_json_file = static::get_file_path_from_theme( 'theme.json', true );
-				if ( '' !== $parent_theme_json_file ) {
-					$parent_theme_json_data = static::read_json_file( $parent_theme_json_file );
+				if ( wp_theme_has_theme_json( $wp_theme->parent()->get_stylesheet() ) ) {


@felixarntz @oandregal Is the perfect example of why we need this function to allow for passing a stylesheet. In this context, we need to know if the parent ( not the current ) theme has theme json. This is need for every request, making this change required.

felixarntz

@oandregal @spacedmonkey Chiming in here late, I see both of your concerns:

I agree with you @spacedmonkey that we should provide a way to find out whether a theme has theme.json for any theme, not just the current theme.
I also agree with you @oandregal that changing this wp_theme_has_theme_json() function to support any theme should not be necessary.

The main use-case surely is the current theme. However, WordPress has always provided ways to get the same information for any theme, so we should work on that too. But my main feedback is that those things should happen in different places to align with how WordPress core works today.

Consider the following: WordPress core today has theme functions, those typically work only for the current theme. But it also has the WP_Theme class that provides methods to get the same data for any theme (whatever you instantiate the WP_Theme with, via wp_get_theme()).

Some examples:

get_stylesheet() vs WP_Theme::get_stylesheet()
get_template() vs WP_Theme::get_template()
get_template_directory() vs WP_Theme::get_template_directory()
get_theme_file_path() vs WP_Theme::get_file_path()

In all of the above, the function is always for the current theme (no parameter), and the method works for whatever WP_Theme is instantiated as. In other words, I would propose we follow the same approach:

Keep wp_theme_has_theme_json() simple, without a parameter and without having to use wp_get_theme().
Introduce a method WP_Theme::has_theme_json() that allows doing the same for any theme.

That's my 2 cents.

felixarntz · 2022-12-16T00:31:34Z

lib/compat/wordpress-6.2/get-global-styles-and-settings.php

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


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).

carolinan · 2023-01-04T05:07:31Z

check if none active themes support theme json.

I believe we will need this change for #45371 which is part of Phase 2: Customization

felixarntz · 2023-01-17T11:31:13Z

@spacedmonkey Per my comment in #45955 (review), I still don't think this is a good idea. IMO we should rather introduce a has_theme_json() method to WP_Theme to support that use-case. Then you can call wp_get_theme( ... )->has_theme_json().

spacedmonkey requested review from oandregal, mmtr, felixarntz and andrewserong November 21, 2022 22:52

oandregal mentioned this pull request Nov 22, 2022

Deprecated get_file_path_from_theme method. #45831

Merged

oandregal reviewed Nov 22, 2022

View reviewed changes

spacedmonkey self-assigned this Nov 24, 2022

spacedmonkey mentioned this pull request Nov 29, 2022

Make theme.json object caches non persistent #46150

Merged

spacedmonkey commented Dec 6, 2022

View reviewed changes

lib/experimental/class-wp-theme-json-resolver-gutenberg.php Outdated Show resolved Hide resolved

spacedmonkey commented Dec 6, 2022

View reviewed changes

felixarntz reviewed Dec 16, 2022

View reviewed changes

felixarntz mentioned this pull request Dec 16, 2022

wp_theme_has_theme_json: add public method to know whether the theme supports theme.json WordPress/wordpress-develop#3556

Closed

Rebase.

4ed8ce5

spacedmonkey force-pushed the try/multiple-themes-support branch from 567f818 to 4ed8ce5 Compare January 17, 2023 11:05

spacedmonkey requested review from felixarntz and oandregal January 17, 2023 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for passing stylesheet to `wp_theme_has_theme_json`. #45955

Allow for passing stylesheet to `wp_theme_has_theme_json`. #45955

spacedmonkey commented Nov 21, 2022 •

edited

Loading

codesandbox bot commented Nov 21, 2022

oandregal Nov 22, 2022

spacedmonkey Nov 22, 2022

felixarntz Dec 16, 2022

spacedmonkey Dec 22, 2022

oandregal Nov 22, 2022 •

edited

Loading

spacedmonkey Nov 22, 2022

oandregal Nov 22, 2022 •

edited

Loading

spacedmonkey Nov 22, 2022

oandregal Nov 22, 2022

spacedmonkey Nov 22, 2022

oandregal Nov 22, 2022

spacedmonkey Nov 22, 2022

oandregal Nov 22, 2022

oandregal Nov 22, 2022

oandregal Nov 22, 2022 •

edited

Loading

spacedmonkey Nov 22, 2022

oandregal Nov 22, 2022 •

edited

Loading

spacedmonkey Nov 22, 2022

spacedmonkey Dec 22, 2022

spacedmonkey Dec 6, 2022

felixarntz left a comment

felixarntz Dec 16, 2022

carolinan commented Jan 4, 2023

felixarntz commented Jan 17, 2023

Allow for passing stylesheet to wp_theme_has_theme_json. #45955

Are you sure you want to change the base?

Allow for passing stylesheet to wp_theme_has_theme_json. #45955

Conversation

spacedmonkey commented Nov 21, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

codesandbox bot commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolinan commented Jan 4, 2023

felixarntz commented Jan 17, 2023

Allow for passing stylesheet to `wp_theme_has_theme_json`. #45955

Allow for passing stylesheet to `wp_theme_has_theme_json`. #45955

spacedmonkey commented Nov 21, 2022 •

edited

Loading

oandregal Nov 22, 2022 •

edited

Loading

oandregal Nov 22, 2022 •

edited

Loading

oandregal Nov 22, 2022 •

edited

Loading

oandregal Nov 22, 2022 •

edited

Loading