Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#58758 closed defect (bug) (fixed)

Cached result for wp_theme_has_theme_json() is not invalided when theme is switched dynamically

Reported by: westonruter's profile westonruter Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.2
Component: Themes Keywords: has-patch
Focuses: performance Cc:

Description (last modified by westonruter)

In #56975 the return value of wp_theme_has_theme_json() was cached unless the WP_RUN_CORE_TESTS or WP_DEBUG constants are true. (In #57487 the WP_DEBUG constant was replaced with a wp_get_development_mode() !== 'theme' check.) This workaround is needed to:

Ignore cache when automated test suites are running. Why? To ensure the static cache is reset between each test.

This is because core unit tests often switch themes, resulting in a stale cache. However, sites in the wild also can also dynamically switch themes, and plugins may switch themes during their own unit tests. It seems this check is not ideal and is masking a more fundamental problem: the cached value is not invalidated when the theme is dynamically switched.

A simple fix is to invalidate the cache whenever get_stylesheet_directory() or get_template_directory() change. This eliminates the need for the WP_RUN_CORE_TESTS entirely.

Change History (16)

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


10 months ago
#1

  • Keywords has-patch added

#2 @westonruter
10 months ago

  • Description modified (diff)

#3 @westonruter
10 months ago

  • Description modified (diff)

#4 @westonruter
10 months ago

  • Owner set to westonruter
  • Status changed from new to accepted

#5 @flixos90
10 months ago

Related: #58650

There is some history associated with why we went with a static for caching, so we should make sure performance doesn't degrade with this fix. I like the idea of this approach as it may address the other issues too. At the same time, it still feels cleaner to use the cache API as it supports proper invalidation.

If the added logic here doesn't cause a regression, I think it would be a good addition. But unrelated to that, we may want to find a better solution in the long term.

cc @spacedmonkey @noisysocks @ramonopoly

#6 @westonruter
10 months ago

After digging into the history a bit: originally object caching was used when this function was first introduced in [55086] by @hellofromTonya. However, the object caching was reverted in [55092] due to object cache not being available in wp-admin/load-styles.php. So then the static variable approach was settled on in [55138].

#8 @spacedmonkey
10 months ago

I agree that the current implementation is problematic. It basically guess that the stylesheet will not change in the course of a php request.

I have discussed added a parameter to the function in #45955. Little out of scope this, but related.

I also explored adding a method to the WP_Theme class, which would make context aware as well and would add caching. #3860

I think the best solution to this would be make $theme_has_support an array and use the stylesheet as a key. This would allow for themes to be switched and the key would change.

I put together a PR for review. #4820.


#9 @flixos90
10 months ago

@spacedmonkey I really like your solution, at least for the short term I think that's a clean approach. It would be great if you could add a unit test for it, otherwise this is good to go IMO.

#10 follow-up: @flixos90
10 months ago

@westonruter Just curious why you milestoned this for 6.2.3? Is it crucial to be backported? Otherwise, I'd say this would be fine to just go into 6.3.

#11 in reply to: ↑ 10 @westonruter
10 months ago

  • Milestone changed from 6.2.3 to 6.3

Replying to flixos90:

@westonruter Just curious why you milestoned this for 6.2.3? Is it crucial to be backported? Otherwise, I'd say this would be fine to just go into 6.3.

Oh right. Corrected!

@spacedmonkey commented on PR #4820:


10 months ago
#12

@felixarntz We already have a nice test suite for this in Tests_Theme_WpThemeHasThemeJson. There is already a test for switching themes. See test_switching_themes_recalculates_support.

@westonruter commented on PR #4816:


10 months ago
#13

Closing in favor of #4820

#14 @westonruter
10 months ago

  • Owner changed from westonruter to spacedmonkey
  • Status changed from accepted to assigned

#15 @spacedmonkey
10 months ago

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

In 56185:

Themes: Improved caching in wp_theme_has_theme_json().

Improve logic in wp_theme_has_theme_json() to improve caching soo it takes into account that a theme can be switched. Before this function would overly cache if the current theme has a theme.json file. Now it use the value of get_stylesheet() as a key to an array. This way, if the value of stylesheet is changed, by either a filter or in a unit test, a new check to see if the file exists is run. For this reason, the workaround to check if unit tests are running by checking if WP_RUN_CORE_TESTS constant exists can be removed.

Props westonruter, spacedmonkey, flixos90.
Fixes #58758.

Note: See TracTickets for help on using tickets.