#56644 closed defect (bug) (fixed)
Global styles don't work for most blocks (paragraphs, headings, buttons etc).
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.1 | Priority: | high |
Severity: | major | Version: | 6.1 |
Component: | Editor | Keywords: | has-patch |
Focuses: | Cc: |
Description
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.
Change History (39)
#2
@
2 years ago
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 ticket was mentioned in PR #3352 on WordPress/wordpress-develop by andrewserong.
2 years ago
#3
- Keywords has-patch added; needs-patch removed
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.
andrewserong commented on PR #3352:
2 years ago
#4
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?
c4rl0sbr4v0 commented on PR #3352:
2 years ago
#5
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
I will take a look at the E2E test in the Gutenberg repo.
2 years ago
#6
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 ofWP_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.
This ticket was mentioned in PR #3359 on WordPress/wordpress-develop by ockham.
2 years ago
#7
Tentative fix for 56644. Alternative to https://github.com/WordPress/wordpress-develop/pull/3352.
Trac ticket: https://core.trac.wordpress.org/ticket/56644
2 years ago
#8
I've filed an alternative fix here: https://github.com/WordPress/wordpress-develop/pull/3359 😊
2 years ago
#9
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.
2 years ago
#10
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 😅 ).
2 years ago
#11
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 😊
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
andrewserong commented on PR #3359:
2 years ago
#13
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 👍
This ticket was mentioned in PR #3373 on WordPress/wordpress-develop by andrewserong.
2 years ago
#14
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
andget_new_theme_cache_key
toWP_Theme_JSON_Resolver
to allow cache busting when the list of registered blocks changes. - Add
$blocks_cache_key
andget_new_blocks_cache_key
toWP_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:
- 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:
- 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
- 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:
andrewserong commented on PR #3359:
2 years ago
#15
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!
2 years ago
#16
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!
2 years ago
#17
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 ) ) )
}}}
2 years ago
#18
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 ticket was mentioned in PR #3384 on WordPress/wordpress-develop by ajlende.
2 years ago
#19
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
2 years ago
#20
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.
2 years ago
#21
@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 commented on PR #3373:
2 years ago
#22
@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.
andrewserong commented on PR #3373:
2 years ago
#23
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 ticket was mentioned in PR #3392 on WordPress/wordpress-develop by andrewserong.
2 years ago
#24
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 bothregister_block_type
andregister_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
andrewserong commented on PR #3373:
2 years ago
#25
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! 🙇
2 years ago
#26
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. 👍
andrewserong commented on PR #3392:
2 years ago
#27
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!
andrewserong commented on PR #3392:
2 years ago
#28
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.
andrewserong commented on PR #3392:
2 years ago
#29
Thanks for reviewing @talldan! I've made a few small updates to the comments 👍
2 years ago
#30
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
.
#32
@
2 years ago
- Owner set to davidbaumwald
- Resolution set to fixed
- Status changed from new to closed
In 54385:
dream-encode commented on PR #3359:
2 years ago
#33
Merged into core in https://core.trac.wordpress.org/changeset/54385.
andrewserong commented on PR #3373:
2 years ago
#34
Thanks folks! Now that #3359 has landed I'll close this out.
andrewserong commented on PR #3352:
2 years ago
#35
Closing as other solutions are now in the process of being merged / and or validated. Thanks for all the feedback and collaboration!
andrewserong commented on PR #3392:
2 years ago
#36
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.
2 years ago
#38
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.
I could reproduce the issue and have opened an issue on the Gutenberg repo here: https://github.com/WordPress/gutenberg/issues/44434 as suggested.