Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Google Fonts module & standalone Gutenberg plugin conflicting with theme block styles #26224

Closed
JoshuaGoode opened this issue Sep 14, 2022 · 8 comments
Labels
[Feature] Google Fonts [Platform] Atomic [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] High Triaged [Type] Bug When a feature is broken and / or not performing as intended

Comments

@JoshuaGoode
Copy link
Contributor

JoshuaGoode commented Sep 14, 2022

Impacted plugin

Jetpack, Google Fonts Module

p9F6qB-afB-p2

This is an updated version of Automattic/wp-calypso#67739 -- I'd normally move the issue over but the old issue isn't as targeted since we have new findings. We might close the old one to reduce confusion.

Steps to Reproduce

  1. Create site (anywhere, any infrastructure)
  2. Install and activate Jetpack and standalone Gutenberg plugins
  3. Install and activate an FSE theme like Twentytwentytwo
  4. Connect Jetpack, and activate the Google Fonts Jetpack module.
  5. View front end of site with Google Fonts module activate and inactivate

Observe block styles, such as the site title/header are missing while the module is active. E.g. wp-block-site-title

Site title will appear quite large.

Screen Shot on 2022-09-14 at 14:06:45

A clear and concise description of what you expected to happen.

For block styles to load properly.

Screen Shot on 2022-09-14 at 14:07:56

What actually happened

Theme's block styles are missing, large site title for example.

Browser

No response

Other information

Related to p9F6qB-afB-p2

It's also Automattic/wp-calypso#67739 but creating this new specific issue in Jetpack as we now know more about the conflict.

Platform (Simple, Atomic, or both?)

Atomic, Self-hosted

Reproducibility

Consistent

Severity

Some (< 50%)

Available workarounds?

No response

Workaround details

On WPCOM Atomic, we force activate the Google Fonts module within wpcomsh/feature-plugins/google-fonts.php -- so there isn't a direct workaround.

@JoshuaGoode JoshuaGoode added [Type] Bug When a feature is broken and / or not performing as intended [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Feature] Google Fonts Needs triage Ticket needs to be triaged labels Sep 14, 2022
@anomiex
Copy link
Contributor

anomiex commented Sep 14, 2022

I don't think this is really being caused by anything in the Google Fonts module. I can reproduce it on a site without Jetpack installed at all by adding this as a mu-plugin:

<?php

function test_26224() {
        gutenberg_get_global_styles();
}
add_action( 'init', 'test_26224', 11 );

Note the default priority of 10 doesn't work here, it has to be 11 (or, presumably, larger), even though Jetpack's Google Fonts module does it at priority 10, so it seems ordering of the init hooks is important.

@JoshuaGoode
Copy link
Contributor Author

Ah, thank you @anomiex

@pbking mentioned something similar.

@jeyip
Copy link
Contributor

jeyip commented Sep 14, 2022

Potential patch opened by @fullofcaffeine #26193

@anomiex
Copy link
Contributor

anomiex commented Sep 15, 2022

I dug further into the problem. Copying my comment from p9F6qB-afB-p2#comment-49394 here for the record

Dug into it a bit deeper for you all.

What’s happening is that the static method WP_Theme_JSON_6_1::get_blocks_metadata() gets the metadata on its first call and then caches it, so it doesn’t catch anything registered after whenever the first call was. The same issue seems to exist in WP_Theme_JSON_5_9::get_blocks_metadata() and in WP_Theme_JSON::get_blocks_metadata() in core.

Then we have gutenberg_reregister_core_block_types de-registering the blocks on init priority 10, but the re-registration doesn’t happen until init priority 20. If the first call happens in between those two operations, the cache will be missing all those de-registered blocks.

In Gutenberg 13.4.0 that first call was happening before the de-registration, due to gutenberg_register_webfonts_from_theme_json being at priority 10 and happening to run before the de-registration function, so it cached the data from the core versions of the blocks. In wordpress/gutenberg#41569 that was moved to priority 21 because it wanted the data from the Gutenberg version of the blocks instead.

That works when Gutenberg is the only thing doing things, but when Jetpack’s Google Fonts module did its call (which happened to be after the de-registration) then get_blocks_metadata() wound up caching the state where the blocks had been de-registered and not re-registered yet.

Perhaps it’s valid to document the ordering here, that anything that winds up calling get_blocks_metadata() (which someone would have to track back to the relevant public methods to do that documenting) needs to run after init priority 20. Possibly also that block registration should happen at or before init priority 20. And/or perhaps get_blocks_metadata() needs some cache invalidation, or at least a doing-it-wrong if called too early.

The implication of that is that perhaps our Google Fonts module is in the wrong after all for calling the function too early.

@cuemarie
Copy link

📌 FINDINGS/SCREENSHOTS/VIDEO

  • Skipping scrubbing steps as this is already being reviewed!

📌 ACTIONS

  • Marked as Triaged for Quality Squad review

@cuemarie cuemarie added Triaged [Pri] High and removed Needs triage Ticket needs to be triaged labels Sep 21, 2022
@iamtakashi
Copy link
Contributor

Hi, I've seen inconsistency in post titles between Simple and Atomic like this.

Will that be fixed too with this like site title?

@anomiex
Copy link
Contributor

anomiex commented Sep 21, 2022

The issue you linked should be fixed on Atomic once the weekly release goes out to them, which will probably happen later today. Same should apply to Simple. I don't know whether the specific comment you linked was related to that issue or not.

@anomiex
Copy link
Contributor

anomiex commented Sep 21, 2022

Closing as this should be fixed now by #26193.

@anomiex anomiex closed this as completed Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Google Fonts [Platform] Atomic [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] High Triaged [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

No branches or pull requests

5 participants