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.

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

theme.json: theme_has_support() might cache wrong value #30478

Closed
ockham opened this issue Apr 2, 2021 · 17 comments · Fixed by #30830
Closed

theme.json: theme_has_support() might cache wrong value #30478

ockham opened this issue Apr 2, 2021 · 17 comments · Fixed by #30830
Assignees
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@ockham
Copy link
Contributor

ockham commented Apr 2, 2021

Discovered here: #30045 (comment). See that PR for instructions on how to repro.


Since #28786 (and later #28788, which renamed and moved the relevant function), we've been caching the return value of WP_Theme_JSON_Resolver::theme_has_support():

/**
* Whether the current theme has a theme.json file.
*
* @return boolean
*/
public static function theme_has_support() {
if ( ! isset( self::$theme_has_support ) ) {
self::$theme_has_support = (bool) self::get_file_path_from_theme( 'experimental-theme.json' );
}
return self::$theme_has_support;
}

This makes sense at first glance, since, as was stated in #28786:

It is very unlikely the readability of the file will change during the execution time.
Doing file-system requests isn't free of charge, storing the result in memory is much cheaper.

This seems to work fine in a "normal" WordPress environment; however, it seems to pose a problem for unit tests: WP_Theme_JSON_Resolver::theme_has_support() is first called while the current theme is still set to the default theme -- which of course doesn't have an experimental-theme.json file -- so we're erroneously caching false as return value for WP_Theme_JSON_Resolver::theme_has_support().

Even if an individual unit test later sets the current theme to one that has an experimental-theme.json, WP_Theme_JSON_Resolver::theme_has_support() will continue to return the cached false value.

Steps to reproduce

Check out trunk, and apply the following patch on top of it:

diff --git a/lib/class-wp-theme-json-resolver.php b/lib/class-wp-theme-json-resolver.php
index 31ecb553a2..8b2248a0cb 100644
--- a/lib/class-wp-theme-json-resolver.php
+++ b/lib/class-wp-theme-json-resolver.php
@@ -460,10 +460,12 @@ class WP_Theme_JSON_Resolver {
         * @return boolean
         */
        public static function theme_has_support() {
+               debug_print_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 2 );
+               echo get_stylesheet_directory() . "\n";
                if ( ! isset( self::$theme_has_support ) ) {
                        self::$theme_has_support = (bool) self::get_file_path_from_theme( 'experimental-theme.json' );
                }
-
+               echo "theme_has_support: " . self::$theme_has_support . "\n";
                return self::$theme_has_support;
        }
 

Then, run

npm run test-unit-php -- /var/www/html/wp-content/plugins/gutenberg/phpunit/class-block-templates-test.php

This should give something like

Output
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/client-assets.php:364]
#1  gutenberg_register_packages_styles() called at [/var/www/html/wp-includes/class-wp-hook.php:292]
/var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/../data/themedir1/default
theme_has_support: 
#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/global-styles.php:163]
#1  gutenberg_experimental_global_styles_register_user_cpt() called at [/var/www/html/wp-includes/class-wp-hook.php:292]
/var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/../data/themedir1/default
theme_has_support: 
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.16
Configuration: /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist

....#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:127]
#1  _gutenberg_add_template_part_area_info() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:60]
/var/www/html/wp-content/themes/tt1-blocks
theme_has_support: 
..                                                              6 / 6 (100%)#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:127]
#1  _gutenberg_add_template_part_area_info() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:109]
/var/www/html/wp-content/themes/tt1-blocks
theme_has_support: 
#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:127]
#1  _gutenberg_add_template_part_area_info() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:109]
/var/www/html/wp-content/themes/tt1-blocks
theme_has_support: 


Time: 1.44 seconds, Memory: 36.50 MB

OK (6 tests, 70 assertions)

Note that for the first two callsites of theme_has_support, which happen during bootstrapping -- lib/client-assets.php:364 and lib/global-styles.php:163 -- the output for echo get_stylesheet_directory() . "\n"; is

/var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/../data/themedir1/default

i.e., the default theme.

In later calls, it is /var/www/html/wp-content/themes/tt1-blocks; but in all cases, the return value of theme_has_support() is false.

cc/ @moorscode @nosolosw @Addison-Stavlo @david-szabo97 @WordPress/gutenberg-core

@ockham ockham added [Type] Bug An existing feature does not function as intended [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Apr 2, 2021
@ockham
Copy link
Contributor Author

ockham commented Apr 2, 2021

The most straight-forward remedy is to add a cache key -- the theme slug comes to mind:

diff --git a/lib/class-wp-theme-json-resolver.php b/lib/class-wp-theme-json-resolver.php
index 6815dc68ec..e312fe8b38 100644
--- a/lib/class-wp-theme-json-resolver.php
+++ b/lib/class-wp-theme-json-resolver.php
@@ -31,7 +31,7 @@ class WP_Theme_JSON_Resolver {
 	 *
 	 * @var boolean
 	 */
-	private static $theme_has_support = null;
+	private static $theme_has_support = array();
 
 	/**
 	 * Container for data coming from the user.
@@ -503,11 +503,12 @@ class WP_Theme_JSON_Resolver {
 	 * @return boolean
 	 */
 	public static function theme_has_support() {
-		if ( ! isset( self::$theme_has_support ) ) {
-			self::$theme_has_support = (bool) self::get_file_path_from_theme( 'experimental-theme.json' );
+		$theme_slug = get_stylesheet();
+		if ( ! isset( self::$theme_has_support[ $theme_slug ] ) ) {
+			self::$theme_has_support[ $theme_slug ] = (bool) self::get_file_path_from_theme( 'experimental-theme.json' );
 		}
 
-		return self::$theme_has_support;
+		return self::$theme_has_support[ $theme_slug ];
 	}
 
 	/**
diff --git a/lib/full-site-editing/block-templates.php b/lib/full-site-editing/block-templates.php
index d0bcab00c9..324570b30d 100644
--- a/lib/full-site-editing/block-templates.php
+++ b/lib/full-site-editing/block-templates.php
@@ -124,7 +124,7 @@ function _gutenberg_get_template_files( $template_type ) {
  * @return array Template.
  */
 function _gutenberg_add_template_part_area_info( $template_info ) {
-	if ( WP_Theme_JSON_Resolver::theme_has_support() ) {
+	if ( WP_Theme_JSON_Resolver::theme_has_support( $template_info['slug'] ) ) {
 		$theme_data = WP_Theme_JSON_Resolver::get_theme_data()->get_template_parts();
 	}
 

I'll file a PR.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Apr 2, 2021
@moorscode
Copy link
Contributor

Hi @ockham

Sounds like a good approach to me.
Don't forget to not apply the lib/full-site-editing/block-templates.php change though ;)

In efforts of improving things that are touched, I've got a related question; Is there a reason the cache property is not defined in the method itself?

private static $theme_has_support = array();

vs

public static function theme_has_support() {
     static $theme_has_support = array();
     ...
}

It would keep property close by, which shouldn't be used anywhere else, as you want the method to be the one source of truth.

This paradigm is also used elsewhere in the file:

https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-theme-json-resolver.php#L145-L146

@oandregal
Copy link
Member

I left at #30045 (comment) another thread to look at, in case it's related to that issue.

However, it's possible that WP_Theme_JSON_Resolver::theme_has_support() is called before any theme is set up

How can I test this? I mean, in isolation, not by running this. I'd like to understand how to break this code before changing it.

For testing, this is what I've done: hook into an action that runs before the theme is setup: add_action( 'plugins_loaded', ['WP_Theme_JSON_Resolver', 'theme_has_setup'] ) and add some log statements within the method. Also tried with setup_theme hook. Everything looks fine for me (testing with tt1-blocks and other default themes and every time they returned the proper value for that particular theme).

Also, by looking at the code of get_stylesheet_directory I don't see that it needs the theme loaded: it uses get_option('stylesheet') which returns the active theme and then builds a path based on some other checks that don't seem to need any theme loaded either.

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

I left at #30045 (comment) another thread to look at, in case it's related to that issue.

However, it's possible that WP_Theme_JSON_Resolver::theme_has_support() is called before any theme is set up

How can I test this? I mean, in isolation, not by running this. I'd like to understand how to break this code before changing it.

Yeah, apologies for that -- I didn't see the relevant info being rendered anywhere else, so it was a bit hard to repro otherwise 😅

Here's a way to repro (albeit through running a unit test -- so there's potentially a chance that the unit test is flawed):

Apply the following diff, and run npm run test-unit-php -- /var/www/html/wp-content/plugins/gutenberg/phpunit/class-block-templates-test.php:

diff --git a/lib/class-wp-theme-json-resolver.php b/lib/class-wp-theme-json-resolver.php
index 31ecb553a2..8b2248a0cb 100644
--- a/lib/class-wp-theme-json-resolver.php
+++ b/lib/class-wp-theme-json-resolver.php
@@ -460,10 +460,12 @@ class WP_Theme_JSON_Resolver {
         * @return boolean
         */
        public static function theme_has_support() {
+               debug_print_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 2 );
+               echo get_stylesheet_directory() . "\n";
                if ( ! isset( self::$theme_has_support ) ) {
                        self::$theme_has_support = (bool) self::get_file_path_from_theme( 'experimental-theme.json' );
                }
-
+               echo "theme_has_support: " . self::$theme_has_support . "\n";
                return self::$theme_has_support;
        }
 

That gives me:

Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/client-assets.php:364]
#1  gutenberg_register_packages_styles() called at [/var/www/html/wp-includes/class-wp-hook.php:292]
/var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/../data/themedir1/default
theme_has_support: 
#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/global-styles.php:163]
#1  gutenberg_experimental_global_styles_register_user_cpt() called at [/var/www/html/wp-includes/class-wp-hook.php:292]
/var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/../data/themedir1/default
theme_has_support: 
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.16
Configuration: /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist

....#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:127]
#1  _gutenberg_add_template_part_area_info() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:60]
/var/www/html/wp-content/themes/tt1-blocks
theme_has_support: 
..                                                              6 / 6 (100%)#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:127]
#1  _gutenberg_add_template_part_area_info() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:109]
/var/www/html/wp-content/themes/tt1-blocks
theme_has_support: 
#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:127]
#1  _gutenberg_add_template_part_area_info() called at [/var/www/html/wp-content/plugins/gutenberg/lib/full-site-editing/block-templates.php:109]
/var/www/html/wp-content/themes/tt1-blocks
theme_has_support: 


Time: 1.44 seconds, Memory: 36.50 MB

OK (6 tests, 70 assertions)

For testing, this is what I've done: hook into an action that runs before the theme is setup: add_action( 'plugins_loaded', ['WP_Theme_JSON_Resolver', 'theme_has_setup'] ) and add some log statements within the method. Also tried with setup_theme hook. Everything looks fine for me (testing with tt1-blocks and other default themes and every time they returned the proper value for that particular theme).

Also, by looking at the code of get_stylesheet_directory I don't see that it needs the theme loaded: it uses get_option('stylesheet') which returns the active theme

Yeah, so in the tests above, it seems to return the default theme (during tests setup?), sets that as the cache value for self::$theme_has_support, and later returns that (even when get_stylesheet_directory actually returns TT1 Blocks).

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

So judging from those call stacks, it seems the relevant callsites for WP_Theme_JSON_Resolver::theme_has_support() that happen when get_stylesheet_directory() still returns the default theme are

if ( ! WP_Theme_JSON_Resolver::theme_has_support() ) {

and

if ( ! WP_Theme_JSON_Resolver::theme_has_support() ) {

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

If I load WP in the browser with the above patch, both "early" callsites (lib/client-assets.php and lib/global-styles.php) actually do "see" TT1 Blocks, rather than the default theme. So it's arguably an issue with unit tests.

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

If I load WP in the browser with the above patch, both "early" callsites (lib/client-assets.php and lib/global-styles.php) actually do "see" TT1 Blocks, rather than the default theme. So it's arguably an issue with unit tests.

I tracked the stacktrace for those two back a bit further: It goes back to

require $_tests_dir . '/includes/bootstrap.php';
which I think loads this file from wordpress-develop: https://github.com/WordPress/wordpress-develop/blob/234c2b52ccf584cb93dce0eaf17431310b1d7458/tests/phpunit/includes/bootstrap.php

Edit: The latter file comes from wp-phpunit: https://github.com/wp-phpunit/wp-phpunit/blob/fefaaf8d086c0401d887661e21dc9e9afdfc0411/includes/bootstrap.php

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

This could be relevant?

/**
* Since wp-phpunit loads wp-settings.php at the end of its wp-config.php
* file, we need to avoid loading it too early in our own wp-config.php. If
* we load it too early, then some things (like MULTISITE) will be defined
* before wp-phpunit has a chance to configure them. To avoid this, create a
* copy of wp-config.php for phpunit which doesn't require wp-settings.php.
*
* Note that This needs to be executed using `exec` on the wordpress service
* so that file permissions work properly.
*
* This will be removed in the future. @see https://github.com/WordPress/gutenberg/issues/23171
*
*/
await dockerCompose.exec(
environment === 'development' ? 'wordpress' : 'tests-wordpress',
[
'sh',
'-c',
'sed "/^require.*wp-settings.php/d" /var/www/html/wp-config.php > /var/www/html/phpunit-wp-config.php && chmod 777 /var/www/html/phpunit-wp-config.php',
],
{
config: config.dockerComposeConfigPath,
log: config.debug,
}
);

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

I wonder if that means that the phpunit container isn't using the phpunit-wp-config.php copy of wp-config.php (without the require_once ABSPATH . '/wp-settings.php' call) as it apparently should, but uses wp-config.php?

npm run test-unit-php -- /var/www/html/wp-content/plugins/gutenberg/phpunit/class-block-templates-test.php

[...]

Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
#0  WP_Theme_JSON_Resolver::theme_has_support() called at [/var/www/html/wp-content/plugins/gutenberg/lib/client-assets.php:364]
#1  gutenberg_register_packages_styles() called at [/var/www/html/wp-includes/class-wp-hook.php:292]
#2  WP_Hook->apply_filters() called at [/var/www/html/wp-includes/class-wp-hook.php:316]
#3  WP_Hook->do_action() called at [/var/www/html/wp-includes/plugin.php:551]
#4  do_action_ref_array() called at [/var/www/html/wp-includes/class.wp-styles.php:135]
#5  WP_Styles->__construct() called at [/var/www/html/wp-includes/functions.wp-styles.php:24]
#6  wp_styles() called at [/var/www/html/wp-includes/functions.wp-styles.php:132]
#7  wp_register_style() called at [/var/www/html/wp-content/plugins/gutenberg/lib/blocks.php:175]
#8  gutenberg_register_core_block_styles() called at [/var/www/html/wp-content/plugins/gutenberg/lib/blocks.php:129]
#9  gutenberg_reregister_core_block_types() called at [/var/www/html/wp-includes/class-wp-hook.php:292]
#10 WP_Hook->apply_filters() called at [/var/www/html/wp-includes/class-wp-hook.php:316]
#11 WP_Hook->do_action() called at [/var/www/html/wp-includes/plugin.php:484]
#12 do_action() called at [/var/www/html/wp-settings.php:560]
#13 require_once(/var/www/html/wp-settings.php) called at [/var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/bootstrap.php:191]
#14 require(/var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/bootstrap.php) called at [/var/www/html/wp-content/plugins/gutenberg/phpunit/bootstrap.php:84]
#15 include_once(/var/www/html/wp-content/plugins/gutenberg/phpunit/bootstrap.php) called at [phar:///usr/local/bin/phpunit/phpunit/Util/FileLoader.php:57]
#16 PHPUnit\Util\FileLoader::load() called at [phar:///usr/local/bin/phpunit/phpunit/Util/FileLoader.php:45]
#17 PHPUnit\Util\FileLoader::checkAndLoad() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:1058]
#18 PHPUnit\TextUI\Command->handleBootstrap() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:863]
#19 PHPUnit\TextUI\Command->handleArguments() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:173]
#20 PHPUnit\TextUI\Command->run() called at [phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:162]
#21 PHPUnit\TextUI\Command::main() called at [/usr/local/bin/phpunit:620]

cc/ @noahtallen Can you weigh in if that's the expected behavior?

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

Ah no, that seems unrelated: wp-env only removes the wp-settings.php import from phpunit-wp-config.php, whereas in the call stack, wp-settings.php is imported from (wp-phpunit's) includes/bootstrap.php.

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

Maybe this could help? https://github.com/wp-phpunit/wp-phpunit/blob/fefaaf8d086c0401d887661e21dc9e9afdfc0411/includes/bootstrap.php#L177-L188

I.e. using the wp_tests_options global to set the theme somehow? We're already using it here:

$GLOBALS['wp_tests_options'] = array(
'gutenberg-experiments' => array(
'gutenberg-widget-experiments' => '1',
'gutenberg-full-site-editing' => 1,
),
);

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

Yeah, so this seems to do the trick:

diff --git a/phpunit/bootstrap.php b/phpunit/bootstrap.php
index 8315c12292..1767a33748 100644
--- a/phpunit/bootstrap.php
+++ b/phpunit/bootstrap.php
@@ -75,6 +75,8 @@ $GLOBALS['wp_tests_options'] = array(
                'gutenberg-widget-experiments' => '1',
                'gutenberg-full-site-editing'  => 1,
        ),
+       'stylesheet' => 'tt1-blocks',
+       'template'   => 'tt1-blocks'
 );
 
 // Enable the widget block editor.

However, that'd mean we'd hard-wire the theme (initially) used for testing to TT1 Blocks. I'm not totally sure that's okay, but I guess it is?

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

Yeah, so this seems to do the trick: [...]

PR: #30686

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2021

Updated the issue description to reflect the findings, and to provide better instructions for reproducing.

@noahtallen
Copy link
Member

Can you weigh in if that's the expected behavior?

It's definitely weird because we're using the phpunit library instead of the one from core. But it sounds like you figured it out. Related issue about improving phpunit: #23171

@oandregal
Copy link
Member

Thanks for the instructions, Bernie. If I understood this correctly, it looks like there's an issue when a theme is switched programatically but doesn't happen on a normal WordPress request. I understand this can be an issue beyond unit tests theoretically, although I'm not sure under what practical conditions.

Anyway, it sounds like what we need to do is cleaning all the cached values upon a theme switch so they can be recalculated. Prepared #30830

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
4 participants