Do not use init to register block style variations defined via theme.json

[oandregal]

Trac ticket https://core.trac.wordpress.org/ticket/61451
Trac ticket https://core.trac.wordpress.org/ticket/61312

What

Register the block style variation coming from theme.json (theme's theme.json, user's theme.json, and theme.json file partials) in the WP_Theme_JSON_Resolver.

Why

The init hook has proven problematic, because it's also fired during installation, see slack conversation cc @swissspidy.

How

The main advantage of this approach is that the block style variations are registered when the data is read, so we don't have to do any extra data reads or any more theme.json processing than we already do:

• theme & custom origins: 5d18c8c
• theme.json partials: 762b802

The disadvantage is that these block style variations are not registered everywhere, so we have to do that in the specific places that are not yet covered:

• In the global styles endpoint: 3ea8e91

Context

Initially, this feature worked by registering the block style variations using the wp_theme_json_data_* filters, see https://core.trac.wordpress.org/changeset/58264. This approach proved problematic for this flow: the user updates the styles of any of them via the global styles sidebar.

This feature was updated to use the init hook at https://core.trac.wordpress.org/changeset/58394 The rationale was that this hook is what themes currently do to register the block styles variations via the functions.php (example from TwentyTwentyFour). This approach raised issues: the init hook is called during installation, hence the database is not set up — which this feature requires, as it retrieves the existing global styles for user data.

The approach in this PR is similar to how it initially worked, but with a couple of tweaks:

• It doesn't use the wp_theme_json_data_* filters. Instead, this registers the variations directly in the WP_Theme_JSON_Resolver, saving the extra processing required by the filters.
• It also registers the block style variations in the global styles endpoint, so user updates are reflected immediately in the UI without requiring a page reload.

Test

1. Register style variations in all the places the theme is allowed to (use TwentyTwentyFour, aka TT4).

In the theme.json file of the theme, paste the following under styles.blocks.variations:

"theme": {
   "title": "Theme",
    "blockTypes": [ "core/group" ],
    "color": {
            "background": "crimson"
    }
}

In the styles/ember.json (theme style variation) file, paste the following under styles.blocks.variations:

"theme": {
   "title": "Theme",
    "blockTypes": [ "core/group" ],
    "color": {
            "background": "darkseagreen"
    }
}

Create a new file called styles/partial.json (partial theme.json), and paste the following:

{
        "$schema": "https://schemas.wp.org/trunk/theme.json",
        "version": 2,
        "title": "Partial",
        "blockTypes": [ "core/group" ],
        "styles": {
                "color": {
                        "background": "deepskyblue"
                }
        }
}

2. Go to the post editor, add two group blocks. Verify the "Theme" and "Partial" variations are present:

3. Apply them to each of the blocks and verify colors are reflected properly everywhere (editor & front):

4. Go to the site editor and apply the "Ember" style variation. The expected result is that the block with the "Theme" block variation has now the "darkseagreen" background color:

5. Verify that user changes to the block style variations defined are reflected immediately in the site editor:

Gravacao.do.ecra.2024-06-17.as.18.12.25.mov

Commit

Commit message, to make it easy for committers:

Do not use init to register block style variations defined via theme.json.

Props oandregal, aaronrobertshaw, joemcgill, ramonopoly, andrewserong, swissspidy.
See #61451.
Fixes #61312.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props oandregal, aaronrobertshaw, joemcgill, ramonopoly, andrewserong.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[joemcgill]

Thanks, @oandregal. I'm not sure I like the idea of adding side effects to the methods that are intended to return the parsed WP_Theme_JSON objects from each origin.

I think the previous approach of having a dedicated function that has the responsibility for registering style variations is a much better design, we just need to find a better way to handle when that function is called, since the init hook is too broad, and even then, only really because of the need to get user data.

I think adding an early return to wp_register_block_style_variations_from_theme() if wp_installing() returns true is probably a better initial improvement. We're previously taken this approach to avoid building variations of the template part block (see ref).

Can you provide more context about how this variation data gets used? If it's mainly for ensuring styles are enqueued, perhaps we could do this on the enqueue_block_assets hook at priority 9, so these get registered just before wp_enqueue_registered_block_scripts_and_styles() is called.

[aaronrobertshaw]

I'm not sure I like the idea of adding side effects to the methods that are intended to return the parsed WP_Theme_JSON objects from each origin.

Agreed, this aspect isn't ideal.

I think the previous approach of having a dedicated function that has the responsibility for registering style variations is a much better design

This was the thinking behind the original switch to using the init action to register block style variations. As you noted though it is too broad and ironically has come with a number of its own side effects 😅

I think adding an early return to wp_register_block_style_variations_from_theme() if wp_installing() returns true is probably a better initial improvement.

This could be a good fallback option. One of the problems with leveraging init is that we need to know all the scenarios where we don't want this registration to occur and that is proving difficult. In my view, there's a much smaller set of use cases where the registration needs to happen than not.

Leaving the side-effect issue aside, the approach in this PR suffers the flip side of the above issue. We need to know the scenarios in which block style variations should be registered. This is much more easily reconciled and reasoned about. The happy path for the feature can easily tested.

Can you provide more context about how this variation data gets used?

The block style variations need to be registered to the block styles registry so that their data is not incorrectly stripped during the sanitization of theme.json data. In this context the two are fairly closely linked. This sanitization of variations was deemed important to maintain from a security perspective.

If it's mainly for ensuring styles are enqueued, perhaps we could do this on the enqueue_block_assets hook at priority 9

Unfortunately, there is a little more to it than that. As noted, the registration is also required for the sanitization of theme.json data not just ensuring stylesheets are enqueued. André and I have at different times in the implementation of the feature explored different actions and filters to hook onto but haven't found a workable alternative to init.

---

I'd like to stress that I agree that having registration as a side effect of processing the theme.json data isn't ideal. However, I think for beta 3, we are best served to proceed with the approach in this PR.

This is on a couple of grounds:

• It doesn't introduce any public method/new API/etc.
• It could be considered an implementation detail which leaves the door open to switch back to init if desired once all the edge cases and unknowns can be methodically worked through
• It limits the work done to only where and when we need it

The downside other than the design involving a side effect is that we need to ensure the registration occurs everywhere needed. This can be offset by ensuring the happy path for the feature is fully functional. I believe the testing of the feature prior to the switch to using init provides some confidence here.

Ultimately, this PR's approach helps lower our exposure to unexpected consequences of leveraging init and reduces the potential impact of bugs this late in the release cycle. We are still free to iterate further, even post 6.6, if there are no public functions and APIs added.

[andrewserong]

On balance, I agree, I think registering block variations as a side effect of retrieving resolved theme JSON data is preferable to using init as the catch all for registration. I also like the idea of it being an implementation detail so it can continue to be refactored if we think it warrants it after the fact.

[ramonjd]

Ultimately, this PR's approach helps lower our exposure to unexpected consequences of leveraging init and reduces the potential impact of bugs this late in the release cycle. We are still free to iterate further, even post 6.6, if there are no public functions and APIs added.

My reading is that the pros of this PR's approach outnumber the current, demonstrated pros of using the init or any other hook.

Furthermore, the limitations — the introduction of side-effects to these methods — are recognized, and are also isolated to specific methods, which appears to me to be more information than thus far is known about the consequences of registering variations in the 'init' hook.

Therefore it sounds like a viable path to document these limitations and come up with a plan to iterate if folks want this feature to make it for 6.6.

I'd like to stress that I agree that having registration as a side effect of processing the theme.json data isn't ideal.

Just so I understand, in the case of the Global Styles controller, is the processing of theme.json data required to generate an accurate REST transaction?

[aaronrobertshaw]

Just so I understand, in the case of the Global Styles controller, is the processing of theme.json data required to generate an accurate REST transaction?

What's really needed for an accurate REST transaction is for block style variations to be registered at the time the global styles post type is saved as well as prepared for response. Without being registered the block style variation's data will be sanitized and stripped out, creating the issue/inaccuracy.

Any variations registered within the theme data, need to be registered here, in addition to the existing registration from the user origin. As the theme data can be filtered, I think it the retrieval via get_theme_data, is required.

[ramonjd]

Without being registered the block style variation's data will be sanitized and stripped out, creating the issue/inaccuracy.

Thanks for the explainer. I was just trying to square it in my brain whether, in the case of the controller, it's really a
side-effect given in the input and output values.

[aaronrobertshaw]

I've created a matching Gutenberg PR for these changes: WordPress/gutenberg#62640

Initial smoke tests look good. I'll start giving it a thorough run and review shortly.

[aaronrobertshaw]

I've given this a deeper test. It's all working as advertised.

After throwing several rounds of more contrived variation partials, data etc., I didn't encounter any issues.

✅ Variations register correctly and are available in both editors
✅ Customizations to variations in global styles are reflected in the editor
✅ Variation customizations are saved and applied on the frontend correctly

As discussed in this comment, we should proceed with this change. It appears we have consensus on that approach as well, with a more deliberate revisiting of init in the near future.

Screen.Recording.2024-06-18.at.3.52.09.PM.mp4

[aaronrobertshaw]

After the latest tweaks I've given this another run. It tests well still and the comment tweaks are a nice touch. Thanks!

LGTM 🚢

[oandregal]

The different alternatives have pros/cons, none is perfect. Given what people has shared and the current time-constraints, I agree we should go ahead with this approach.

[oandregal]

Committed at https://core.trac.wordpress.org/changeset/58429

[oandregal]

In terms of performance, the Front-End tests for TwentyTwenty Four do show a consistent and slight decrease for wpTotal and timeToFirstByte: