Move caching of block patterns to object cache, rather than transients. by joemcgill · Pull Request #5533 · WordPress/wordpress-develop

joemcgill · 2023-10-19T18:47:34Z

This caches block patterns using WP_Object_Cache rather than transients. Caches are stored in the global group so sites with persistent caches can still get the performance benefit, while allowing this data to be cleared with a standard cache flush, while avoiding persistence issues for sites not using a persistent object cache.

Trac ticket: https://core.trac.wordpress.org/ticket/59633

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.

This caches block patterns using `WP_Object_Cache` rather than transients. Caches are stored in the global group so sites with persistent caches can still get the performance benefit, while allowing this data to be cleared with a standard cache flush, while avoiding persistance issues for sites not using a persistent object cache.

felixarntz

@joemcgill The approach looks reasonable to me, though I wonder about not using any specific cache group. Is there existing cache usage without a specific cache group in core? I'm not opposed of committing this without a specific cache group, but would want to make sure that's conforming with how core should use the object cache functions.

I realize you may be omitting this because neither the themes nor the theme_json groups are persistent cache groups, which we therefore shouldn't use here. We could still rely on a new group (which would thus be persistent as that is the default), maybe theme_files?

joemcgill · 2023-10-19T19:06:40Z

@felixarntz, my initial approach here is the lightest possible change—converting the transient functions to their wp_cache_ alternatives. The advantage here is that we're caching this to the global caching group so this data will still be persistent on environments with object caches available, while avoiding bugs for non-persistent sites who can't easily clear transients using standard cache flushing methods.

As is, I think this is the minimum viable solution for 6.4. However, while working on this, I'd like to open consideration of two other changes:

Cache Groups

I think keeping this in the global group for now is fine, and we can move it to a more appropriate group later, here are the alternatives I considered:

Use the themes group: By default this group is not persistent (reference). If this group is used, only sites who have filtered wp_cache_themes_persistently to true would receive the performance benefit, since these caches are only called once per request—when patterns are registered.
Use the theme_json group: This group is persistent, but currently is only used for data and files related to theme.json (settings, styles, etc.). Caching patterns here as well makes the purpose of this group less clear, which is why I think it's best to avoid.
Add a new caching group: Ultimately, we may want to add a new caching group here, but seems imprudent to do so during RC. I'd rather propose and get feedback on a new group structure for these types of caches during a full release cycle.

Optional architectural improvements

WP 6.4 adds three new public methods to the WP_Theme to get/set/delete the pattern cache. These methods are only used in core by the private function _wp_get_block_patterns() which is called by the private _register_theme_block_patterns() on init. I don't like the idea of adding these public methods and needing to maintain them in the future. I think a much better solution is to move _wp_get_block_patterns() to a public method WP_Theme::get_block_patters() and make the new cache methods private. This would also be more consistent with the WP_Theme::get_block_template_folders() method, which is also being introduced in WP 6.4.

felixarntz

@joemcgill Thanks for clarifying. Fair points, I think we can go with the global group for now and iterate later as part of coming up with a more consistent approach in 6.5.

On a related note, regarding theme_json, I believe that group is non-persistent as well, so we wouldn't be able to use it anyway given we would like this data to be persistently cached for sites with an object cache.

It's also reassuring to see that all tests are still passing without any modifications 🎉

felixarntz · 2023-10-19T19:19:43Z

@joemcgill Regarding the additional architectural considerations:

WP 6.4 adds three new public methods to the WP_Theme to get/set/delete the pattern cache. These methods are only used in core by the private function _wp_get_block_patterns() which is called by the private _register_theme_block_patterns() on init. I don't like the idea of adding these public methods and needing to maintain them in the future. I think a much better solution is to move _wp_get_block_patterns() to a public method WP_Theme::get_block_patters() and make the new cache methods private. This would also be more consistent with the WP_Theme::get_block_template_folders() method, which is also being introduced in WP 6.4.

Generally, I agree this would be a better solution. It's late in the cycle for this kind of change, but if we can make it happen with minimal code changes, let's do it.

So we would remove _wp_get_block_patterns() and instead call WP_Theme::get_block_patterns()?

joemcgill · 2023-10-19T19:40:16Z

Generally, I agree this would be a better solution. It's late in the cycle for this kind of change, but if we can make it happen with minimal code changes, let's do it.

I agree that it’s late. The only reason I feel it’s worth doing now is because once those methods ship, they need to be maintained, so now seems better. I’ll update this PR accordingly.

This moves `_wp_get_block_patterns` to a new public method of the `WP_Theme` class called `get_block_patterns` and makes all of the related caching methods private to avoid future compatibility issues.

src/wp-includes/class-wp-theme.php

joemcgill · 2023-10-19T21:40:20Z

src/wp-includes/class-wp-theme.php

 		return $this->block_template_folders;
 	}

+	/**


Reviewer note:

These methods are just moved from above, so they are organized with other block-related methods. The only changes are that the set_pattern_cache and get_pattern_cache are now private, while the delete_pattern_cache remains public for cache clearing purposes.

joemcgill · 2023-10-19T21:40:53Z

tests/phpunit/tests/blocks/wpGetBlockPatterns.php

@@ -1,35 +1,55 @@
 <?php


This file name needs to be updated, but let's agree on the method name first.

See my other comment, maybe tests/phpunit/tests/theme/wpThemeGetBlockPatterns.php?

felixarntz

@joemcgill This looks great to me and pretty much good to go. A few last notes regarding your existing comments on naming.

tests/phpunit/tests/blocks/wpGetBlockPatterns.php

felixarntz · 2023-10-19T23:27:47Z

tests/phpunit/tests/blocks/wpGetBlockPatterns.php

@@ -1,35 +1,55 @@
 <?php


See my other comment, maybe tests/phpunit/tests/theme/wpThemeGetBlockPatterns.php?

mukeshpanchal27

Thank you, @joemcgill, for the PR. I have some minor suggestions.

We should rename the file from wpGetBlockPatterns.php to getBlockPatterns.php since we didn't use the wp_ prefix.

tests/phpunit/tests/blocks/wpGetBlockPatterns.php

joemcgill · 2023-10-20T13:44:14Z

Thanks @mukeshpanchal27! I've applied your message suggestions in c35134e. I renamed and moved the test file, per @felixarntz's suggestion in 2ff2d69.

felixarntz · 2023-10-20T14:21:36Z

@joemcgill Not sure why, and you probably already saw it, but it looks like unit tests are currently failing. I'm not sure what you could have changed that led to this, as they were passing yesterday and the recent changes were just about renaming?

joemcgill · 2023-10-20T16:52:25Z

@felixarntz I've spent the morning tracking down why the tests started failing, and it looks like moving the file into the themes directory exposed an existing problem with our test suite, where development mode had gotten set to 'theme' (which causes the caching we're testing in this file to return false). I've fixed that issue in the tests/phpunit/tests/theme/wpGetGlobalStylesheet.php test class, and updated the test class for this functionality to be a bit more resilient against external changes in 88fa6ec.

felixarntz

Thanks @joemcgill, that makes sense. I had run into a similar situation the other day when working on #5467, I should have considered to fix this holistically 🙃

One point of feedback below, we should rather unset the global than setting it to an empty string to clean up global state, but that's only a minor issue that could be fixed pre-commit.

tests/phpunit/tests/theme/wpGetGlobalStylesheet.php

tests/phpunit/tests/theme/wpThemeGetBlockPatterns.php

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

joemcgill · 2023-10-23T14:13:36Z

Merged in https://core.trac.wordpress.org/changeset/56978

joemcgill requested review from afercia, felixarntz and hellofromtonya October 19, 2023 18:47

felixarntz reviewed Oct 19, 2023

View reviewed changes

felixarntz approved these changes Oct 19, 2023

View reviewed changes

joemcgill added 3 commits October 19, 2023 14:50

Move block patterns getter to WP_Theme.

288114b

This moves `_wp_get_block_patterns` to a new public method of the `WP_Theme` class called `get_block_patterns` and makes all of the related caching methods private to avoid future compatibility issues.

Make WP_Theme::delete_pattern_cache public

0287428

Update unit tests

648d852

joemcgill commented Oct 19, 2023

View reviewed changes

src/wp-includes/class-wp-theme.php Show resolved Hide resolved

joemcgill and others added 3 commits October 19, 2023 16:26

Update src/wp-includes/class-wp-theme.php

d8bf013

Misc docblock and naming improvements to tests

9ac3057

CS cleanup

f3133fd

joemcgill commented Oct 19, 2023

View reviewed changes

joemcgill requested a review from felixarntz October 19, 2023 21:41

felixarntz reviewed Oct 19, 2023

View reviewed changes

mukeshpanchal27 reviewed Oct 20, 2023

View reviewed changes

tests/phpunit/tests/blocks/wpGetBlockPatterns.php Outdated Show resolved Hide resolved

tests/phpunit/tests/blocks/wpGetBlockPatterns.php Outdated Show resolved Hide resolved

joemcgill added 2 commits October 20, 2023 08:38

Rename, move, and cleanup test class

2ff2d69

Apply message suggestions to assertions

c35134e

Fix test failures due to development mode bleed

88fa6ec

felixarntz approved these changes Oct 20, 2023

View reviewed changes

tests/phpunit/tests/theme/wpGetGlobalStylesheet.php Outdated Show resolved Hide resolved

tests/phpunit/tests/theme/wpThemeGetBlockPatterns.php Show resolved Hide resolved

Unset _wp_tests_development_mode global

f83c7b2

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

joemcgill closed this Oct 23, 2023

joemcgill deleted the 59633/update-pattern-cache-persistance branch October 11, 2024 19:23

Conversation

joemcgill commented Oct 19, 2023

Uh oh!

felixarntz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joemcgill commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cache Groups

Optional architectural improvements

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

felixarntz commented Oct 19, 2023

Uh oh!

joemcgill commented Oct 19, 2023

Uh oh!

Uh oh!

joemcgill Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

joemcgill Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felixarntz Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joemcgill commented Oct 20, 2023

Uh oh!

felixarntz commented Oct 20, 2023

Uh oh!

joemcgill commented Oct 20, 2023

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joemcgill commented Oct 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

felixarntz left a comment •

edited

Loading

joemcgill commented Oct 19, 2023 •

edited

Loading