-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[WIP] Limit Global Styles: Invalidate cache after change / during preview #69703
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
apps/editing-toolkit/editing-toolkit-plugin/wpcom-global-styles/index.php
Outdated
Show resolved
Hide resolved
apps/editing-toolkit/editing-toolkit-plugin/wpcom-global-styles/index.php
Show resolved
Hide resolved
add_filter( 'pre_transient_' . $transient_name, '__return_null' ); | ||
add_filter( 'pre_set_transient_' . $transient_name, '__return_false' ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
apps/editing-toolkit/editing-toolkit-plugin/wpcom-global-styles/index.php
Outdated
Show resolved
Hide resolved
add_action( | ||
'save_post_wp_global_styles', | ||
function () use ( $transient_name ) { | ||
delete_transient( $transient_name ); | ||
} | ||
); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I prefer not to be testing this PR since for some reason I'm just unable to reproduce the original issue on my env. Could someone else give a test? |
…s/index.php Co-authored-by: Richard Ortiz <rcrd.ortiz@gmail.com>
This PR modifies the release build for editing-toolkit To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-mMA-p2 |
Following the test plan worked for me, and trying resetting stuff at various points also worked as I'd expect. However it also worked without these changes :/ have you got a step-by-step repro case @mmtr? |
Yes, I can easily reproduce the issue when following these steps:
Screen.Recording.2022-11-04.at.10.44.48.mov |
hmm, the theme I used didn't have variations, so I just made other gs changes. I'll try again, but I'd be surprised if that was related. |
Closing this in favor of WordPress/gutenberg#45679. Once that lands, we won't need to invalidate the cache after changing the styles, and we'll be able to use a simple filter to prevent the cached styles from being loading when previewing the hidden styles.
I moved this to a separate PR: #70121 |
Fixes https://github.com/Automattic/dotcom-forge/issues/1151
Proposed Changes
This PR invalidates the cached Global Styles after they are changed or when previewing them, so we always show the styles expected by the user.
I also took the opportunity to remove the
theme_json_user
filter (since WP.com no longer loads the Gutenberg version that triggered it) and modify thewpcom_global_styles_in_use
function to account for cases where Gutenberg does not provide a compatWP_Theme_JSON_Resolver_Gutenberg
class. They are unrelated to cache issue, so I'm happy to move them to a separate PR.Testing Instructions
install-plugin.sh editing-toolkit fix/cache-global-styles
wpcom-limit-global-styles
blog sticker to the new site from the Blog RC?preview-global-styles
param)?preview-global-styles
param