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

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

Merged
merged 25 commits into from
Dec 16, 2022

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Dec 15, 2022

What?

This PR proposes a different way to organize theme.json code.

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?

The current mechanism relies on inheritance. The code in Gutenberg inherits from the core class and modifies the relevant parts of it, scoped to a specific WordPress version so backports were easier. This means that we have:

  • The WP_Theme_JSON class that lives in the runtime.
  • The WP_Theme_JSON_6_1 class at lib/compat/wordpress-6.1 for features to be shipped in WordPress 6.1.
  • The WP_Theme_JSON_6_2 class at lib/compat/wordpress-6.2 for features to be shipped in WordPress 6.2.
  • The WP_Theme_JSON_Gutenberg class at lib/compat/experimental for features that are not ready to be shipped and/or as a placeholder. The whole Gutenberg codebase we should use WP_Theme_JSON_Gutenberg to have the latest changes.

This PR proposes that we have a single class, WP_Theme_JSON_Gutenberg, that does not inherit from the core one. Comparing the core and the Gutenberg classes is a matter of using a diff tool over a single file.

Questions

Is there a better setup?

I don't think the current setup is working for the reasons I mentioned above, so it's best if we change it. Happy to hear alternatives if people have them.

Where this single class should live?

Initially, this PR proposed to have it at lib/compat/wordpress-6.2. It was switched to live at lib/, which follows what we do for lib/block-supports and other files (client-assets.php).

Using the @since tag

The expectations for this class going forward should be that we use @since PHPDoc tag to document the changes like we do in WordPress core.

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.).

Next steps

I'd like to merge this one as soon as the tests pass. This is an active area of development and it the PR is up for more than a few days is going to consume a lot of time to keep it up with the changes.

The same should be done for WP_Theme_JSON_Resolver. Given this is a big change in terms of lines of code touched, I'd prefer to do it individually, to manage scope, if at all possible.

@oandregal oandregal self-assigned this Dec 15, 2022
@oandregal oandregal added the [Status] In Progress Tracking issues with work in progress label Dec 15, 2022
@oandregal oandregal changed the title Try: bundle theme.json class instead of inheriting per WordPress version Try: bundle WP_Theme_JSON class instead of inheriting per WordPress version Dec 15, 2022
@oandregal
Copy link
Member Author

oandregal commented Dec 16, 2022

The only thing I imagine that might be difficult is discerning what should go into the next release and what shouldn't.

Yeah. That's going to be something that we find in the resolver already (e.g.: webfonts). Some ideas:

  • There may be opportunities to use the current filters in place for extending the behavior of the resolver, though there will be situations where that's not enough.
  • Add comments in the code.
  • Inherit in experimental (it's different from before in that we're inheriting from the Gutenberg class, not core, so we won't have issues such as cached data).

Essentially, we need to look at how other areas deal with this. My thinking is that different situations may need different approaches.

By looking at the list of backports we missed in the 6.1 cycle, we cannot say the current approach is working or is better than this. This is the list of backports we missed (only considering the WP_Theme_JSON class, there may be more related to the resolver, etc.):

My point is that we may want to ship this and figure how to deal with the experimental stuff when we have a specific case (the resolver will have one already: webfonts).

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense to me. Simpler is usually better. I suggest we get it merged ASAP to avoid dealing with lots of conflicts.

@oandregal
Copy link
Member Author

OK, going to go ahead with this. Thanks all for the testing. I'll follow up with a PR for the resolver next week, and we can continue the conversation about experimental code over there.

@oandregal
Copy link
Member Author

Follow-up for the resolver at #46750

@noahtallen
Copy link
Member

noahtallen commented Dec 28, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants