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.

Sign up for GitHub

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

[WIP] Limit Global Styles: Invalidate cache after change / during preview #69703

Closed
wants to merge 5 commits into from
Closed

[WIP] Limit Global Styles: Invalidate cache after change / during preview #69703

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4bb2baa
Limit Global Styles: Invalidate cache after change / during preview
mmtr Nov 2, 2022
14b6266
Update apps/editing-toolkit/editing-toolkit-plugin/wpcom-global-style…
mmtr Nov 2, 2022
2b0bd77
Add comment explaining why we give priority to Gutenberg classes
mmtr Nov 2, 2022
273aeb5
Rename variable
mmtr Nov 2, 2022
aa8cb27
Also invalidate wp_global_styles_%s cache
mmtr Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 31 additions & 5 deletions apps/editing-toolkit/editing-toolkit-plugin/wpcom-global-styles/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,10 @@ function wpcom_block_global_styles_frontend( $theme_json ) {
return $theme_json;
}

if ( class_exists( 'WP_Theme_JSON_Data' ) ) {
return new WP_Theme_JSON_Data( array(), 'custom' );
} elseif ( class_exists( 'WP_Theme_JSON_Data_Gutenberg' ) ) {
if ( class_exists( 'WP_Theme_JSON_Data_Gutenberg' ) ) {
return new WP_Theme_JSON_Data_Gutenberg( array(), 'custom' );
} elseif ( class_exists( 'WP_Theme_JSON_Data' ) ) {
return new WP_Theme_JSON_Data( array(), 'custom' );
mmtr marked this conversation as resolved.
Show resolved Hide resolved
}

/*
Expand All @@ -130,7 +130,6 @@ function wpcom_block_global_styles_frontend( $theme_json ) {
*/
return $theme_json;
}
add_filter( 'theme_json_user', 'wpcom_block_global_styles_frontend' );
add_filter( 'wp_theme_json_data_user', 'wpcom_block_global_styles_frontend' );

/**
Expand Down Expand Up @@ -184,7 +183,14 @@ function wpcom_track_global_styles( $blog_id, $post, $updated ) {
* @return bool Returns true if custom styles are in use.
*/
function wpcom_global_styles_in_use() {
$user_cpt = WP_Theme_JSON_Resolver_Gutenberg::get_user_data_from_wp_global_styles( wp_get_theme() );
$current_theme = wp_get_theme();
if ( class_exists( 'WP_Theme_JSON_Resolver_Gutenberg' ) ) {
$user_cpt = WP_Theme_JSON_Resolver_Gutenberg::get_user_data_from_wp_global_styles( $current_theme );
} elseif ( class_exists( 'WP_Theme_JSON_Resolver' ) ) {
$user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $current_theme );
} else {
return false;
}

if ( ! isset( $user_cpt['post_content'] ) ) {
do_action( 'global_styles_log', 'global_styles_not_in_use' );
Expand Down Expand Up @@ -286,3 +292,23 @@ function wpcom_display_global_styles_banner_extra_tooltip() {
<?php
}
add_action( 'launch_bar_extra_tooltip_toggle_global_styles', 'wpcom_display_global_styles_banner_extra_tooltip' );

/**
* Invalidates the cached Global Styles after changing them or when previewing them.
*/
function wpcom_invalidate_global_styles_cache() {
$transient_name = 'gutenberg_global_styles_' . get_stylesheet();
mmtr marked this conversation as resolved.
Show resolved Hide resolved

if ( wpcom_should_limit_global_styles() && wpcom_is_previewing_global_styles() ) {
add_filter( 'pre_transient_' . $transient_name, '__return_null' );
add_filter( 'pre_set_transient_' . $transient_name, '__return_false' );
Copy link
Contributor

@rcrdortiz rcrdortiz Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too keen on this approach. I really think it would be better to leverage the $can_use_cached condition in https://github.com/WordPress/gutenberg/blob/trunk/lib/compat/wordpress-6.1/get-global-styles-and-settings.php#L71. On that file, when $can_use_cache is false, the current request will not use the cached results and will not cache the generated results.

Maybe we can request a filter called gutenberg_global_styles_can_use_cached that would serve as a discriminator for when we want to programmatically turn off the cache.

Hooking into the transient to achieve the same result assumes that the reader of this code understands how Gutenberg is handling the cache and returning null and false without explanation has a bit of the effect of a magic number, at least for me (I had to check elsewhere why these specific values were needed instead of any other value). I do understand that the changes you are proposing do not require a Gutenberg release instead of needing to wait for one, but I think that having a cache disabling filter will add value beyond Gating Global Styles.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can request a filter called gutenberg_global_styles_can_use_cached that would serve as a discriminator for when we want to programmatically turn off the cache.

I totally agree. In fact, there is already a PR implementing this (WordPress/wordpress-develop#2732, WordPress/gutenberg#41201) but it looks like it didn't get too much attention.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can try to give that PR a second chance? We can always come back and merge this PR if necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. But even if those PRs go forward, we'll still need to fix the cache problems in the meantime since it can take weeks until the changesets are available in WP.com (which is why we temporary disabled the default cache in Simple sites a few months ago: D81197-code).

@tyxla: do we have any way to expedite your PRs above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, really. They have been lingering there for a while now. I wonder if we can get help from someone from .org. @annezazu do you have any idea who we can ask for help with reviews and shipping?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On it :) I'll tag some folks in that PR to see if we can get things moving, considering 6.1 is now out the door.

}

add_action(
'save_post_wp_global_styles',
function () use ( $transient_name ) {
delete_transient( $transient_name );
}
);
mmtr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really love if we could actually fix the issue in Gutenberg instead of circumventing it. With this code, we aren't preventing the issue from happening to other components/systems that use global styles. Gutenberg should be purging the cache whenever the Global Styles CPT gets updated.

Copy link
Member Author

@mmtr mmtr Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll create an equivalent PR for both Core and Gutenberg tomorrow that will fix https://core.trac.wordpress.org/ticket/56910 and WordPress/gutenberg#43914. But as noted above, we would still need this PR since otherwise we would have the cache problems for weeks.

Copy link
Contributor

@rcrdortiz rcrdortiz Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But as noted above, we would still need this PR since otherwise we would have the cache problems for weeks.

I think that we can wait, Gating Global Styles isn't live so technically the cache problem doesn't exist yet. There still needs to be some conversations before we can actually resume working on Gating Global Styles. We won't be launching the feature any time soon, but I'm ok with merging this as long as we add a follow-up task to remove the code once the issue is fixed in Core/Gutenberg.

I really believe that we aren't in a hurry with this and should prioritize fixing the underlying problem rather than making it work for our specific case and then cleaning up the tech debt, but as I said above I don't mind releasing this as long as we take ownership and clean up after.

Copy link
Member Author

@mmtr mmtr Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gating Global Styles isn't live so technically the cache problem doesn't exist yet

It does exist, since the cache currently prevents all WP.com users from immediately see style changes done in the site editor.

Said that, the preferred approach by Core seems to cache the styles forever (instead of a minute) and correctly invalidate them. This makes the invalidation harder, since we need to account for many more scenarios like WordPress updates, theme updates, etc. However, this is currently investigated in this PR that seeks to cache the gutenberg_get_global_settings method, so I think we should wait for that one first and replicate the same strategy afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gutenberg should be purging the cache whenever the Global Styles CPT gets updated.

This is the key issue we need to address, in my view. Marin and I talked about WordPress/gutenberg#41201 (comment), and I'd love this is addressed in addition to migrate away from transients to object cache.

Copy link
Contributor

@oandregal oandregal Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also want to share WordPress/gutenberg#45171 for you to see. I've seen many issues with caches, so I created that issue to seek some clarity as to what needs fixing.

}
add_action( 'init', 'wpcom_invalidate_global_styles_cache' );