Use low-level cache for get_user_data_from_wp_global_styles(). #3517
Conversation
peterwilsoncc
force-pushed
the
gb44946-use-low-level-cache-for-user-data-global-styles
branch
from
16aac5e to
b75ab81
Compare
| @@ -456,8 +445,6 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post | |||
| $user_cpt = get_post( $cpt_post_id, ARRAY_A ); | |||
| } | |||
| } | |||
| $cache_expiration = $user_cpt ? DAY_IN_SECONDS : HOUR_IN_SECONDS; | |||
| set_transient( $cache_key, $user_cpt ? $user_cpt['ID'] : -1, $cache_expiration ); | |||
There was a problem hiding this comment.
Switching to low level caching in wp-query per discussion on ticket.
| $this->assertSameSets( $user_cpt, $new_user_cpt, "User CPTs do not match on run {$i}." ); | ||
| } | ||
| $query_count = count( $this->queries ) - $query_count; | ||
| $this->assertSame( 1, $query_count, 'Unexpected SQL queries detected for the wp_global_style post type after creation.' ); |
There was a problem hiding this comment.
This has increased from zero to one.
As the transient has been removed, it's no longer set on post creation so a single query is made on the following call (in run 0), in subsequent runs, it is cached.
There was a problem hiding this comment.
This change looks good to me, is testing well manually, and still appears to correctly return the most recent custom post type storing the user-defined global styles JSON rules for the currently active theme. Thanks for digging into this one @peterwilsoncc!
In terms of the issue in WordPress/gutenberg#44946, I think there is one other piece to the puzzle. For sites where the user has already made a change to global styles via the site editor UI, there will already be a custom post storing the global styles JSON data, so the imported will not overwrite that post — this means that the exported custom global styles will not be imported into the site. I believe that might need to be fixed up potentially in the importer logic? I have next to no knowledge of how importers work, so that's just a guess at this stage.
For testing this PR with the importer, I manually listed out the custom post type by running the following locally:
npm run env:cli -- wp post list --post_type=wp_global_styles
Then, I deleted the global styles custom post for my selected theme, ran the importer, and it imported correctly, and loading the site frontend and global styles editor both worked as expected.
In regards to fixing the reported issue, either way, I think the caching removal provided in this PR is a good idea, and the tests look good. If this lands in core, we should probably also copy it over to the Gutenberg plugin to make sure the plugin is running a consistent version of the caching logic.
|
I've done some performance analysis to see how this PR fares (compared it with trunk 54678, the one without the changes from this PR). Tested by loading the "hello world" post as a logged-out user.
The data variation says this PR (removing the cache) is faster by 0,1ms. On the overall performance of the request, this PR is slower by 2ms (794ms vs 797ms). I'm inclined to think this change does not have any actual effect on performance. I've also reviewed the PRs that introduced caching to this method: WordPress/gutenberg#36584 and #2414 and could not see any data, so I'm a bit blind as to what were the actual performance gains back there. From a performance point of view, this seems fine to land. |
|
As noted WordPress/gutenberg#44946 (comment), once the cache is in place, sure the caches are simple. However, with WP_Query this cache is far more likely to be invalidated and the As a workaround for this issue, I think we could commit this change. But I would like to look at a solution that caches global styles forever and correctly cache invalidates on change. But this would take more time than we have in this release cycle. |
|
@oandregal Question: Why is |
|
Thanks everyone! Merged into core in https://core.trac.wordpress.org/changeset/54706. |
|
Merged into Core's 6.1 branch by @dream-encode in https://core.trac.wordpress.org/changeset/54707. |
See WordPress/gutenberg#44946
See https://core.trac.wordpress.org/ticket/56901