search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.49k stars 4.19k forks source link

Streamline the way `WP_Theme_JSON` related classes and helpers are maintained #62594

Open joemcgill opened 4 months ago

joemcgill commented 4 months ago

What problem does this address?

There are a number of classes and functions related to the WP_Theme_JSON system that are developed in this repo but then are synced to https://github.com/WordPress/wordpress-develop/ during each release cycle. Currently the way this is maintained is by having duplicate files in both repos and then manually syncing changes made in this repo to the WordPress Core codebase during release cycles.

This adds cognitive overhead for maintainers of these systems, as well as a lot of manual labor, since you need to manually keep changes in sync and ensure any modifications made in either repo get additionally made in the relevant files in the other. We should streamline our process for maintaining these files to reduce this complexity while retaining the ability to iterate on these APIs and test them in Gutenberg plugin releases.

Relevant code:

Background

When these files were initially introduced to WordPress Core in version 5.8, this repo maintained separate classes that extended the Core classes in the relevant compat/{version} directories. However, as implemented, that strategy also proved to be overly complex to maintain, leading to these files being consolidated under the /lib directory (see: https://github.com/WordPress/gutenberg/pull/46579).

What is your proposed solution?

There are a few different ways that we could consider simplifying things, each with tradeoffs, but to avoid duplication we first would need to decide which repo should be considered the source of truth for these files and implement a strategy around that decision. Here are the main options that I see, but I'm offering them just to start discussion, not to prescribe a solution.

Maintain in Core, extend in Gutenberg

There are several tactics that could be used here, but generally speaking, this would mean that any enhancements to these APIs being worked on in this repo would need to be implemented via filters to modify the relevant Core code, or by extending the Core classes so that core's class methods get overridden by the Gutenberg class.

To achieve this, we would need to introduce a way for the plugin to replace all places where Core would use it's own classes with the child class. This is commonly achieved in PHP with service containers (also referred to as dependency injection containers), but we also already do similar types of things in WordPress to allow extenders to replace a Core class with their own. See _wp_image_editor_choose or rest_get_server as examples.

We would also need to swap out direct calls to classes throughout the codebase like:

$tree = WP_Theme_JSON_Resolver::get_theme_data( array(), array( 'with_supports' => false ) );

To first get the active class like this (pseudo example):

$container = new WP_Theme_JSON_Container();
$resolver = $container->get( 'resolver' );
$tree = $resolver->get_theme_data( array(), array( 'with_supports' => false ) );

The main advantage to this approach is that this API can continue to be developed in this repo but avoid compatibility issues during a development cycle when the Core versions of these classes change in WordPress trunk (example). It would also allow for more customization of these features by others who want to extend these APIs.

The disadvantages are:

Maintain in Gutenberg, sync only to Core

In this scenario, the files in WP Core would essentially become "read only" and we would only modify these files in the Gutenberg repo and sync them to WP during releases similar to the way blocks are now synced. Exactly what strategy we use to manage the syncing process can be worked out (e.g., CI automation, publish these files as packages, etc.) but we would still need to solve how to ensure the Gutenberg classes are fully taking over responsibility for this functionality when being tested in the plugin.

The main advantages to this are:

The disadvantages are:

Next steps

I'd love to get feedback from folks who regularly work on these APIs (e.g., @tellthemachines, @oandregal, etc.) about what would improve their experience of maintaining this code. I'd also be interested in hearing from @ellatrix and previous editor release leads who have had to manage the PHP syncs process to make sure we're considering common friction points that come up during release syncs that could be reduced through this effort.

ramonjd commented 4 months ago

Thanks for the issue. It's a pretty tricky area.

I think things have improved slightly since Gutenberg started loading local copies of the usual suspects — the Theme_JSON class family, block supports, and, to an extent, global styles REST controllers. These files were updated so constantly, with inherited compatibility classes inheriting other inherited compatibility classes... it was arduous to maintain and sometimes led to errors.

I'm not arguing to keep the status quo, there's definitely room to optimize the workflow, but one advantage I do see in manual backporting is that it obliges developers to test isolated changes in both environments.

With a package update from Gutenberg to Core for example, I'd imagine there'd be several features to test in Core.

This includes unit tests, as the test environments differ as well.

So along with all this, a testing strategy would also be great.

Any features tested through the Gutenberg plugin would still need to be implemented as separate PRs to the Core repo

I'd lean towards Gutenberg as the location where these classes/functions are maintained. Feature development is certainly faster and more dynamic.

Also experimental features have time in the plugin to "stabilize" to some extent.

Speaking of experimental features, whatever the solution, it would be also helpful to developers to have a preferred, compatible way to add experimental changes to these classes/functions in the plugin. Such changes often live in the plugin over several WordPress release cycles. I know it might be obvious, e.g., use filters or class inheritance, but it'd foster consistency I'd hope.

we would still need to solve how to ensure the Gutenberg classes are fully taking over responsibility for this functionality when being tested in the plugin.

Could we preserve the _Gutenberg prefixes in the plugin and strip them out when publishing the package (assuming packages are the way to go)?

The plugin does it for a few classes already.

Or 😱 namespaces? 😄

Updates to these features are not testable in WordPress nightly releases or to people developing in Core trunk until late in release cycles, which packages are generally synced for a release.

The github backport workflow I think should help to some extent — this CI check forces Gutenberg PR authors to have a corresponding Core patch prepared before merge. But it helps only insofar as these Core PRs are tested and committed in a timely manner.

tellthemachines commented 4 months ago

Thanks for opening this discussion @joemcgill !

Speaking as both a maintainer of some of these troublesome classes and a former release lead, my favoured approach here would be maintaining in Gutenberg and auto-syncing to core, like we currently do with the PHP files in block-library.

The major advantage to this is removing the need for manual sync of PHP across repos. This will make the release process so much easier, and it will do much to solve one of the potential cons:

Updates to these features are not testable in WordPress nightly releases or to people developing in Core trunk until late in release cycles, which packages are generally synced for a release.

Packages are often updated late in the cycle because they depend on PHP changes that have to be manually synced. If we automate most of the PHP sync, we make it easier to update the packages earlier in the cycle!

we would still need to solve how to ensure the Gutenberg classes are fully taking over responsibility for this functionality when being tested in the plugin.

Could we preserve the _Gutenberg prefixes in the plugin and strip them out when publishing the package (assuming packages are the way to go)?

Ideally we'd use the same approach we currently use in block-library , which is to prefix all its functions with gutenberg_ at build time.

Extending Core classes during Gutenberg development still requires using filters that can cause compatibility issues since we're not fully overriding these classes with Gutenberg versions.

This wouldn't be fully solvable without a monorepo 😅 but if we improve things for the classes that are changed most frequently, the rest should be manageable.

Individual changes to these APIs are not easily traceable in our SVN (or GitHub mirror) commit history

I think this is a great reason to be more deliberate with using changelogs in Gutenberg. We still have the Gutenberg commit history of course, but a changelog published with the npm packages will be easier to check without having to be across multiple repos.

joemcgill commented 3 months ago

A new Trac ticket has just been opened on the Core side that discusses this same issue: https://core.trac.wordpress.org/ticket/61696

spacedmonkey commented 3 months ago

I would normally agree that we should keep the "point truth" version of the class in this repo and just copy it over. However, these classes have versions, like Gutenberg_REST_Templates_Controller_6_4, which is the 6.4 version of template controller, here is the 6.6 version of the template controller Gutenberg_REST_Templates_Controller_6_6.

Because these classes are condionally loaded depending on WordPress versions, a module, where the is a filter in core for the class name, seems like the other way this could work it.

There are already examples in the gutenberg repo of where filter have been used to high jack core classes. Consider this example.

https://github.com/WordPress/gutenberg/blob/0616cf0268a768bd9357a491f2dd3091b70e6240/lib/rest-api.php#L84-L93

That changes the menu item REST API controller.

There is also this example filter in core. 

function parse_blocks( $content ) {
        /**
         * Filter to allow plugins to replace the server-side block parser.
         *
         * @since 5.0.0
         *
         * @param string $parser_class Name of block parser class.
         */
        $parser_class = apply_filters( 'block_parser_class', 'WP_Block_Parser' );

        $parser = new $parser_class();
        return $parser->parse( $content );
}

This is a pattern that has already been setup in core and would mean that gutenberg and other plugins could use these filters.

joemcgill commented 3 months ago

Unlike the Gutenberg_REST_Templates_Controler classes, the WP_Theme_JSON_ classes are no longer versioned, since #46579 because they were hard to maintain.

Even so, I would prefer that we have a way of overriding these classes consistently in Core, as you are suggesting, so that we can more easily and consistently test the code paths using the Gutenberg classes during development. Currently, when WordPress runs with Gutenberg installed, the code paths that are run include parts of the core classes and parts of the overridden classes, because we lack the filtering to override the entire system. This can lead to very strange bugs when (for example) static methods of WP_Theme_JSON_Resolver are called, and interact with WP_Theme_JSON_Gutenberg object type that has a different schema than expected.

Rather than calling static methods directly in core, we should introduce a form of dependency containment, where the application can retrieve the registered theme.json resolver and run static methods of that class. Here's some quick pseudocode:

wp_get_theme_json_resolver() {
  static $resolver = null;

  if ( null === $resolver ) {
    $class = apply_filters( 'wp_theme_json_resolver', 'WP_Theme_JSON_Resolver' );
    $resolver = new $class();
  }

  if ( ! $resolver typeof 'WP_Theme_JSON_Resolver' ) {
    // Throw an error.
  }

  return $resolver;
}

wp_get_wp_theme_json_data( $origin = null ) {
  $theme_json_resolver = wp_get_theme_json_resolver();

  return $theme_json_resolver::get_merged_data( $origin );
}

Then this plugin would need to ensure that WP_Theme_JSON_Resolver_Gutenberg extended the WP_Theme_JSON_Resolver class and filter wp_theme_json_resolver.

We could do something similar for the other classes.

oandregal commented 2 months ago

@joemcgill thanks for the thorough analysis, that's helpful.

The way I see it, the crux of the issue is having two places to edit the same files: the sync is easy if we only have one (and it could even be automated, something we couldn't do until now because there were changes happening in core).

Styles (and global styles) are an active area of development that happens in Gutenberg. This involves changes in JavaScript and PHP files — sometimes, PHP changes are standalone, other times they need to be synced with JS changes. I don't anticipate this to be different anytime soon, so Gutenberg is going to need to modify those files.

This is the same situation we have for blocks and we treat Gutenberg as the source of truth for that reason. If we go with "Maintain in Core, extend in Gutenberg" the files would still be editable in two places: we'd have the issue, just with more filters or different ways to instantiate them. Given this context, I favor the "Maintain in Gutenberg, sync only to Core" approach.

ramonjd commented 2 months ago

Styles (and global styles) are an active area of development that happens in Gutenberg.

Slightly, but not directly related to this issue, I believe there is some styles-related utility logic that could be abstracted away from WP_Theme_JSON.

Maybe some other specific themed logic as well 🤷🏻

WP_Theme_JSON itself is very large and difficult to grok for newcomers, and, I'd wager, its size often contributes the friction one experiences when synching changes between repos. Not to mention the fact that, as it ages, it grows even bigger as it accommodates changes in the way WordPress expresses CSS/interprets theme.json.

oandregal commented 2 months ago

WP_Theme_JSON itself is very large and difficult to grok for newcomers, and, I'd wager, its size often contributes the friction one experiences when synching changes between repos. Not to mention the fact that, as it ages, it grows even bigger as it accommodates changes in the way WordPress expresses CSS/interprets theme.json.

I agree with this sentiment. My view is that WP_Theme_JSON should deal with the core functions (sanitization, merging, etc) and delegate the style generation to other pieces.

ramonjd commented 1 month ago

I agree with this sentiment. My view is that WP_Theme_JSON should deal with the core functions (sanitization, merging, etc) and delegate the style generation to other pieces.

I might create a separate issue to capture ideas.