Make WordPress Core

Opened 15 months ago

Closed 11 months ago

Last modified 11 months ago

#57886 closed defect (bug) (fixed)

get_post_templates includes results of get_block_templates while caching (WP_Theme)

Reported by: maniu's profile maniu Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: minor Version: 5.9
Component: Themes Keywords: has-patch needs-unit-tests
Focuses: multisite, performance Cc:

Description

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.

Change History (13)

#1 @hellofromTonya
15 months ago

  • Version changed from trunk to 4.7

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.

#2 @spacedmonkey
15 months ago

  • Focuses performance added
  • Version changed from 4.7 to 5.9

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.

#3 @maniu
15 months ago

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

This ticket was mentioned in Slack in #core-test by juhise. View the logs.


14 months ago

#5 @oglekler
14 months ago

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.

#6 @maniu
14 months ago

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

This ticket was mentioned in PR #4321 on WordPress/wordpress-develop by @maniu.


14 months ago
#7

  • Keywords has-patch added; needs-patch removed

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

#8 @spacedmonkey
13 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Owner set to spacedmonkey
  • Status changed from new to assigned

#9 @mukesh27
13 months ago

  • Keywords needs-unit-tests added

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

#10 @joemcgill
13 months ago

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.

#11 @spacedmonkey
12 months ago

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

#12 @spacedmonkey
11 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55939:

Themes: Fix caching issue in get_post_templates method in WP_Theme.

Fix an edge case caching issue, where if a developer use the wp_cache_themes_persistently filter and is running the site in multisite configuration, it meant block template are incorrectly cached. Block templates are stored in the posts table. But in a multisite configuration, different sites on the multisite could have different block templates stored in there post table. As themes cache group is a global group, it resulted in incorrect values being cached.

Props maniu, spacedmonkey, hellofromTonya, oglekler, mukesh27, joemcgill.
Fixes #57886.

@spacedmonkey commented on PR #4321:


11 months ago
#13

Comitted.

Note: See TracTickets for help on using tickets.