get_post_templates includes results of get_block_templates while caching (WP_Theme)

[maniu]

WP_Theme->get_post_templates relies on global themes cache group. This means that the same results are being used on every subsite in a network. In the past, it did not lead to issues, but now, with FSE, page templates can be created in admin area, which complicates things.

get_block_templates function that is used in WP_Theme->get_post_templates loads templates created on subsite and later on caches those results. This leads to all sites in a network accessing the same cached results - site A can end up seeing templates of site B when editing a page. This becomes especially problematic if wp_cache_themes_persistently filter returns true.

I think the solution here should be to move get_block_templates related code outside of cache regeneration part. This way, templates from files would be cached globally, but block templates would be unique per site in a network.

Am I missing anything? If not, I can prepare PR for that.

[hellofromTonya]

Hello @maniu,

Welcome back to WordPress Core's Trac! Thank you for reporting this issue.

I'm adjusting the Version as this was not introduced in 6.2.0. When was it introduced?

This code was original introduced in WordPress 3.4.0 via [20029] / #20103 but was for only page post types. In WordPress 4.7.0 via [38951] / #18375, it was changed to support all post types and renamed from WP_Theme::get_page_templates() to WP_Theme::get_post_templates().

So I'll change the Version to 4.7.0.

Also pinging @spacedmonkey for caching performance input.

[spacedmonkey]

Okay, I see the issue.

If you set wp_cache_themes_persistently to true by filter, are running multisite, are using a block theme and have custom templates in the database, then as themes is a global cache, then the last site to change the cache, will overwrite changes.

As this is FSE related, I am changing this to 5.9 version.

This is an oversite when this was created. Sadly there is not a simple solution for this. ​This is the code that is the problem.

Some possibly solutions.

• If the theme is a block theme, make the cache group not a global cache group.
• Prefix the template with the site id.
• Do not cache this data.

All of these have their downside. I am going to assign this to myself, so I can take a look into it. Also going to ping the performance lead @flixos90 for his thoughts on it.

[maniu]

@spacedmonkey considering those recent improvements in WP_Query - ​https://make.wordpress.org/core/2022/10/07/improvements-to-wp_query-performance-in-6-1/, not caching this data in global themes cache group (moving it outside of the part that regenerates cache in WP_Theme->get_post_templates) sounds like good enough solution to me.

[oglekler]

I wonder is it is possible to cache block templates in regard to the current site in case of a multisite installation by adding a prefix to template cache key accordingly.

[maniu]

@oglekler all the theme data beside block templates is the same on the whole network. I think it would be waste o resources to do it like that, especially on bigger multisite installations.

I find my idea from previous comment to be the best solution to this problem.

Moves block templates outside global caching in WP_Theme->get_post_templates so those templates are not cached globally in the WP network.

Trac ticket: https://core.trac.wordpress.org/ticket/57886)](https://core.trac.wordpress.org/ticket/57886

[mukesh27]

As discussed in today's Performance bug scrub, we should need unit tests for the changes.

[joemcgill]

Caching theme templates separate from edited block templates, i.e. the result of get_block_templates() could make sense as long as we've optimized get_block_templates() and the logic that adds relevant block templates to the theme's post templates here. Otherwise, I think adding a site-specific cache for post templates that includes the current site ID would be good. Ideally, we'd like to enhance this whole process so that it CAN more reliably be cached persistently, but fixing the immediate bug described here is more important in the short term, in my opinion.

@spacedmonkey we could probably do some profiling comparisons here to make sure this doesn't cause a performance regression.

[spacedmonkey]

@joemcgill Sadly, even through this change will mean worse performance, I think that we have to make. Data being incorrectly cache, meaning the invalidation would not be in place. This change has to happen for data integrating reasons.