Fix presets data for themes that do not provide any preset by oandregal · Pull Request #36054 · WordPress/gutenberg

oandregal · 2021-10-28T16:01:53Z

This is a dormant bug that was uncovered by #35970 (comment)

The bug

In the core/block-editor store, we have the presets keyed by origin (core, theme, user). For example, the color palette is stored under __experimentalFeatures.color.palette as:

{
  core: [ { slug: 'value', color: 'value', name: 'value' } ],
  preset: [ { slug: 'value', color: 'value', name: 'value' } ],
  user: [ { slug: 'value', color: 'value', name: 'value' } ]
}

If a given origin has not provided data, there's no key for them. For themes that do not provide any preset and the user hasn't either, the presets are expected to have this shape:

{
  core: [ { slug: 'value', color: 'value', name: 'value' } ]
}

Storefront is one of those themes that do not provide any preset so should be expected to have the above shape. The same can be reproduced for any theme that has a theme.json by removing the settings.color.palette contents. By testing #35970 (comment) we found that these themes included in the theme origin the same data that already was in the core one:

{
  core: [ { slug: 'value', color: 'value', name: 'value' } ],
  theme: [ { slug: 'value', color: 'value', name: 'value' } ] // The values here are the same than core's.
}

How to test

Activate Storefront. You can also use the empty theme and remove the settings.color.palette property.
Inspect the contents of the core/block-editor store, specifically the settings.__experimentalFeatures.color.palette key.
The expected result is that you'll find only a core origin but instead there're core and theme.

Why it happens

WordPress 5.8 does the following (see get_block_editor_settings):

1.1. Takes the data from the theme (either from theme supports or theme.json).
1.2 It stores under settings.__experimentalFeatures the theme.json settings.
1.3. For backward compatibility, it also retrofits the data to the legacy settings mechanism (settings.colors for the palette, settings.gradients for the gradients, and so on). At this point, all themes have stored presets in the old legacy settings mechanism, whether it's core presets or presets coming from the theme.
1.4. Then, it fires the block editor settings filter, so 3rd parties can filter them.

The Gutenberg plugin hooks into that filter to reorganize the settings.__experimentalFeatures with the new schema and then retrofit again the data to the legacy settings mechanism. The bug is here, in the plugin. Because we use the filtered legacy settings (settings.color) to create the new ones (settings.__experimentalFeatures.color.palette) there's always data coming from settings.color that we interpret as coming from the "theme origin".

The fix is not taking into account the filtered legacy settings (settings.color).

The drawback to this fix is that the legacy settings can be filtered no longer, because the plugin will ignored them anyway.

For reference, there're less than a dozen, see here.

lib/global-styles.php

oandregal · 2021-10-29T09:00:23Z

I've got this thought: for the backward-compatible theme settings we retrofit plugins can still filter them via after_theme_setup. So we're not stopping them from being filterable. We need to communicate that so plugins can adapt, if necessary.

youknowriad

If I'm not wrong, this change means that all add_theme_support calls are ignored when constructing the "theme" global styles.

For me, it seems that it's not the right approach. My expectation is the following:

add_theme_support calls should be taken into consideration when building the "theme" global styles.
block_editor_settings should not be taken into consideration.

I tried following the code but I think the issue comes from the fact that the current code base is optimized for this flow:

theme-supports ----(transform)----> block-editor-settings -----( merge with raw theme.json )------> theme global styles

It seems that the solution should be that we do this instead

theme-supports --------( merge with raw theme.json ) -----> theme global styles.

Does that make sense?

youknowriad · 2021-10-29T11:50:54Z

I guess in other words, WP_Theme_JSON_Gutenberg::get_merged_data shouldn't be receiving any "block editor settings" in all situations but it should take into consideration the add_theme_support calls.

oandregal · 2021-10-29T12:05:43Z

I've just merged two PRs I had on top of this #36075 and #36081

oandregal · 2021-10-29T12:10:55Z

I guess in other words, WP_Theme_JSON_Gutenberg::get_merged_data shouldn't be receiving any "block editor settings" in all situations but it should take into consideration the add_theme_support calls.

Yeah, exactly, that's what this PR should do.

It is a change from how it worked until now but we need it to make the UI color control work as we want (show all origins but only if they have data). Plugins that were filtering the theme support settings we retrofit in theme.json via the block_editor_settings will need to be updated to do it via after_theme_setup instead.

Before this is ready, I still need to understand why some tests needed changing.

youknowriad · 2021-10-29T12:11:46Z

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

 	 * @return WP_Theme_JSON_Gutenberg
 	 */
-	public static function get_merged_data( $settings = array(), $origin = 'user' ) {
+	public static function get_merged_data( $origin = 'user' ) {


This change is great :)

youknowriad · 2021-10-29T12:13:59Z

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

+		* So we take theme supports, transform it to theme.json shape
+		* and merge the self::$theme upon that.
+		*/
+		$theme_support_data  = WP_Theme_JSON_Gutenberg::get_from_editor_settings( gutenberg_get_default_block_editor_settings() );


Can you help me understand what this line does? My understanding is the following: it uses the "add_theme_support" calls to build something like a "theme.json" tree based on these.

If that's correct, then we're aligned here. The thing that bother me though is that it still says "editor settings", it should be more something like:

$theme_support_data = WP_Theme_JSON_Gutenberg::get_from_theme_supports();

Yes, that's exactly what it does.

The current name is actually representative of what this function does at the moment, as gutenberg_get_default_block_editor_settings returns editor settings. However, I understand your point is that you'd rather inline the call to theme supports into WP_Theme_JSON.

This makes me think of the conversation about the endpoint and whether to allow getting data for the non-active themes. How can we retrieve the theme supports data of a non-active theme? Does the REST API request load the theme being requested? Or we won't have access to that data?

Good point, I guess for now, we'll limit that endpoint to the active theme. I think it's fine for our current usage.

youknowriad · 2021-10-29T12:17:36Z

lib/global-styles.php

+	$settings['__experimentalFeatures'] = gutenberg_get_global_settings();

 	if ( isset( $settings['__experimentalFeatures']['color']['palette'] ) ) {
 		$colors_by_origin   = $settings['__experimentalFeatures']['color']['palette'];


All the changes here make things a lot simpler, it's cool and also validates these higher level APIs.

youknowriad

This is looking good to me, I think we should be looking at removing the get_from_editor_settings function in favor of a get_from_theme_supports for clarity. But that's not a blocker.

Do not read them from the filtered block settings. This has the side-effect of not taking into account changes 3rd parties may have done to theme supports data. For example, adding support for link color, or changing the theme editor palette.

* Use gutenberg_get_global_* functions (#36075) * Remove settings as an argument from `WP_Theme_JSON_Resolver` (#36081)

oandregal · 2021-11-02T16:29:29Z

phpunit/class-wp-theme-json-resolver-test.php

-				'color'  => array(
-					'palette' => array(
+				'color'      => array(
+					'custom'         => false,


These tests now get the defaults for color.custom, color.customGradients, typography.customFontSize, typography.customLineHeight, spacing.units, and spacing.customPadding.

The reason is that the (legacy) theme supports are now pulled from the WP_Theme_JSON_Resolver_Gutenberg::get_theme_data() method directly, while, before, these were passed via an argument (and it was empty for the tests).

oandregal · 2021-11-03T10:42:12Z

Apparently, some PRs got merged in parallel and not all the uses of get_merged_data were migrated #36173

oandregal requested a review from spacedmonkey as a code owner October 28, 2021 16:01

This was referenced Oct 28, 2021

Add Theme's base global styles endpoint #35985

Merged

Update: Show all color and gradient origins (core, theme, and user). #35970

Merged

oandregal self-assigned this Oct 28, 2021

oandregal added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Oct 28, 2021

oandregal requested review from jorgefilipecosta and youknowriad October 28, 2021 16:06

oandregal commented Oct 28, 2021

View reviewed changes

lib/global-styles.php Outdated Show resolved Hide resolved

oandregal force-pushed the fix/settings-themes-no-theme-json branch from 82d9913 to 13685c7 Compare October 29, 2021 09:10

oandregal mentioned this pull request Oct 29, 2021

Adapt global styles code to use get global styles functions instead of low-level APIs #36075

Merged

youknowriad reviewed Oct 29, 2021

View reviewed changes

oandregal mentioned this pull request Oct 29, 2021

Make tests idempotent by removing theme support after having it added #36080

Closed

youknowriad reviewed Oct 29, 2021

View reviewed changes

oandregal force-pushed the fix/settings-themes-no-theme-json branch from 0ec76f8 to 6d9098c Compare November 2, 2021 12:13

youknowriad approved these changes Nov 2, 2021

View reviewed changes

oandregal added 5 commits November 2, 2021 17:05

Read theme supports directly from the theme

a6caba2

Do not read them from the filtered block settings. This has the side-effect of not taking into account changes 3rd parties may have done to theme supports data. For example, adding support for link color, or changing the theme editor palette.

Remove settings as an argument plus use gutenberg_get_global_* functions

a187a1d

* Use gutenberg_get_global_* functions (#36075) * Remove settings as an argument from `WP_Theme_JSON_Resolver` (#36081)

Focus tests on translations

cd33376

Remove unused settings from test

d374e3d

Update test dat

76f61ab

oandregal force-pushed the fix/settings-themes-no-theme-json branch from 6d9098c to 76f61ab Compare November 2, 2021 16:06

oandregal commented Nov 2, 2021

View reviewed changes

oandregal merged commit d5d524d into trunk Nov 2, 2021

oandregal deleted the fix/settings-themes-no-theme-json branch November 2, 2021 16:53

github-actions bot added this to the Gutenberg 11.9 milestone Nov 2, 2021

oandregal mentioned this pull request Nov 3, 2021

Hotfix for leftovers of WP_Theme_JSON_Resolver::get_merged_data #36173

Merged

oandregal mentioned this pull request Nov 9, 2021

Gutenberg 11.9: Not possible to filter the global style settings anymore #36330

Closed

Conversation

oandregal commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

How to test

Why it happens

Uh oh!

Uh oh!

oandregal commented Oct 29, 2021

Uh oh!

youknowriad left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Oct 29, 2021

Uh oh!

oandregal commented Oct 29, 2021

Uh oh!

oandregal commented Oct 29, 2021

Uh oh!

youknowriad Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

youknowriad Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

oandregal Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

youknowriad Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

youknowriad Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

oandregal Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

oandregal commented Nov 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oandregal commented Oct 28, 2021 •

edited

Loading

youknowriad left a comment •

edited

Loading