Remove usage of get_default_block_editor_settings #46112

spacedmonkey · 2022-11-28T10:08:08Z

What?

For context #57077

This fix, fixes a very serious performance issue, that results in calling get_optiion 2700-4000 times per page load.

The issue, is that the function get_default_block_editor_settings is called. This function returns an array of data, including lots of information needed for the editor. But in this context, json resolved is getting lots of data is that is never used. As this method is call 200-500 times per page load, it results in all these calls to get_option, is_rtl and get_allowed_mime_types, data is never used. This functions have serious overhead, as all of them have filters and some even database queries attached to them. As this function is called on the front end, this results in 11-15% of extra page load.

This PR is simple, just pick the parts of get_default_block_editor_settings that are needed.

Why?

How?

Testing Instructions

Screenshots or screencast

Code path.

Before

After

felixarntz

@spacedmonkey The idea here makes total sense to me, however I think there's a more efficient approach to it. This PR would introduce much duplicate code. I left my alternative suggestion below.

If we go with that, we won't be able to do it purely in Gutenberg since the function is only in WP core, but I think that's okay. If you agree, let's open a Trac ticket to enhance that function to allow granular field computation for only what is needed.

Then we can already fix this in core, and Gutenberg will only need a tiny PR to pass the new parameter. (It will be backward compatible since when the function doesn't support the new parameter it'll just be slower, but not broken.)

felixarntz · 2022-11-28T19:00:01Z

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

+		$gradient_presets = current( (array) get_theme_support( 'editor-gradient-presets' ) );
+		if ( false !== $gradient_presets ) {
+			$editor_settings['gradients'] = $gradient_presets;
+		}


All of the new code here is now a bit of a duplicate code from what is already in get_default_block_editor_settings(). I think we can solve the problem in a more efficient way by enhancing that function.

felixarntz · 2022-11-28T19:02:35Z

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


 		/*
 		 * We want the presets and settings declared in theme.json
 		 * to override the ones declared via theme supports.
 		 * So we take theme supports, transform it to theme.json shape
 		 * and merge the static::$theme upon that.
 		 */
-		$theme_support_data = WP_Theme_JSON_Gutenberg::get_from_editor_settings( get_default_block_editor_settings() );
+		$theme_support_data = WP_Theme_JSON_Gutenberg::get_from_editor_settings( $editor_settings );


What I would suggest is the following:

Add an optional $fields parameter (array of strings) to the get_default_block_editor_settings() function.

Change the function internals to only compute the properties which are requested.

Pass the new parameter here so that only the above actually needed properties are computed.

Basically so that we can do something like:

$theme_support_data = WP_Theme_JSON_Gutenberg::get_from_editor_settings( get_default_block_editor_settings( array( 'disableCustomColors', 'disableCustomFontSizes', 'disableCustomGradients', 'disableLayoutStyles', 'enableCustomLineHeight', 'enableCustomSpacing', 'enableCustomUnits', 'colors', 'fontSizes', 'gradients', ) ) );

Broken it up into it's own function with a comment. b0a9764

spacedmonkey · 2022-11-28T23:52:34Z

@spacedmonkey The idea here makes total sense to me, however I think there's a more efficient approach to it. This PR would introduce much duplicate code. I left my alternative suggestion below.

If we go with that, we won't be able to do it purely in Gutenberg since the function is only in WP core, but I think that's okay. If you agree, let's open a Trac ticket to enhance that function to allow granular field computation for only what is needed.

Then we can already fix this in core, and Gutenberg will only need a tiny PR to pass the new parameter. (It will be backward compatible since when the function doesn't support the new parameter it'll just be slower, but not broken.)

Where I understand where you are coming from here. I am not sure I agree. Using block settings functions was always wrong here. These are not block settings, these are theme feature detection. This function gives you the data but doesn't really describe what is happening here. If we add fields or a flag as a param, to function, that just makes things more confusing and the logic harder to follow.

I think a better way forward, might be to add a function, like get_block_theme_settings, or something, on merge into core, we can use get_block_theme_settings in get_default_block_editor_settings, to stop repeated code.

oandregal · 2022-12-01T15:49:11Z

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

@@ -16,103 +16,6 @@
 * @access private
 */
 class WP_Theme_JSON_Resolver_Gutenberg extends WP_Theme_JSON_Resolver_6_2 {
-	/**


There's something in this file that still needs to live in the experimental folder: the call to gutenberg_add_registered_webfonts_to_theme_json.

This is in my list of things to look at to give it some clarity and improve the developer experience. Given that the change required here is a one-liner (get_default_block_editor_settings to gutenberg_get_block_theme_supports) and we need to look at this separately, would you mind making your changes in this file directly instead of moving it to 6.2?

Clarified in this slack thread.

oandregal · 2022-12-01T15:51:33Z

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

+ *
+ * @return array
+ */
+function gutenberg_get_block_theme_supports() {


Would you consider a name such as gutenberg_get_legacy_theme_supports_for_theme_json? Or something along that vein. It's more in line with what this code is used for. The current naming may imply that it returns all theme supports, and may mislead people.

Orignally was in experimental, but it would mean, that this code would do nothing in the plugin - 999b258

I'm not sure I follow. Perhaps you meant to comment on this other thread? https://github.com/WordPress/gutenberg/pull/46112/files#r1037282457

If so, how does it do nothing if this code stays in lib/experimental? The whole Gutenberg uses the class WP_Theme_JSON_Resolver_Gutenberg defined in this file.

oandregal · 2022-12-01T15:56:41Z

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

+ */
+function gutenberg_get_block_theme_supports() {
+	$theme_settings = array(
+		'disableCustomColors'    => get_theme_support( 'disable-custom-colors' ),


Looking at the code that uses this data (I didn't find a more recent definition of this code in Gutenberg), we need:

disableCustomColors

disableCustomGradients

disableCustomFontSizes

enableCustomLineHeight

enableCustomUnits

enableCustomSpacing

It seems we can get rid of disableLayoutStyles in this method?

oandregal · 2022-12-01T16:00:22Z

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

@@ -184,6 +184,41 @@ function gutenberg_get_global_settings( $path = array(), $context = array() ) {
 }

 /**
+ * Repeated logic from `get_default_block_editor_settings` function. When implemented into core,


It should also be ok to have two functions in core: get_default_block_editor_settings and the very specific get_legacy_theme_supports_for_theme_json.

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

lib/compat/wordpress-6.2/class-wp-theme-json-resolver-6-2.php

Co-authored-by: André <583546+oandregal@users.noreply.github.com>

oandregal · 2022-12-01T17:48:14Z

lib/experimental/get-global-styles-and-settings.php

+ *
+ * @return array
+ */
+function gutenberg_get_legacy_theme_supports_for_theme_json() {


Oh, sorry for the confusion. This code should live in lib/compat/wordpress-6.2 or lib/compat/wordpress-6.1, depending on the version we aim to backport it to. I only mentioned the resolver in my comment about the experimental folder.

oandregal · 2022-12-01T17:50:55Z

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

@@ -73,7 +81,7 @@ public static function get_theme_data( $deprecated = array(), $settings = array(
 		 * So we take theme supports, transform it to theme.json shape
 		 * and merge the static::$theme upon that.
 		 */
-		$theme_support_data = WP_Theme_JSON_Gutenberg::get_from_editor_settings( get_default_block_editor_settings() );
+		$theme_support_data = WP_Theme_JSON_Gutenberg::get_from_editor_settings( gutenberg_get_legacy_theme_supports_for_theme_json() );


Other than this line, is any other change necessary?

They are part of core. I will do that in another PR.

* Remove usage of get_default_block_editor_settings`. * Remove to it's own function. * Fix lints. * Move to 6.2 class. * WP_Theme_JSON -> WP_Theme_JSON_Gutenberg. * Update lib/compat/wordpress-6.2/class-wp-theme-json-resolver-6-2.php Co-authored-by: André <583546+oandregal@users.noreply.github.com> * Move functionality around again. * Fix merge conflict * Fix more issues. * More moving. * Revert changes. * More reverts. Co-authored-by: André <583546+oandregal@users.noreply.github.com>

#46112

… inheriting per WordPress version (#46750) * Backport WP_Theme_JSON_Resolver from core as it is * Substitute WP_Theme_JSON by WP_Theme_JSON_Gutenberg * Substitute WP_Theme_JSON_Data by WP_Theme_JSON_Data_Gutenberg * Rename WP_Theme_JSON_Resolver to WP_Theme_JSON_Resolver_Base The goal is to use it as base to inherit from, until we are able to remove all the children. * Make WP_Theme_JSON_Resolver_6_1 inherit from WP_Theme_JSON_Resolver_Base * 6.1: remove field already defined in base class * 6.1: remove translate, it is equal to base method * 6.1: remove get_core_data, it does not have base changes * 6.1: remove get_user_data Missed core changes and does not do anything differently from core. * 6.1: remove get_user_data_from_wp_global_styles It misses core changes and does not do anything differently. * 6.2: inherit from WP_Theme_JSON_Resolver_Base instead of 6.1 This makes the WP_Theme_JSON_Resover_6_1 class unused. * 6.1: remove class no longer in use * 6.2: deprecate theme_has_support #45380 * 6.2: substitute WP_Theme_JSON_Resolver::theme_has_support by wp_theme_has_theme_json() #45168 * 6.2: port changes to get_user_data_from_wp_global_styles #46043 * 6.2: update get_merged_data #45969 * 6.2: remove get_user_data Same code as core. There's a check for detecting whether the class is an instance of WP_Theme_JSON_Gutenberg that was not ported. This check was introduced to make sure the cache of the core class didn't interfere with the cache of the Gutenberg class, so it's no longer necessary. See #42756 * experimental: make it inherit from WP_Theme_JSON_Resolver_Base * 6.2: remove class no longer in use * experimental: delete remove_json_comments It's already part of the base class. * experimental: remove get_block_data It's already part of the base class and it didn't have the changes from core. * experimental: appearanceTools Port changes to the base class that were meant to be part of 6.1. See #43337 * experimental: port webfonts (experimental API) see #37140 * experimental: use gutenberg_get_legacy_theme_supports_for_theme_json #46112 * experimental: get_theme_data, all code is in the base class * experimental: remove empty class * Rename WP_Theme_JSON_Resolver_Base to WP_Theme_JSON_Resolver_Gutenberg * Fix lint issue: rename class-wp-theme-json-resolver.php to class-wp-theme-json-resolver-gutenberg.php * Move theme.json to unversioned lib/ * Move theme-i18n.json to unversioned lib/

Mamaduka · 2023-01-24T18:10:46Z

Backport: WordPress/wordpress-develop#3902

Remove usage of get_default_block_editor_settings`.

9e82a0b

spacedmonkey added [Type] Performance Related to performance efforts Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release Needs PHP backport Needs PHP backport to Core labels Nov 28, 2022

spacedmonkey requested review from scruffian, peterwilsoncc, aristath, felixarntz, jorgefilipecosta and anton-vlasenko November 28, 2022 10:08

spacedmonkey self-assigned this Nov 28, 2022

spacedmonkey requested review from oandregal and draganescu November 28, 2022 10:08

spacedmonkey mentioned this pull request Nov 28, 2022

Add WP_Object_Cache to the gutenberg_get_global_settings method #45372

Merged

felixarntz reviewed Nov 28, 2022

View reviewed changes

spacedmonkey and others added 5 commits November 29, 2022 12:46

Remove to it's own function.

b0a9764

Fix lints.

2e646a7

Move to 6.2 class.

999b258

WP_Theme_JSON -> WP_Theme_JSON_Gutenberg.

4d1a053

Merge branch 'trunk' into fix/performance-issue

0e7894a

spacedmonkey requested a review from felixarntz November 30, 2022 17:36

oandregal reviewed Dec 1, 2022

View reviewed changes

lib/compat/wordpress-6.2/get-global-styles-and-settings.php Outdated Show resolved Hide resolved

oandregal reviewed Dec 1, 2022

View reviewed changes

lib/compat/wordpress-6.2/class-wp-theme-json-resolver-6-2.php Outdated Show resolved Hide resolved

spacedmonkey and others added 2 commits December 1, 2022 16:46

Update lib/compat/wordpress-6.2/class-wp-theme-json-resolver-6-2.php

725db89

Co-authored-by: André <583546+oandregal@users.noreply.github.com>

Move functionality around again.

0f3c45b

Fix merge conflict

c978b88

oandregal reviewed Dec 1, 2022

View reviewed changes

Fix more issues.

90cc1ce

oandregal reviewed Dec 1, 2022

View reviewed changes

spacedmonkey added 3 commits December 1, 2022 17:51

More moving.

f595fba

Revert changes.

fcc46b2

More reverts.

0a2fcfd

oandregal approved these changes Dec 2, 2022

View reviewed changes

spacedmonkey merged commit 5726f30 into trunk Dec 6, 2022

spacedmonkey deleted the fix/performance-issue branch December 6, 2022 14:09

github-actions bot added this to the Gutenberg 14.8 milestone Dec 6, 2022

spacedmonkey mentioned this pull request Dec 17, 2022

wp_get_global_stylesheet: substitute 1-minute transient by non-persistent object cache WordPress/wordpress-develop#3712

Closed

oandregal added a commit that referenced this pull request Dec 22, 2022

experimental: use gutenberg_get_legacy_theme_supports_for_theme_json

78c51b3

#46112

This was referenced Jan 24, 2023

Backport: WP_Theme_JSON_Resolver changes WordPress/wordpress-develop#3901

Closed

Remove get_default_block_editor_settings from theme JSON resolver WordPress/wordpress-develop#3902

Closed

Mamaduka removed the Needs PHP backport Needs PHP backport to Core label Jan 24, 2023

Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of get_default_block_editor_settings #46112

Remove usage of get_default_block_editor_settings #46112

spacedmonkey commented Nov 28, 2022 •

edited

Loading

felixarntz left a comment

felixarntz Nov 28, 2022

felixarntz Nov 28, 2022

felixarntz Nov 28, 2022

spacedmonkey Nov 29, 2022

spacedmonkey commented Nov 28, 2022

oandregal Dec 1, 2022

oandregal Dec 1, 2022

oandregal Dec 1, 2022

spacedmonkey Dec 1, 2022

oandregal Dec 1, 2022

oandregal Dec 1, 2022

oandregal Dec 1, 2022

oandregal Dec 1, 2022

oandregal Dec 1, 2022

spacedmonkey Dec 1, 2022

Mamaduka commented Jan 24, 2023

Remove usage of get_default_block_editor_settings #46112

Remove usage of get_default_block_editor_settings #46112

Conversation

spacedmonkey commented Nov 28, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Before

After

felixarntz left a 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

spacedmonkey commented Nov 28, 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

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

Mamaduka commented Jan 24, 2023

spacedmonkey commented Nov 28, 2022 •

edited

Loading