Try: bundle the `WP_Theme_JSON_Resolver` class instead of inheriting per WordPress version #46750

oandregal · 2022-12-22T16:37:42Z

What?

This PR bundles the WP_Theme_JSON_Resolver_Gutenberg class into a single file, continuing the work done for the WP_Theme_JSON class at #46579

Why?

The existing mechanism uses an inheritance model, where Gutenberg modifies only certain parts of WordPress. This approach:

has proven buggy: we've missed many backports.
is difficult to maintain: syncing core<=>Gutenberg is time-consuming and error-prone.
does not allow us to do certain things, for example, remove specific methods, change the signature, etc.
we've done some hacks to clean the static variables set by core: 42756 and 42776.

How?

This PR creates a single file under lib/ for:

WP_Theme_JSON_Resolver_Gutenberg
theme.json
theme-i18n.json

Testing Instructions

Given this moves code from one place to another, it's difficult to provide a test plan. The fact that automated tests pass gives us some certainty the main paths are working.

This is what I've done, though you may want to try things you're most familiar with:

Theme with theme.json (TwentyTwentyThree):

In the site editor, make some changes to the global styles (e.g.: select a different style variation) and verify they are applied everywhere (editor, front-end).
In the site editor, I've added a new color to the global palette and updated an existing one.
In the editors, verify that presets (typography, colors, etc.) are properly listed (no values missing, etc.).

Theme without theme.json (TwentyTwenty):

Load the theme in the editor and front-end. Verify that it works as before.
In the editors, verify that presets (typography, colors, etc.) are properly listed (no values missing, etc.).

Questions

What to do with experimental code

Experimental code is code that is not ready for backport yet. So far, we've only run into 1 use case I know of: webfonts. For this use case, we only need to call gutenberg_add_registered_webfonts_to_theme_json in two places (I've marked them as comments in the PR).

In this PR, I propose we simply mark this code as experimental via comments. It's simple enough and works well for this case.

An alternative would be keeping the experimental class and override the get_theme_data method. This is what we did now and I'd try to avoid it. We missed a couple of things using this system (and many others in the WP_Theme_JSON class):

The theme support for appearanceTools missed the window for being backported to WordPress 6.1.
The Gutenberg code didn't have changes from core (such as translating the parent theme of a child theme or reorganizing how default parameters work).

spacedmonkey

YES! One file to port. Let's do this for all files going forward.

hellofromtonya · 2022-12-22T17:13:06Z

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

+			// BEGIN OF EXPERIMENTAL CODE. Not to backport to core.
+			$theme_json_data = gutenberg_add_registered_webfonts_to_theme_json( $theme_json_data );
+			// END OF EXPERIMENTAL CODE.


I'm doing something similar in the (Web) Fonts API, i.e. wrapping code blocks in inline comments to alert not to backport to Core. I used:

BACKPORT NOTE: <explanation>

How to mark "don't backport" code though likely should be standardized across the codebase. For example, may want to add an annotation such as @backportNote, @doNotBackport, etc. Though this is likely best discussed in a separate issue and then added to the docs.

spacedmonkey · 2022-12-22T17:14:20Z

Can we do this for other classes in this repo. It could go a long way to help porting files between the repos.

scruffian

Tested in Loudness:
✅ Changing the background and text color
✅ Using refs in theme.json
✅ Using hover in theme.json

Also
✅ Changing the core theme.json file
✅ Changing settings in Global Styles, saving and reloading the frontend.

scruffian · 2022-12-22T17:16:53Z

Lets get this in ASAP to avoid having to deal with the conflicts

hellofromtonya · 2022-12-22T17:19:08Z

I too like the idea of bundling all changes into 1 class and file. As you note, it'll ease backporting and maintaining efforts.

On the Core side though, need to think about how to keep this file synchronized with what's in Core. Why? The WP_Theme_JSON_Resolver can and will change independently in Core. Since this PR no longer extends from Core's class, any changes made in Core to fix bugs will need to ported back to Gutenberg to keep both in sync. Why?

To avoid unexpected behaviors when testing and further developing here.
To ease conflicts at merge into Core time.

I don't yet have an answer of how to keep things synchronized. But flagging it here for awareness and further thinking.

oandregal · 2022-12-22T17:24:23Z

On the Core side though, need to think about how to keep this file synchronized with what's in Core. Why? The WP_Theme_JSON_Resolver can and will change independently in Core.

Yeah. Ideally, all changes are made in Gutenberg, first. Though I understand that this is a conversation larger than this PR and can also benefit/require some automation, in addition to clarify around flows.

Since this PR no longer extends from Core's class, any changes made in Core to fix bugs will need to ported back to Gutenberg to keep both in sync. Why?

Even with the previous setup, most of the methods were overridden, so most changes in core were ignored. Not inheriting also gives us peace of mind to avoid running into unexpected conflicts with some sites running the nightlies.

oandregal · 2022-12-22T17:52:21Z

Tested the TwentyTwentyThree theme in a variety of scenarios: updated user data (via global styles sidebar), updated theme data (via theme.json of the theme), and update core data (theme.json bundled with gutenberg). Everything works as expected.

oandregal · 2022-12-22T17:58:25Z

Tested TwentyTwenty, including modifying some values from the theme.json bundled with Gutenberg. Works as expected.

oandregal · 2022-12-22T17:58:40Z

Going to merge this one as soon as tests pass.

oandregal · 2022-12-22T18:09:43Z

Thanks everyone for the quick turnaround on this one :)

noahtallen · 2022-12-28T21:45:32Z

I'm including this in a patch release of 14.8, because it is required for compatibility with WordPress 6.0. I looked through the PR changes, and each deleted file was not modified after 14.8 was released. Assuming this PR is mostly a refactor (maintaining functionality rather than changing how the feature works), including it in 14.8 won't functionally change how 14.8 works. See #46811 for more info

… 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/

oandregal requested a review from spacedmonkey as a code owner December 22, 2022 16:37

oandregal self-assigned this Dec 22, 2022

oandregal added Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Code Quality Issues or PRs that relate to code quality [Status] In Progress Tracking issues with work in progress labels Dec 22, 2022

spacedmonkey approved these changes Dec 22, 2022

View reviewed changes

oandregal added 3 commits December 22, 2022 17:55

Backport WP_Theme_JSON_Resolver from core as it is

96c0239

Substitute WP_Theme_JSON by WP_Theme_JSON_Gutenberg

13185e2

Substitute WP_Theme_JSON_Data by WP_Theme_JSON_Data_Gutenberg

1f5ad2d

oandregal added 2 commits December 22, 2022 17:55

Move theme.json to unversioned lib/

9938731

Move theme-i18n.json to unversioned lib/

703ccaf

oandregal force-pushed the try/bundle-theme-json-resolver-class branch from 52fdc71 to 703ccaf Compare December 22, 2022 16:58

oandregal removed the [Status] In Progress Tracking issues with work in progress label Dec 22, 2022

oandregal mentioned this pull request Dec 22, 2022

Try: bundle WP_Theme_JSON class instead of inheriting per WordPress version #46579

Merged

hellofromtonya reviewed Dec 22, 2022

View reviewed changes

scruffian approved these changes Dec 22, 2022

View reviewed changes

oandregal merged commit c365a80 into trunk Dec 22, 2022

oandregal deleted the try/bundle-theme-json-resolver-class branch December 22, 2022 18:09

github-actions bot added this to the Gutenberg 14.9 milestone Dec 22, 2022

oandregal mentioned this pull request Dec 22, 2022

Code quality check for theme.json related APIs #45171

Open

noahtallen mentioned this pull request Dec 28, 2022

Gutenberg 14.8.0+ compat issue with WordPress < 6.1 #46811

Closed

noahtallen modified the milestones: Gutenberg 14.9, Gutenberg 14.8 Dec 28, 2022

oandregal mentioned this pull request Dec 29, 2022

PHP: backport from 6.1 again. #46363

Closed

Mamaduka mentioned this pull request Jan 16, 2023

Plugin: Backport PHP changes for WordPress 6.2 release #47187

Closed

85 tasks

glendaviesnz mentioned this pull request Mar 5, 2023

Block Supports: Add text columns (column count) to typography block supports #33587

Merged

Mamaduka mentioned this pull request Apr 25, 2023

Change the 'WP_Theme_JSON_Data_Gutenberg' class directory #50062

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try: bundle the `WP_Theme_JSON_Resolver` class instead of inheriting per WordPress version #46750

Try: bundle the `WP_Theme_JSON_Resolver` class instead of inheriting per WordPress version #46750

oandregal commented Dec 22, 2022 •

edited

Loading

spacedmonkey left a comment

hellofromtonya Dec 22, 2022

spacedmonkey commented Dec 22, 2022

scruffian left a comment

scruffian commented Dec 22, 2022

hellofromtonya commented Dec 22, 2022

oandregal commented Dec 22, 2022

oandregal commented Dec 22, 2022

oandregal commented Dec 22, 2022

oandregal commented Dec 22, 2022

oandregal commented Dec 22, 2022

noahtallen commented Dec 28, 2022 •

edited

Loading

Try: bundle the WP_Theme_JSON_Resolver class instead of inheriting per WordPress version #46750

Try: bundle the WP_Theme_JSON_Resolver class instead of inheriting per WordPress version #46750

Conversation

oandregal commented Dec 22, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Questions

spacedmonkey left a comment

Choose a reason for hiding this comment

hellofromtonya Dec 22, 2022

Choose a reason for hiding this comment

spacedmonkey commented Dec 22, 2022

scruffian left a comment

Choose a reason for hiding this comment

scruffian commented Dec 22, 2022

hellofromtonya commented Dec 22, 2022

oandregal commented Dec 22, 2022

oandregal commented Dec 22, 2022

oandregal commented Dec 22, 2022

oandregal commented Dec 22, 2022

oandregal commented Dec 22, 2022

noahtallen commented Dec 28, 2022 • edited Loading

Try: bundle the `WP_Theme_JSON_Resolver` class instead of inheriting per WordPress version #46750

Try: bundle the `WP_Theme_JSON_Resolver` class instead of inheriting per WordPress version #46750

oandregal commented Dec 22, 2022 •

edited

Loading

noahtallen commented Dec 28, 2022 •

edited

Loading