search

WordPress / gutenberg

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

[WP 6.1] Block-specific global styles don't seem to save #44434

Closed RahiDroid closed 2 years ago

RahiDroid commented 2 years ago

Description

Step-by-step reproduction instructions

Copy pasting from the trac ticket,

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

Screenshots, screen recording, code snippet

https://user-images.githubusercontent.com/59014930/192104832-a8b0f526-0baf-41b9-8c7a-f1e57099fa58.mov

Environment info

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

ndiego commented 2 years ago

I can confirm this issue. It's a pretty major one 😬 global-style-no-saving

glendaviesnz commented 2 years ago

Looks like the values are being saved to the styles custom post type, but are not being retrieved for some reason, will see if I can work out why that is.

glendaviesnz commented 2 years ago

It looks like in wordpress-develop this call is not returning the custom global styles data. The correct data is there in the preceding config var, ran out of time to try and work out why it is working in GB trunk and not 6.1 beta.

@oandregal may have some thoughts - this PR touched on some of these areas, but it all seems to have been ported to 6.1

andrewserong commented 2 years ago

Interestingly, in wordpress-develop I can save styles for server rendered blocks in global styles (e.g. site title), but not static blocks (like Group). Just in case that points in any particular direction for this one or the linked trac issue.

andrewserong commented 2 years ago

I think this one will likely need to be fixed from the core side, but I noticed (and commented here on the trac issue) that if we update the priority of the register_core_block_types_from_metadata callback on the init action to have a priority of 1, that appears to resolve the issue (this line). So it sounds like it's likely related to a caching issue / order of calls with get_blocks_metadata being called too early, like @jorgefilipecosta described? (While updating the priority worked in my local environment, I imagine that won't work as a proper fix, as it possibly obfuscates the real bug 🤔)

ndiego commented 2 years ago

I can confirm that this issue does not occur with Gutenberg active. Only in 6.1 Beta. So it seemly like something might not have been ported over properly? 🤔

oandregal commented 2 years ago

I've done some testing: go to the global styles sidebar and set the global background color and the background color of the heading block. This is how I'm tracing this data usage:

{
  "styles": {
    "color": { "background": "var:preset|color|vivid-red" },
    "blocks": {
      "core/heading": {
        "color": { "background": "var:preset|color|luminous-vivid-amber" }
      }
    }
  },
  "isGlobalStylesUserThemeJSON": true,
  "version": 2
}
(
    [theme_json:protected] => Array
        (
            [styles] => Array
                (
                    [color] => Array
                        (
                            [background] => var:preset|color|vivid-red
                        )

                )

            [version] => 2
        )

)

So the data is properly saved but is discarded when rendering.

oandregal commented 2 years ago

What happens is that, at some point, the theme.json class receives the data saved in the database. It wants to sanitize it. One of the things it does is removing data for blocks that are not registered. And, apparently, the core/heading is not, hence its data is removed and the styles are generated without it. These are the "valid blocks" at the point the user data is first calculated:

(
    [0] => core/legacy-widget
    [1] => core/widget-group
    [2] => core/archives
    [3] => core/avatar
    [4] => core/block
    [5] => core/calendar
    [6] => core/categories
    [7] => core/comment-author-name
    [8] => core/comment-content
    [9] => core/comment-date
    [10] => core/comment-edit-link
    [11] => core/comment-reply-link
    [12] => core/comment-template
    [13] => core/comments
    [14] => core/comments-pagination
    [15] => core/comments-pagination-next
    [16] => core/comments-pagination-numbers
    [17] => core/comments-pagination-previous
    [18] => core/comments-title
    [19] => core/cover
    [20] => core/file
    [21] => core/gallery
    [22] => core/home-link
    [23] => core/image
    [24] => core/latest-comments
    [25] => core/latest-posts
    [26] => core/loginout
    [27] => core/navigation
    [28] => core/navigation-link
    [29] => core/navigation-submenu
    [30] => core/page-list
    [31] => core/pattern
    [32] => core/post-author
    [33] => core/post-author-biography
    [34] => core/post-comments-form
    [35] => core/post-content
    [36] => core/post-date
    [37] => core/post-excerpt
    [38] => core/post-featured-image
    [39] => core/post-navigation-link
    [40] => core/post-template
    [41] => core/post-terms
    [42] => core/post-title
    [43] => core/query
    [44] => core/query-no-results
    [45] => core/query-pagination
    [46] => core/query-pagination-next
    [47] => core/query-pagination-numbers
    [48] => core/query-pagination-previous
    [49] => core/query-title
    [50] => core/read-more
    [51] => core/rss
    [52] => core/search
    [53] => core/shortcode
    [54] => core/site-logo
    [55] => core/site-tagline
    [56] => core/site-title
    [57] => core/social-link
    [58] => core/tag-cloud
)
oandregal commented 2 years ago

I've quickly looked at the places where we require global styles data. Inspected the usage of the wp_get_global_stylesheet (and similar functions) as well as the classes (WP_Theme_JSON and WP_Theme_JSON_Resolver). I wanted to learn if we had changed the places where we use this code (script-loader.php, etc.) and made it so it runs earlier than block registration. I haven't found anything meaningful.

The other reason some blocks may not be registered at that point is that block registration had changed, as Jorge suggested in the trac ticket. A bit out of my expertise at the moment, and given some time-constraints I have this week I'm not sure if I'm going to be able to help investigate much further this path.

andrewserong commented 2 years ago

Thanks for writing all that up @oandregal — I think I've found where the caching issue is occuring:

After a git-bisect on wordpress-develop that pointed to this backport commit, I tried looking through the backported server-rendered blocks, and I think I've found the area that's causing the issue in the Template Part block's build_template_part_block_instance_variations function. This function is called when the block is registered, rather than when it's rendered, and uses the get_block_templates function on this line, which appears to eventually further down the line call get_theme_data here, causing the list of registered blocks to be cached.

If I update the following line in register_block_core_template_part to set variations to null, then global styles appears to be working correctly again: https://github.com/WordPress/wordpress-develop/blob/3b63a75108b7f6d5e9112c556285ab1bc80ddbcf/src/wp-includes/blocks/template-part.php#L248

So, the problem appears to be calling a getter function like this at registration time prior to all blocks being registered.

For a bit more background, this caused a few issues in the Gutenberg plugin earlier on. The solution at the time was to adjust the caching in Gutenberg, but it looks like this didn't get to the root of the problem for when the block was backported. Here are some earlier links:

The question now, then, is what's the right fix for this problem? @ramonjd mentioned the idea of potentially calling or adding an action to call clean_cached_data again, similar to the switch theme action: add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver', 'clean_cached_data' ) );.

I'm wondering if we should either a) avoid calling a getter function for theme data at registration time in the Template Part block, or b) work out a way to reliably clean the cached data at the right time once all blocks are registered. CC: @talldan in case this sparks any ideas, too.

Does anyone have any strong opinions on what the correct caching behaviour should be here?

talldan commented 2 years ago

I'm wondering if we should either a) avoid calling a getter function for theme data at registration time in the Template Part block, or b) work out a way to reliably clean the cached data at the right time once all blocks are registered. CC: @talldan in case this sparks any ideas, too.

A short term fix isn't a bad idea for 6.1. A longer term fix should also be investigated as this code is too fragile. get_block_templates is a public function, and it's one that plugins could very well be interested in calling, so while there's some ability to fix this internally, the deeper problem is exposing brittle APIs for public consumption.

Possibly a quick fix would be to add a way to get templates without filling the theme json cache.

Looking at the deeper problem, to me it seems like there should be some cache invalidation at certain points.

There are also dependency issues. Getting template parts shouldn't require compiling and caching the entire theme json. A lot of the settings in theme json are completely unrelated, so it might be worth breaking the cache into smaller sub-caches. The filling and invalidation of those caches can then be optimized in a way that wouldn't have been possible before.

andrewserong commented 2 years ago

Thanks @talldan, you raise some great points! Ideally get_block_templates wouldn't have such potentially risky side-effects 👍

In order to try to find a pragmatic short-term fix, I've opened up a core PR in https://github.com/WordPress/wordpress-develop/pull/3352 to try adding a comparable clean_cached_data static method to the WP_Theme_JSON class and call both it and WP_Theme_JSON_Resolver::clean_cached_data() when the Template Part block has finished gathering its variations. I'm wrapping up for the day now, but happy to try to look into adding tests for it tomorrow if it looks viable (I'm not 100% sure how to reliably test for it, so also very happy for any ideas on that PR if anyone gets the chance 🙂)

I figure a core PR is a better way to test for a fix than doing it in the GB repo. Whichever fix we come up with, we can then port it back over to GB once it's landed.

ockham commented 2 years ago

Thanks @oandregal @andrewserong @talldan for helping track this down!

Upon reading @andrewserong's comment, especially this part:

[...] I think I've found the area that's causing the issue in the Template Part block's build_template_part_block_instance_variations function. This function is called when the block is registered, rather than when it's rendered, and uses the get_block_templates function on this line, which appears to eventually further down the line call get_theme_data here, causing the list of registered blocks to be cached.

...I had one idea off the top of my head: It seems like there's actually a few unneeded function calls, mostly due to the fact that templates and template parts share a lot of logic. Specifically, we're obviously only interested in template parts here; still we end up calling WP_Theme_JSON_Resolver::get_theme_data()->get_custom_templates(). But that's the list of custom page templates that are included in the theme -- which shouldn't have any bearing on template parts.

So maybe we can leverage the fact that we already know (and specify) that we're only interested in template parts upon invoking get_block_templates in order to avoid calling functions that are only relevant to templates (but not parts) 🤔

ockham commented 2 years ago

Uh, nevermind. I think the line we're calling is this (rather than this), so it goes through _add_block_template_part_area_info rather than _add_block_template_info. That's the correct behavior; _add_block_template_part_area_info just also happens to call WP_Theme_JSON_Resolver::get_theme_data() -- albeit to chain it to ->get_template_parts() rather than templates.

So no potential to fix this here 😬

ockham commented 2 years ago

I've filed an alternative fix here, based on y'all's findings: https://github.com/WordPress/wordpress-develop/pull/3359 😊

andrewserong commented 2 years ago

Great work @ockham, that feels much closer to an ideal fix. Just left a comment over on that PR — I think there's still a bit more fussing we need to do with the clearing the resolver cache to get the Button styling in TwentyTwentyTwo working again, but I think we're heading in the right direction!

andrewserong commented 2 years ago

Building on the idea @ockham is exploring, I've opened up another alternative PR in https://github.com/WordPress/wordpress-develop/pull/3373 which tries to be a bit more explicit about when we cache bust in the Theme JSON and Resolver classes — basically, trying to make sure that a change in the number of registered blocks results in the cache being regenerated for just what we need. I think this should fix the issue while still preserving the performance benefits of the current caching.

If anyone gets a chance to take a look, I can work through any feedback on Monday. CC: @talldan @jorgefilipecosta @oandregal

andrewserong commented 2 years ago

I've opened (yet another) PR in https://github.com/WordPress/wordpress-develop/pull/3392 to explore a potential solution. @ramonjd and @ajlende rightly pointed out that checking for the number of registered blocks can result in edge cases where the caching isn't busted when we expect it to be. In this alternative PR, I'm proposing adding a registered_block_type action, and to use this to flush the Theme JSON and Resolver caches — that way it's ensured the caches are cleared whenever a block is registered, rather than attempting to infer the current state within the Theme JSON/Resolver classes.

Happy for any feedback / ideas if folks think a different approach would be better!

oandregal commented 2 years ago

I've looked at alternatives and https://github.com/WordPress/wordpress-develop/pull/3392 (cleaning data when a new block is registered) sounds theoretically the best approach, though 1) we also need to consider that blocks can be unregistered as well, and 2) there are a few paths 3rd parties can use to register a block AFAIK.

It sounds like https://github.com/WordPress/wordpress-develop/pull/3359 may be a more robust fix for 6.1. We can look at this more holistically after 6.1.

andrewserong commented 2 years ago

Thanks for taking a look @oandregal! I think @ajlende's feedback about moving the "registered" hook to the WP_Block_Type_Registry class would ensure we catch all block registrations (including 3rd party ones), and remove a little duplication. I'll have a play at updating https://github.com/WordPress/wordpress-develop/pull/3392 today.

andrewserong commented 2 years ago

I've updated https://github.com/WordPress/wordpress-develop/pull/3392 to move the hook to the registry class, which now seems to capture all cases where a block might be registered.

ockham commented 2 years ago

Update: We now have a few candidates to fix this issue:

Finally, while not a fix to this issue, there's https://github.com/WordPress/gutenberg/pull/44658, which is a related minor performance and code quality improvement.

I'll review these alternatives in a follow-up comment and will try to draw up a strategy that we can take from here.

ockham commented 2 years ago

We have two partial fixes/improvements that I consider very low-risk:

I would then like to land those two.

This leaves us with the caching issue in WP_Theme_JSON_Resolver. I would like to explore this suggestion by @oandregal for a bit, since it seems like it would tackle the caching issue at its core.

If we cannot get that ready in time, we might instead merge a fallback. I'm currently leaning towards https://github.com/WordPress/wordpress-develop/pull/3384, since it doesn't introduce any new APIs.

ockham commented 2 years ago

I think one important thing is to treat the issues separately:

  1. The WP_Theme_JSON PR fixes the manual Global Styles-styling of buttons by the user (this issue).
  2. The theme.json stuff is broken because of WP_Theme_JSON_Resolver caching (#44619).

@c4rl0sbr4v0 is currently looking into @oandregal's suggestion.

ockham commented 2 years ago

Update:

  1. We've now landed the WP_Theme_JSON caching fix; it's part of WP 6.1 Beta 3.
  2. Carlos has filed https://github.com/WordPress/wordpress-develop/pull/3401 as a tentative fix for https://github.com/WordPress/gutenberg/issues/44619.

I'm considering closing this issue (since it's arguably fixed), and moving discussion over to https://github.com/WordPress/gutenberg/issues/44619 (which AFAICS covers the remaining issue with the theme resolver). Thoughts? 👍 / 👎

ndiego commented 2 years ago

I'm considering closing this issue (since it's arguably fixed), and moving discussion over to https://github.com/WordPress/gutenberg/issues/44619 (which AFAICS covers the remaining issue with the theme resolver). Thoughts?

Yeah, in my testing, this issue appears to be fixed in Beta 3

andrewserong commented 2 years ago

Thanks for weighing up all the options @ockham, and landing the incremental fixes! 🙇

I'm considering closing this issue (since it's arguably fixed), and moving discussion over to https://github.com/WordPress/gutenberg/issues/44619 (which AFAICS covers the remaining issue with the theme resolver). Thoughts? 👍 / 👎

That sounds good to me. Should we keep a trac issue open for #44619, though, as it's still a blocker for 6.1?

@c4rl0sbr4v0 is currently looking into @oandregal's https://github.com/WordPress/wordpress-develop/pull/3359#discussion_r985689298.

To keep efforts streamlined, I've closed out my alternate PRs now, but do ping me if you need a review on anything. Thanks for digging in to find the right solution!

ockham commented 2 years ago

That sounds good to me. Should we keep a trac issue open for https://github.com/WordPress/gutenberg/issues/44619, though, as it's still a blocker for 6.1?

Yeah, good point. I'll file one 👍

To keep efforts streamlined, I've closed out my alternate PRs now, but do ping me if you need a review on anything. Thanks for digging in to find the right solution!

Thank you for all your work and research! ❤️

ockham commented 2 years ago

That sounds good to me. Should we keep a trac issue open for #44619, though, as it's still a blocker for 6.1?

Yeah, good point. I'll file one 👍

https://core.trac.wordpress.org/ticket/56736