WordPress database error when installing PHPUnit tests

[Chouby]

It is reported for example in this ​job

WordPress database error:</strong> [Table &#039;wordpress_develop_tests.wptests_posts&#039; doesn&#039;t exist]<br /><code>
					SELECT   wptests_posts.ID
					FROM wptests_posts 
					WHERE 1=1  AND ( 
  0 = 1
) AND wptests_posts.post_type = &#039;wp_global_styles&#039; AND ((wptests_posts.post_status = &#039;publish&#039;))
					GROUP BY wptests_posts.ID
					ORDER BY wptests_posts.post_date DESC
					LIMIT 0, 1
					SELECT   wptests_posts.ID
					FROM wptests_posts 
					WHERE 1=1  AND ( 
  0 = 1
) AND wptests_posts.post_type = 'wp_global_styles' AND ((wptests_posts.post_status = 'publish'))
					GROUP BY wptests_posts.ID
					ORDER BY wptests_posts.post_date DESC
					LIMIT 0, 1
				 made by require_once('wp-settings.php'), do_action('wp_loaded'), WP_Hook->do_action, WP_Hook->apply_filters, call_user_func_array, WP_Duotone::set_global_style_block_names, WP_Theme_JSON_Resolver::get_merged_data, WP_Theme_JSON_Resolver::get_user_data, WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles, WP_Query->query, WP_Query->get_posts

I noticed it in WP 6.3-beta2-56110. It is not reported in WP 6.3-beta2

[Chouby]

It seems that this error occurs since [56101]

[swissspidy]

This is still happening for me on trunk.

It happens the first time when installing & running the PHPUnit tests from a plugin.

After that, all the database tables exist, so on the second run it's not failing.

From what I can see:

1. phpunit/includes/bootstrap.php calls phpunit/includes/install.php to install WordPress
2. phpunit/includes/install.php loads wp-settings.php
3. wp-settings.php fires wp_loaded
4. WP_Duotone::set_global_style_block_names() is hooked to wp_loaded and triggers a WP_Query
5. Now you have the database error

[costdev]

If this only occurs in the test suite and we want to avoid changes to src, could we just include wp-includes/plugin.php, unhook the action near the start of tests/phpunit/includes/install.php and hook it again at the end? Or do similar in tests/includes/bootstrap.php around lines 261-269?

[swissspidy]

That'd just paper over the issue, we should rather fix this at the root.

There is no point in doing a WP_Query on every single page load just for duotone. wp_loaded is really not the right way for this.

[costdev]

Agreed, as it's also happening when installing Beta 4.

[SergeyBiryukov]

This seems similar to a few earlier tickets: #53830, #54634, #54800, #54849, #55632.

In line with [53306] / #55632, perhaps WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles() could bail early if wp_installing(), see 58673.diff​.

[spacedmonkey]

I strongly recommend removing these lines

add_action( 'wp_loaded', array( 'WP_Duotone', 'set_global_styles_presets' ), 10 );
add_action( 'wp_loaded', array( 'WP_Duotone', 'set_global_style_block_names' ), 10 );

Loading these method on every requests is wasteful, as duo tone is not used on all pages. REST API calls for example.

[joemcgill]

@spacedmonkey noticed that this same WP_Duotone::set_global_styles_presets method being called on every request is causing a large unnecessary performance regression in trunk (7–9% of total server response time in my profiling).

While ensuring this isn't run during wp_installing() would be sufficient to fix this issue, it would be nice to fix the performance regression at the same time, by only calling that method when it's needed.

I'm adding the performance focus and increasing the priority on this to "high" so we can investigate and resolve this prior to RC1. @SergeyBiryukov if you prefer to commit your patch in the mean time, we can open a new ticket for the performance regression.

cc: @isabel_brison and @onemaggie, since you were both involved in the original commit that introduced this functionality.

Trac ticket: https://core.trac.wordpress.org/ticket/58673

[spacedmonkey]

@joemcgill @flixos90 I put together a little POC of lazily loading the expensive properties in the Duotone class.

Take a look at the PR ​#4831.

[SergeyBiryukov]

Replying to joemcgill:

While ensuring this isn't run during wp_installing() would be sufficient to fix this issue, it would be nice to fix the performance regression at the same time, by only calling that method when it's needed.

Yeah, removing this from wp_loaded and calling conditionally when needed would be ideal.

​PR #4831 looks good to me at a glance.

[flixos90]

Cross-posting here from ​my Slack comment: ​Beta 4 benchmarks show a small regression in server-side performance compared to ​Beta 2 benchmarks. Given the commits from #58555 happened after Beta 2, together with what @joemcgill reported above, they may be partially responsible for that regression.

[flixos90]

I conducted server-side performance benchmarks for (​see the results): While it looks like the PR is improving performance overall, it is only a small overall difference (in the wp-total metric) of less than 1%. Interestingly, the PR makes the wp-before-template metric faster but the wp-template metric slower. It's almost as if the performance overhead is just being moved elsewhere.

I agree with the previous comments and with the approach of the PR. That said, the benchmark difference is too small to reliably confirm this as a regression. So I doubt that this change is what caused the overall regression that I shared in 15.

Maybe profiling can help clarify. @spacedmonkey Can you please do a profiling before & after comparison on your end?

[spacedmonkey]

Thanks for doing the benchmarking here @flixos90 .

I beleive the biggest performance issue here is this ​line. Parsing and merging all this data, when it was not doing so before. I expected to see better results than 1%. I will do some profiling in the morning and get back. My PR just defers this later / when it is needed. I saw REST API calls being much faster for example.

it worth noting, you need to test on a classic theme without a theme.json, with a theme.json and a block theme.

[spacedmonkey]

@flixos90 @swissspidy @joemcgill

I have spent some time improving ​#4831, it now ready for review. Details of the change are detailed in the description.

This change, only defers the loading of theme.json data until it needed. This fixes the PHPUnit issue in this ticket and means Duo tone only loads if needed. Without a larger refactor ( out of scope of this stage of release ), we can not get rid of this performance regression completely. Calling WP_Theme_JSON_Resolver::get_merged_data() is expensive and there is no getting around that for now.

In my testing, I compared the following. Block / classic theme, post with and without a image block ( that support duo tone) and a REST API request with no blocks.

These were run on 1000 runs, PHP 8.2, using test data.

URL	Success Rate	Response Time (median)	wp-load-alloptions-query (median)	wp-before-template (median)	wp-before-template-db-queries (median)	branch	theme
/2023/01/13/media-category-blocks/	100%	69.45	0.89	30.4	2.13	trunk	twentytwentyone
/2023/01/13/media-category-blocks/	100%	85.33	0.77	38.31	2.89	trunk	twentytwentythree
/2023/01/13/media-category-blocks/	100%	59.37	0.76	21.63	1.77	fix/fix-duo-tone	twentytwentyone
/2023/01/13/media-category-blocks/	100%	82.33	0.75	35.69	2.7	fix/fix-duo-tone	twentytwentythree
/2023/07/10/hello-world/	100%	64.94	0.91	31.25	2.11	trunk	twentytwentyone
/2023/07/10/hello-world/	100%	97.63	0.9	46.66	3.46	trunk	twentytwentythree
/2023/07/10/hello-world/	100%	52.87	0.77	21.32	1.71	fix/fix-duo-tone	twentytwentyone
/2023/07/10/hello-world/	100%	79.4	0.76	36.57	2.72	fix/fix-duo-tone	twentytwentythree
/wp-json/wp/v2/types	100%	40.19	N/A	N/A	N/A	trunk	twentytwentyone
/wp-json/wp/v2/types	100%	41.33	N/A	N/A	N/A	trunk	twentytwentythree
/wp-json/wp/v2/types	100%	28.12	N/A	N/A	N/A	fix/fix-duo-tone	twentytwentyone
/wp-json/wp/v2/types	100%	40.37	N/A	N/A	N/A	fix/fix-duo-tone	twentytwentythree

[nazmul111]

I am Not find it when installing to 6.3beta4

[mukesh27]

Thanks @spacedmonkey for the PR. It need manual testing.

However, there seems to be a bug caused by this based on ​https://github.com/WordPress/wordpress-develop/pull/4831#pullrequestreview-1526511182? From that perspective, I am slightly worried about the refactoring in this PR. While I'm onboard with the PR in its current state, we may want to be more cautious and go with the much simpler change based on the changes from yesterday, before you refactored further.

I've tested it and it is fixed by f10a53f.

Can we rebase this now that 4839 has been merged?

[swissspidy]

@nazmul111 This only happens when installing the WordPress test suite when doing custom plugin/theme development, not when regularly installing WordPress itself.

[spacedmonkey]

In 56226:

Editor: Lazily load Duotone settings only when needed.

Introduced in [56101] the WP_Duotone class, hooks into the wp_loaded action to load duotone style data from global styles. Hooking in early in the bootstrap process caused a number of problems. This hook, triggered an error on installing, as this lookup for global styles, would result in a global post trying to be created, even before the table existed. Additionally, this implementation caused a severe performance regression, as duotone styling data was loaded unnecessarily for requests that did not require such data, such as REST API calls or actions within the wp-admin interface.

In this change, refactor the WP_Duotone to lazily load the global styles and theme.json data, only when a block that supports duotone is encountered. The method render_duotone_support was change to take a third parameter to reuse the existing WP_Block_Type object passed to the filter, to save it being looked up again. The code has also got improved type checking and the use of the util function block_has_support. Furthermore, the code's readability has been improved, along with enhancements to the documentation blocks.

Props Chouby, spacedmonkey, SergeyBiryukov, swissspidy, costdev, joemcgill, flixos90, mukesh27, nazmul111, ajlende, isabel_brison.
Fixes #58673.

Committed https://core.trac.wordpress.org/changeset/56226 🎉