Global styles don't work for most blocks (paragraphs, headings, buttons etc).

[jorgefilipecosta]

With WordPress 6.1 beta one and the latest version on the trunk, global styles don't work for some blocks.

Things work well when the Gutenberg plugin is enabled, so this is a core-only issue.

Replicate

• Go to the site editor.
• Open the global styles sidebar.
• Open the blocks section.
• Navigate to the paragraph block (or button, heading, etc, I think most static blocks).
• Apply a background color.
• Save the global styles.
• The background color is wholly discarded on save.

What's happening

WP_Theme_JSON::get_blocks_metadata() is not returning all the blocks

Can be checked with the following debug code:

<?php
$r = new ReflectionMethod('WP_Theme_JSON', 'get_blocks_metadata');
$r->setAccessible(true);
var_dump( $r->invoke(new WP_Theme_JSON() ) );
exit;

Blocks like paragraphs, heading, and buttons are missing.
As the blocks are not known to the global styles system during the sanitization static::sanitize( $theme_json, $valid_block_names, $valid_element_names ); the styles of these blocks are removed and not saved.

There is a call to WP_Theme_JSON::get_blocks_metadata() before all the blocks are registed

WP_Theme_JSON::get_blocks_metadata() has a caching mechanism and not does its computations multiple times.
On the first call that happens to the function, the blocks are not all registered yet.

<?php
        $registry = WP_Block_Type_Registry::get_instance();
        $blocks   = $registry->get_all_registered();

On the first call of get_blocks_metadata blocks do not yet contain all the blocks. As the function is cached, some blocks don't exist at all for the global styles system.

It seems there was a change in how blocks are registered that is causing this issue.

[tan007]

I could reproduce the issue and have opened an issue on the Gutenberg repo here: ​https://github.com/WordPress/gutenberg/issues/44434 as suggested.

[andrewserong]

Thanks for writing this up @jorgefilipecosta! I can reproduce the issue, too. I did a little hacking around and found that in TwentyTwentyTwo theme, the only block styles that appear to be output in global styles are dynamic blocks, in case that helps hone in on the issue at all.

Also, interestingly, if I update the init action for register_core_block_types_from_metadata to have a priority of 1, that seems to get all the global styles in TwentyTwentyTwo theme working again (most obviously, the Button block). This is the line: ​https://github.com/WordPress/wordpress-develop/blob/537e5239f94853049ce84e815ddf15f8ecf7d259/src/wp-includes/blocks/index.php#L29

So, that does seem to point toward a caching issue / order of calls of some kind, as you've described above.

This is an attempt to resolve the issue where global styles for static blocks are not being output in the WordPress 6.1 beta. There is discussion behind why this issue occurred over in ​https://github.com/WordPress/gutenberg/issues/44434#issuecomment-1260270326. The short version is that the Template Part block calls get_block_templates, a public function which has a side-effect of caching the current list of registered blocks. The Template Part block calls this at registration time, so unless we clear the cache after calling it, the Theme JSON classes will not recognise subsequently registered blocks.

The explored fix here is to add a mechanism to also clear the static cache in WP_Theme_JSON and clear the cache for both this and WP_Theme_JSON_Resolver once the Template Part block has concluded getting the information it requires to generate the list of variations.

Trac ticket: https://core.trac.wordpress.org/ticket/56644

See also related Github issue: ​https://github.com/WordPress/gutenberg/issues/44434

### How to test

A simple way to test whether this fix works, is to add a few Button blocks to a post in TwentyTwentyTwo theme. See the below before and after and note that in the after screenshot, the theme's theme.json styles for the Button block are correctly output to the site frontend.

### A note on this fix

A better fix might to come up with a more generalised solution for this, or to update how get_block_templates works to ensure data isn't cached? The goal with this PR is to try to find a pragmatic but good enough short-term fix. If this looks viable, I'm happy to have a go at adding tests, too — though, I'm not 100% sure how to reliably test for it, so would be very happy for any feedback.

| Before | After |
| --- | --- |
| | |

Update: it looks like the E2E test is failing because I'm attempting to update version controlled files that exist in Gutenberg (template-part.php). Given that the Gutenberg version would likely call WP_Theme_JSON_Gutenberg instead of WP_Theme_JSON, if this fix winds up being appropriate, we might need to ignore that failure, and separately update the Gutenberg version of the file?

Nice job @andrewserong!
It works as expected.

• The Gutenberg plugin is not installed. No other plugins installed on the site.
• WP Version: 6.1-beta2-54337-src
• TT3 Theme
• PHP 7.4.30

​https://user-images.githubusercontent.com/37012961/192767434-ddf5a8c2-dbb0-4a39-a83a-88cfe125eb30.mov

I will take a look at the E2E test in the Gutenberg repo.

Update: it looks like the E2E test is failing because I'm attempting to update version controlled files that exist in Gutenberg (template-part.php).

So we will need to apply the fix to the Template Part block in GB, as dynamic blocks' code is automatically synced to Core -- so any changes here will be overridden.

Given that the Gutenberg version would likely call WP_Theme_JSON_Gutenberg instead of WP_Theme_JSON, if this fix winds up being appropriate, we might need to ignore that failure, and separately update the Gutenberg version of the file?

AFAIK, generally speaking, block code isn't supposed to call a Static_Class::member, but instead wp_some_kind_of_wrapper_function. (We have some logic to replace the wp_ prefix by gutenberg_ during our build process in GB). We have some precedent for this, see e.g. ​https://github.com/WordPress/gutenberg/pull/44099 and ​https://github.com/WordPress/wordpress-develop/pull/3244.

Tentative fix for 56644. Alternative to ​https://github.com/WordPress/wordpress-develop/pull/3352.

Trac ticket: https://core.trac.wordpress.org/ticket/56644

I've filed an alternative fix here: ​https://github.com/WordPress/wordpress-develop/pull/3359 😊

Ah, good spot, @andrewserong! Turns out I only tested for user modifications (setting the background color of the button block in the Global Styles menu) but forgot to test for theme-supplied styling 😅

Anyway, this part might be harder to fix 😕 I think that the problem actually also goes back to WP_Theme_JSON (via WP_Theme_JSON_Resolver::get_theme_data). In WP_Theme_JSON's constructor, we set $this->theme_json after sanitizing the theme.json contents based on the list of known -- i.e. currently registered -- blocks: ​https://github.com/WordPress/wordpress-develop/blob/92a8bd811f228aa284ba2c88ff41b691aa7af1cd/src/wp-includes/class-wp-theme-json.php#L512-L515

$this->theme_json is then read frequently across that file; and it's never updated to take into account that the list of registered blocks might've changed.

Inside WP_Theme_JSON_Resolver::get_theme_data, we could probably do something around here: ​https://github.com/WordPress/wordpress-develop/blob/92a8bd811f228aa284ba2c88ff41b691aa7af1cd/src/wp-includes/class-wp-theme-json-resolver.php#L181

Akin to the changes I made to WP_Theme_JSON, we'd need to check if more blocks have been registered. Unfortunately, that's probably harder here, since AFAICS, we don't have access to the list of blocks that were registered at the time that the WP_Theme_JSON object was created (ironically, this is partly a consequence of the fix to WP_Theme_JSON::get_blocks_metadata, which would otherwise give us that 😅 ).

Yeah, so I haven't found a good way to smartly update the resolver's caching logic.

So I've tentatively pushed b746d24 which I believe fixes the issue -- by removing the caching altogether for the theme-supplied styling and settings 😬 I'm not sure how big the performance impact would be.

I'll be AFK on Friday and Monday. @andrewserong would you mind taking the lead on this one (so we'll have a fix ready for WP 6.1 Beta 3 on Tuesday)? I guess we basically need to decide between this solution and ​https://github.com/WordPress/wordpress-develop/pull/3352 (which would require some more action on the Gutenberg side, see ​https://github.com/WordPress/wordpress-develop/pull/3352#issuecomment-1261293667).

Maybe folks familiar with Global Styles can weigh in on both solutions (and this PR's performance impact). cc/ @oandregal @talldan 😊

Thanks for all the digging here, @ockham!

I'll be AFK on Friday and Monday. @andrewserong would you mind taking the lead on this one (so we'll have a fix ready for WP 6.1 Beta 3 on Tuesday)?

Yes, I'm happy to dig into this one today and on Monday and try to figure out what would be the best trade-off. It'd be great to get _a_ fix in for Beta 3, whichever direction we do go in. I'll give this PR a test and do some more exploration 👍

Ensure that block data does not become stale in the Theme JSON and Theme JSON Resolver classes if they are called during block registration. Specifically, attempt to resolve the issue described in: ​https://github.com/WordPress/gutenberg/issues/44434

This is an alternative to #3352 and #3359. The goal in this PR is to take a similar general approach to #3359, but focus on clearing the cached data when the number of registered blocks changes. This appears to have the same desired result of fresh data once all blocks have been registered, without removing the caching logic altogether.

Scope of changes:

• Add $theme_cache_key and get_new_theme_cache_key to WP_Theme_JSON_Resolver to allow cache busting when the list of registered blocks changes.
• Add $blocks_cache_key and get_new_blocks_cache_key to WP_Theme_JSON, also to allow cache busting when the list of registered blocks changes.

Trac ticket: https://core.trac.wordpress.org/ticket/56644

Testing this change:

1. Confirm that the issue as raised in ​https://github.com/WordPress/gutenberg/issues/44434 and https://core.trac.wordpress.org/ticket/56644 is resolved. You can do this by:
2. Activate theme TwentyTwentyTwo, and add a couple of buttons to a post. On the site frontend the buttons should use the theme's green colour, and have a border-radius of 0
3. In global styles, go to make changes at the block level to the Site Title block. You should be able to set background color, padding, etc.

This screenshot shows the Site Title block with custom background color and padding styling, and the Buttons block using the theme's styling:

Update: I've looked into this a bit further, and I think we might need to add in a variable to keep track of the number of blocks that have been registered to fix the resolver — or to put it differently, I think we'll achieve more reliable results for this particular caching if we generate a cache key and store it in a static variable.

I've opened up ​https://github.com/WordPress/wordpress-develop/pull/3373 as an alternative — basically, taking the same general idea as this PR, but attempting to largely preserve the caching if the number of blocks hasn't changed. This appears to be necessary because when testing this PR, it seems that by removing the resolver caching, read_json_file winds up being called 136 times on a single load of a test site running TwentyTwentyTwo theme.

Because we need to keep track of the number of blocks registered, I thought it might make for a good parallel between the two classes for them to share a similar approach to cache busting. It also gives us a function where we can keep the cache key generation logic explicit, to hopefully aid future debugging if need be. There's definitely more we could do to improve the caching behaviour, but so far #3373 seems to resolve the issue while largely preserving the caching as far as I can tell.

Happy for any feedback if folks have time to take a look at it!

This is working as advertised for me, thanks @andrewserong !

It seems like a good approach, assuming that the number of blocks alone is enough to determine the validity of any cached theme data.

I guess if it turns out that it's not down the road, get_new_theme_cache_key and others are generic enough to accommodate any changes to the internal cache key logic.

I'll leave to folks with more context to determine the robustness of the approach, but it LGTM!

assuming that the number of blocks alone is enough to determine the validity of any cached theme data.

I was wondering about the robustness of this too. This works fine in core without any plugins, but the way this would break is if someone unregisters a block and registers a new one after wp-template-part has been registered.

I don't expect that someone would unregister and re-register a block with the same name as often, so maybe something like this could work?

{{{php
hash( 'crc32', implode( array_keys( $blocks ) ) )
}}}

I don't expect that someone would unregister and re-register a block with the same name as often

It looks like we're already actually doing this in ​`register_legacy_post_comments_block` which is getting registered after wp-template-part with a priority of 21 vs 10 in core and 20 in gutenberg.

This is another alternative to #3352, #3359, and #3373.

This adjusts the priorities of block registration so that the core/template-part block is registered last. This means that all blocks (except core/template-part) will be registered before the metadata form get_blocks_metadata is called for the first time.

It currently has the problem that the block metadata for the core/template-part block itself won't be included, but that may be fixed with the combination of cache invalidation from #3373 if we can guarantee that register_block_core_template_part never unregisters any blocks.

I upped the WP_Block_Supports::init priority value to make room for blocks like ​`core/post-comments` to be registered after the rest of the core blocks, but before the core/template-part registration.

Trac ticket: https://core.trac.wordpress.org/ticket/56644

If we make sure that core/template-part (which only registers itself and does not unregister anything) is registered last, the count($blocks) heuristic will work. I opened #3384 to see what that would look like.

@andrewserong do you think this fix might solve ​https://github.com/WordPress/gutenberg/issues/44619? It seems related somehow to ​https://github.com/WordPress/gutenberg/issues/44434.

@andrewserong do you think this fix might solve ​https://github.com/WordPress/gutenberg/issues/44619? It seems related somehow to ​https://github.com/WordPress/gutenberg/issues/44434.

Thanks for the ping @ndiego, yes, it should fix that issue, too, as I believe it's the same underlying problem.

If we make sure that core/template-part (which only registers itself and does not unregister anything) is registered last, the count($blocks) heuristic will work. I opened ​https://github.com/WordPress/wordpress-develop/pull/3384 to see what that would look like.

Thanks for the feedback @ajlende! That's a good point about registration / re-registration of blocks. I think ideally we wouldn't need to depend on the Template Part block being registered in a particular order, but I'm also all in favour of pragmatic fixes to resolve the problem in the shorter-term 🙂

I'll have a play with either pulling in your changes to this PR, or updating the caching logic today and see what I can come up with!

This is (yet another) alternative to #3352, #3359, #3373 and #3384, as an attempt to resolve the issue described in: ​https://github.com/WordPress/gutenberg/issues/44434 — where block-level global styles are not output and cannot be saved in 6.1.

Changes proposed:

• Add a registered_block_type action that fires after a block is registered (added to both register_block_type and register_block_type_from_metadata functions.
• Register a callback against this action that then flushes the Theme JSON and Theme JSON Resolver caches.

This should fix the issue where the list of registered blocks becomes cached, preventing block-level global styles from working. The approach here ensures that any time a block is registered, the cache will be cleared after the block is registered, so the Theme JSON and Resolver caches should not contain any stale block data.

In #3359 and #3373 it was proposed to invalidate the cache based on the number of registered blocks. However, as @ajlende and @ramonjd flagged, using the number of blocks as the determining factor for the cache key may not be robust enough.

The idea in this PR is that if we add an action once a block is registered, then we don't need to perform a look-up in the Theme JSON and Resolver classes, as a side effect of registering a block will be to clear out the existing cache.

Trac ticket: https://core.trac.wordpress.org/ticket/56644

Great points about the issues of robustness @ajlende and @ramonjd! I've had a go at opening up (yet another) alternative PR in #3392 that proposes to add a registered_block_type action and use this as the mechanism to flush the cache instead of adding an extra cache key as in this PR. I _think_ something like that might be preferable so that we don't have to fuss about trying to determine whether or not the list of registered blocks has changed.

One concern I have with the idea in #3384 is that altering the order in which blocks are registered also seems to be a bit brittle, in that we then need to be careful about future changes to the block registration order, and it's still possible for a plugin to accidentally break things if something similar to what the Post Template block does, happens in a 3rd party block.

Happy to keep exploring options tomorrow, if anyone has any extra feedback. Thanks so much for digging in, too, @ajlende! 🙇

I think we could move the hook into WP_Block_Type_Registry's register and unregister rather than in blocks.php, but otherwise this seems to me like a good fix to me to mitigate the problem for 6.1. 👍

I think we could move the hook into WP_Block_Type_Registry's register and unregister rather than in blocks.php

Good call — that ensures that we don't miss any registrations that happen using the class directly, and means we can remove the duplication in this PR 👍. I'll update!

I've updated this PR to move the actions to the class, and that seems to fix up the issue of making sure that we capture all registration events. I've left the callback targeting _just_ the registered event, though — since the action occurs after registration completes, if a plugin unregisters blocks for any reason, the cache will still be clear. I think it's probably pretty unlikely that a plugin would call the Theme JSON classes doing an _unregister_ task, so possibly not worth the extra processing to target unregister, too. I've included the action, though, for consistency and so that it's easy to add in if we need to in the future.

Thanks for reviewing @talldan! I've made a few small updates to the comments 👍

Based on ​https://github.com/WordPress/gutenberg/issues/44434#issuecomment-1266668769 (which is in turn based on the concerns expressed in ​https://github.com/WordPress/wordpress-develop/pull/3359#issuecomment-1263105858), I'll revert the commit that disables caching for WP_Theme_JSON_Resolver::get_theme_data.

Thank you @hellofromtonya! I've applied your suggestions 😊

[davidbaumwald]

In 54385:

Editor: Invalidate blocks metadata cache when needed in WP_Theme_JSON::get_blocks_metadata().

This change ensures that user-supplied global styles settings for blocks aren't lost due to sanitization. This could previously occur due to outdated blocks metadata that did not include all registered blocks.

Props jorgefilipecosta, andrewserong, oandregal, talldanwp, cbravobernal, bernhard-reiter, hellofromTonya.
Fixes #56644.

Merged into core in https://core.trac.wordpress.org/changeset/54385.

Thanks folks! Now that #3359 has landed I'll close this out.

Closing as other solutions are now in the process of being merged / and or validated. Thanks for all the feedback and collaboration!

I'll close out this PR now as folks decided to go in a different direction to avoid adding new actions, as flagged in ​https://github.com/WordPress/gutenberg/issues/44434#issuecomment-1266637142.

Rebased.

I'll change the Trac ticket linked in the PR desc to https://core.trac.wordpress.org/ticket/56736, since this PR would solve that, and the previously linked ticket (https://core.trac.wordpress.org/ticket/56644) has now been fixed.

Closing this out. Both related trac tickets have been solved.