Update transient cache for wp_add_global_styles_for_blocks
#6879
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| if ( ! is_array( $cached ) ) { | ||
| $cached = array(); | ||
| $cache_key = 'wp_styles_for_blocks'; | ||
| $cached = get_transient( $cache_key ); |
There was a problem hiding this comment.
Why is this changed to get_transient from get_site_transient?
There was a problem hiding this comment.
This cache needs to be site specific, because it uses global settings and the computed styles for blocks in the hash, which incorporates users settings. When the hash key is part of the cache key name, every site on a multisite network with different settings will generate unique keys, which will expire naturally in 10 minutes.
To avoid the extra DB lookup for a transient with an expiration, I'm looking at making the cache key consistent, and put the hash inside the cached data for invalidation, but that means we need to save separate transients for each site on a multisite install, rather than as a shared transient for the whole network.
There was a problem hiding this comment.
Thanks @spacedmonkey. The unit test failures are expected currently, because I didn't update the new tests yet. I wanted to get feedback on the approach first, but will update the test before making this change (if at all).
| if ( ! is_array( $cached ) ) { | ||
| $cached = array(); | ||
| $cache_key = 'wp_styles_for_blocks'; | ||
| $cached = get_transient( $cache_key ); |
There was a problem hiding this comment.
This cache needs to be site specific, because it uses global settings and the computed styles for blocks in the hash, which incorporates users settings. When the hash key is part of the cache key name, every site on a multisite network with different settings will generate unique keys, which will expire naturally in 10 minutes.
To avoid the extra DB lookup for a transient with an expiration, I'm looking at making the cache key consistent, and put the hash inside the cached data for invalidation, but that means we need to save separate transients for each site on a multisite install, rather than as a shared transient for the whole network.
|
@joemcgill On a side now, 
Same with 
|
This uses a consistent cache key with the cache busting hash saved to the cache value rather than as part of the key. This allows us to remove the expiry time for the cache and only invalidate when the hash changes. This also swaps the use of a site transient to the use of a regular transient so that the option will be autoloaded since there is no expiration time.
joemcgill
force-pushed
the
update/global-styles-for-blocks-cache
branch
from
d5fda60
to
3a66386
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
joemcgill
requested review from
tellthemachines,
andrewserong and
aaronrobertshaw
| if ( ! is_array( $cached ) ) { | ||
| $cached = array(); | ||
| // Hash the merged WP_Theme_JSON data to bust cache on settings or styles change. | ||
| $cache_hash = md5( wp_json_encode( $tree->get_raw_data() ) ); |
There was a problem hiding this comment.
This is a much simpler hash generation, and accounts for a much broader set of changes that might happen to the merged Theme JSON data, which could affect global block styles, which is less risky.
There was a problem hiding this comment.
This has been updated to also fix a bug that was reported in https://core.trac.wordpress.org/ticket/59595#comment:71 with testing instructions.
Thanks @joemcgill for updating this PR with a fix for the reported issue 👍
After applying the changes here, the global block styles update as expected on the frontend.
| Before Revert (ad135d5) | After |
|---|---|
Screen.Recording.2024-07-15.at.10.17.40.AM.mp4 |
Screen.Recording.2024-07-15.at.10.15.54.AM.mp4 |
It looks like this PR needs a rebase but in general it's looking pretty close to me.
|
Thanks all, I'll update this PR so it applies cleanly after the revert of the original caching so we can try again for 6.6.1 |
This uses a consistent cache key with the cache busting hash saved to the cache value rather than as part of the key. This allows us to remove the expiry time for the cache and only invalidate when the hash changes.
This also swaps the use of a site transient to the use of a regular transient so that the option will be autoloaded since there is no expiration time.
Update July 12, 2024
This has been updated to also fix a bug that was reported in https://core.trac.wordpress.org/ticket/59595#comment:71 with testing instructions.
Trac ticket: https://core.trac.wordpress.org/ticket/59595
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.