Ensure theme files caching mechanisms are filterable and caches can be manually cleared

[afercia]

Splitting this out from #59591.

#59591 surfaced the need of a broader, holistic discussion about theme files caching mechanisms. While caching is of course a welcomed performance benefit, it may introduce problems as pointed out in #59591.

Some of these problems will be mitigated by ​https://github.com/WordPress/wordpress-develop/pull/5462 and ​https://github.com/WordPress/wordpress-develop/pull/5495

However, there are scenarios where the caches won't be cleared, thus leading to potential PHP notices / warnings or non functional UI.

So far, theme files caching consists of a few parts:

• Caching of data from theme pattern files.
• Other instances of cached theme files are already in core, as mentioned in https://core.trac.wordpress.org/ticket/59591#comment:24
• A similar caching mechanism is in the works for template files.

Typically, the caches will be invalidated when:

• There is a theme update.
• The theme version is new.

However, #59591 highlighted that there are legitimate scenarios and user flows where the caches will not be cleared out. For example. when adding / deleting / renaming pattern files via FTP. This potentially applies to any other cached files.

Scenarios I could think of so far:

Incomplete themes updates
A theme update is unsuccessful ending up with incomplete / corrupted files or missing files. This is not uncommon especially for themes with a large amount of files, as the files write operation may simply fail. In these cases it's a common practice to upload the theme via FTP. Likely, WordPress won't understand the theme is 'new' and caches won't be cleared.

Hosting companies providing a staging environment
If the staging environment is provided by simply copying production to staging, I'm not sure the caches will be cleared and I'm not sure what would happen. Anyways, this seems to be a scenario that hasn't been considered in the current implementation.

Professionals and web agencies that manage sites for their customers
They may want to provide custom pattern files to their customers by simply uploading the pattern file via FTP. Caches won't be cleared.

To my understanding, any flow that bypasses a theme update via the WordPress admin and a theme version compare would not clear the caches.

There's likely several scenarios where this may happen that I can't think of at the moment. Any additional case and concerns are welcome in the comments below.

As of now, these caching mechanisms can't be disabled other than setting the WP_DEVELOPMENT_MODE constant to theme. However, this constant is meant for developers. Users, site owners, site managers, and even agencies or hosting companies are not required to know what this constant is. In some cases they could not have the ability to change it anyways.

There may be cases where users (in the broader sense: final users, agencies, hosting companies etc.) would also want or need to disable caching entirely. To my knowledge, as fo now there is no way to disable it.

I'd like to invite everyone to better consider the current implementation. So far, I could think of a couple things:

Filters

I'd tend to think _any_ caching mechanism should be filterable in order to disable it. Ideally, there should be a filter to disable any caching everywhere and then a series of more granular filters to disable specific caching (e.g. only patterns, only templates, etc.0.

Provide a way to clear caches in the UI

It appears there are legitimate cases where users would need to manually clear the caches. As mentioned above, this need may arise when theme files are updated outside of the WP admin. Such an UI control could offer a quick, easy, way to solve any caching problem.

Overall, I'd think the work done so far has been remarkable and it is for the greater good of performance. However, performance should never come at the cost of breaking legitimate user flows or, even worse, potential breakages. In this regard, I'm not sure all the potential scenarios have been considered and I'd recommend extreme caution with the introduction of this feature in Core.

[hellofromTonya]

Hmm seems a typo happened in the Reporter username. Hello @afercia can you confirm please?

[flixos90]

@afercia As mentioned in our previous discussion on #59591:

Users, site owners, site managers, and even agencies or hosting companies are not required to know what this constant is.

People that don't touch code are not required to know what this constant is. People that do touch code however should know about it, just as they need to know about other constants for wp-config.php.

The average end user does not touch the theme, so there's no problem. In fact, it's common WordPress knowledge that touching the theme is discouraged since updates will override any of the changes - unless it's your own custom theme, but in that case you're not the average end user and can be expected to follow WordPress development principles and best practices.

I would like to understand what your issue is with setting a constant for this.

[hellofromTonya]

I'm moving this ticket to 6.5. Why?

6.4 RC1 is tomorrow. It's too late in the cycle to consider adding filters (which are an enhancement) or improving caching mechanisms for additional workflows.

In reading #59591 and this ticket, it seems the ask is to improve the theme caching strategies for additional potential user workflows. @afercia lists some of the workflows for consideration. These additional workflows appear to be enhancements, rather than bugfixes. And there's not yet consensus of these workflows do or do not fall into theme development mode.

What about considering ​https://github.com/WordPress/wordpress-develop/pull/5495 for 6.4?

• It's a big change, as such is concerning this late in the cycle.
• It's still being discussed and there's not yet consensus to move forward with it.
• As @flixos90 ​notes in the PR, it's a "small improvement" that needs tests and testing:

  Despite the above, what you're doing here partially addresses the concerns on the ticket, so I'm not strongly opposed to merging this as a small improvement. But we should have unit tests to make sure it doesn't introduce problems, especially this late in the cycle.
  

However, if these workflows are indeed a must-have, then likely the caching changes would shift to revert consideration, given the latest of the 6.4 cycle, to give more time to consider additional workflow needs. Thus, if indeed must-have for 6.4, then please move this ticket back into 6.4 to start revert discussions.

[afercia]

Replying to hellofromTonya:

Hmm seems a typo happened in the Reporter username. Hello @afercia can you confirm please?

Ahah yes, thanks for fixing it.

[afercia]

Replying to flixos90:

The average end user does not touch the theme, so there's no problem.

I think this is a little too optimistic assumption. I appreciate optimism, but I'm not sure it's a good base where to build solid software upon. They do touch code, and they will, forever.

I would like to understand what your issue is with setting a constant for this.

I'm not sure I fully understand your question.

Anyways, my point is not about what the nature of WP_DEVELOPMENT_MODE is. That's too technical and doesn't help understanding what WordPress users need. My point is about legitimate user flows that haven't been taken sufficiently into consideration when implementing the caching.

FTP was a big one. It has been only partially addressed in #59591 / [56931]. A more complete mitigation of the problem would be ​https://github.com/WordPress/wordpress-develop/pull/5495

I'd like to stress again on the point that there are very likely more legitimate scenarios that we can't think of right now and that will break user flows.

One of them appears to be working with the Theme File Editor at /wp-admin/theme-editor.php, which I will detail in a separate comment.

[afercia]

Regarding the Theme File Editor at /wp-admin/theme-editor.php I would like to better understand what happens with the pattern files caching mechanism in place.

To my understanding:

• WordPress provides users with the ability to edit theme files, including patterns, directly via the Theme File Editor. I don't want to enter the debate whether the Theme File Editor should be deprecated or not. That has been discussed at length for years. The point is users can edit pattern files via the admin UI.
• WordPress does _not_ provide users with the ability to edit wp-config.php via the admin UI (although there are plugins to do that).
• As such, users can edit pattern files via the admin UI but they cannot edit WP_DEVELOPMENT_MODE via the admin UI.
• As a consequence, I'm assuming that when editing pattern files via the Theme File Editor the changes won't have any effect on the front end because patterns are cached.
• The admin UI doesn't provide any way to clear the caches.

As I see it, this way WordPress 6.4 will ship with a non-fully-functional Theme File Editor.

This appears to be one more legitimate scenario that hasn't been taken into consideration.

If the above is correct, I'm not sure what the best way to solve this specific issue is at this point of the release cycle. I'm not sure making the Theme File Editor basically pointless for pattern files is a good thing for the release.

• Should block themes entirely disable the Theme File Editor?
• Should caches be invalidated when saving a file edited in the Theme File Editor?
• Any other thoughts?

@hellofromTonya when you have a chance, I'd appreciate your thoughts on whether this specific point warrants this ticket to be milestoned for 6.4.

[flixos90]

@afercia Saying how my "optimism" is not a good base to build software upon is not helpful and honestly dismissive. Please stop using personal remarks just because you disagree with my opinion.

Now to the facts. Let's decouple, as you are talking about a few different points:

• Users editing theme files via the built-in theme file editor is a good point, and I agree improving compatibility for that in regards to WP_DEVELOPMENT_MODE is a good idea.
• That editor indeed allows users to make file changes. Even then, they are making changes to the theme, but I agree in that case WordPress core should take care of it since they cannot necessarily control WP_DEVELOPMENT_MODE themselves.
• For FTP, or any other ways outside of WordPress core to edit the theme, that is not the case. You are then expected to remain up to date with latest development best practices for WordPress. So it is up to you to edit wp-config (which you certainly can in these situations) and set WP_DEVELOPMENT_MODE.

One last thing I want to clarify: This is not strictly a 6.4 issue. Certain theme.json file data has been cached since 6.2. It used to rely on WP_DEBUG as a temporary workaround, and since 6.3 on WP_DEVELOPMENT_MODE. Given this file can be edited in the theme file editor as well, we need to think about a holistic solution, not just part of it.

I think we should open a ticket for the theme file editor integration with the WP_DEVELOPMENT_MODE related caches. That one can be immediately actioned, while continuing to discuss larger implications of WP_DEVELOPMENT_MODE here.

[hellofromTonya]

Sorry for my delay in responding. Thanks for the ping.

The Theme Editor UI concerns: yes, good catch @afercia. As users can edit files in this editor UI, an enhancement is needed to improve compatibility as @flixos90 noted.

But this is not strictly a 6.4 issue, as the concerns pre-date 6.4. Thus, I don't think they are a blocker for the 6.4 release.

Rather, I agree with @flixos90 to address the Theme Editor UI in a separate ticket which can be targeted for 6.5.

[flixos90]

Thanks @hellofromTonya. I have opened #59662 for the theme file editor specific bug.

[afercia]

Replying to hellofromTonya:

But this is not strictly a 6.4 issue, as the concerns pre-date 6.4. Thus, I don't think they are a blocker for the 6.4 release.

Rather, I agree with @flixos90 to address the Theme Editor UI in a separate ticket which can be targeted for 6.5.

@hellofromTonya thanks for your feedback. My concerns are a little broader than the one specific to the Theme File Editor, which I'd call a new bug introduced in 6.4 anyways.

I still kindly disagree on rushing the pattern files caching in 6.4 but fair enough, the decision and responsibility is totally up to the release squad. Thanks.

[joemcgill]

I tend to agree with @afercia that there are many legitimate use cases when this new theme pattern cache can become stale and potentially lead to errors, where they would not get cleared by one of the cache invalidation methods that have been set in place, nor where using the WP_DEVELOPMENT_MODE constant is appropriate. The Theme Editor is one—quite legitimate—concern, but this also doesn't take into account sites that have their themes in evergreen development, where incremental updates are made and deployed via CI or any other automated method.

This is the first place where persistent caching has been added for theme files (we previously added the wp_core_block_css_files transient for core block styles in register_core_block_style_handles). There have been other places in the past where file caching for themes/plugins has been considered and reverted (e.g. [42242]).

I think we need to consider removing this transient, or pursuing one of the options @afercia suggested during RC to ensure sites can opt-out of this theme cache or clear it easily when changes are deployed. Moving back to the 6.4 milestone for discussion.

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

@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.

@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()?

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.

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

@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?

@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.

[joemcgill]

In 56978:

Themes: Make caches for block patterns clearable.

In [56765], theme block pattern files were cached to a transient as a performance enhancement. However, transients are not easily clearable when caches are flushed on environments not using a persistent cache, which can lead to errors if the theme files are renamed, edited, or moved.

This changes the caching mechanism to use wp_cache_set() instead, and caches these values to the global group so they are still persistent on environments using an object cache, and will be cleared by a cache flush.

In addition, the helper _wp_get_block_patterns has been moved WP_Theme::get_block_patterns for consistency with other block related theme methods and cache helpers for these values, WP_Theme::get_pattern_cache and WP_Theme::set_pattern_cache, have been made private.

Relevant unit tests updated.

Props: afercia, flixos90, mukesh27, joemcgill.
Fixes #59633. See #59591, #59490.

[joemcgill]

This is now fixed in trunk. Reopening for a second committer review before back-porting to the 6.4 branch.

[flixos90]

Giving my +1 for the backport here @joemcgill.

[joemcgill]

In 56979:

Themes: Make caches for block patterns clearable.

In [56765], theme block pattern files were cached to a transient as a performance enhancement. However, transients are not easily clearable when caches are flushed on environments not using a persistent cache, which can lead to errors if the theme files are renamed, edited, or moved.

This changes the caching mechanism to use wp_cache_set() instead, and caches these values to the global group so they are still persistent on environments using an object cache, and will be cleared by a cache flush.

In addition, the helper _wp_get_block_patterns has been moved WP_Theme::get_block_patterns for consistency with other block related theme methods and cache helpers for these values, WP_Theme::get_pattern_cache and WP_Theme::set_pattern_cache, have been made private.

Relevant unit tests updated.

Props afercia, flixos90, mukesh27, joemcgill.
Merges [56978] to the 6.4 branch.
Fixes #59633. See #59591, #59490.

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

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

[spacedmonkey]

@joemcgill While reviewing the last commit, I noticed a bug. There is no cache group assigned to the cache set/get/delete. This feels wrong.

There are also caching methods in WP_Theme, that could easily be reused here. These use the global cache group themes, which makes sense.

For your consideration. ​https://github.com/WordPress/wordpress-develop/pull/5555

[flixos90]

@spacedmonkey I left a comment on your PR ​https://github.com/WordPress/wordpress-develop/pull/5555, as I hadn't spotted your comments here yet.

Not using any cache group was a deliberate decision, because we shouldn't use the non persistent themes group for this cache, as otherwise the caching would be useless.

For that reason, it was intentionally decided to go without a cache group for now, with the goal of revising the approach in 6.5. @joemcgill also explained that in the commit message for [56978]. I think we should close this ticket and focus on a more holistic solution in 6.5.

[hellofromTonya]

@joemcgill reached out to me before reopening this ticket to make me aware of this change being "the first place where persistent caching has been added for theme files". He had some ideas, such as replacing the transient or reverting the changes to continue in 6.5.

While [56978] and its backport to 6.4 [56979] are significant code changes, there are valid reasons:

• Provide the means to more easily clear the cache. Replaces the transient to use WP_Object_Cache. This should address the concerns raised by @afercia while also avoiding adding enhancements such as a filter during Beta/RC.

• Guard for future BC concerns to allow future iteration in 6.5 and beyond. Relocates newly introduced global internal helper functions into WP_Theme class with private visibility.

Replying to flixos90:

Not using any cache group was a deliberate decision, because we shouldn't use the non persistent themes group for this cache, as otherwise the caching would be useless.

For that reason, it was intentionally decided to go without a cache group for now, with the goal of revising the approach in 6.5. @joemcgill also explained that in the commit message for [56978]. I think we should close this ticket and focus on a more holistic solution in 6.5.

As the design was intentional and work will continue in 6.5 (in a different ticket), reclosing this ticket for 6.4.

Thank you everyone for addressing the concerns raised by @afercia while also minimizing the changes during RC. It's a solid compromise.