Cache `wp_theme_has_theme_json` via static variable #3887

oandregal · 2023-01-22T11:38:16Z

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

This PR implements caching to wp_theme_has_theme_json using a static variable.

hellofromtonya · 2023-01-23T19:36:44Z

Per discussions in the Trac ticket, f8d09bc removes the cache reset and $can_use_cache logic in wp_theme_has_theme_json(). The removal of the function param is also for BC considerations should this param not be needed.

However, a reset of some sort is still needed between automated tests. Else, tests will fail (as seen by the commit ❌ ).

hellofromtonya · 2023-01-23T19:48:48Z

src/wp-includes/global-styles-and-settings.php

+	/*
+	 * Ignore cache when `WP_DEBUG` is enabled. Why?
+	 * - to avoid interfering with the theme developer's workflow.
+	 * - to reset between each automated test.
+	 *
+	 * @todo Replace `WP_DEBUG` once an "in development mode" check is available in Core.
+	 */
+	if ( WP_DEBUG ) {
+		$theme_has_support = null;
+	}


As noted in #3887 (comment), a static cache reset is needed to reset after each automated test. This can be accomplished in many ways:

Checking constants like WP_DEBUG and/or WP_RUN_CORE_TESTS

Adding a hard reset parameter to the function

Filtering

Global

There is precedence within Core for using the WP_DEBUG and `WP_RUN_CORE_TESTS constants.

I'd suggest avoiding the approach of adding a reset param to the function. Why? (a) BC concerns for the future if the param is no longer needed and (b) performance concerns of exposing a hard reset in prod.

However if a hard reset option is needed, for example on theme switch or preview, then the param can be added and constant(s) removed.

cc @felixarntz @azaozz @oandregal What do you think?

Could also combine the check with early bail-out below.

@hellofromtonya +1 to avoid the function parameter, I'd be concerned about BC with that too.

IMO the WP_DEBUG approach is the most suitable for now, since that's what we were going to go with at this point anyway (keeping in consideration the @todo comment to iterate on it with a more appropriate solution). Later, once there is e.g. a filter for this too, we would be able to adjust the tests themselves to decide on whether or not the static variable cache should be used here or not. That's why I think WP_DEBUG works best for this for now.

Nit-pick, can you move it from the extra if clause to the existing one below? IMO no need to have a clause to reset the static cache value when we could instead not use the static cache at all. Having WP_DEBUG in the existing clause below is also better in line with how we would like to adjust this code in the future.

FWIW, I think none of that is really great, but I think keeping the static and relying on WP_DEBUG has the least impact on anything global that we wouldn't be able to change later, and it's relatively close to where we want this code to go eventually.

felixarntz · 2023-01-23T20:15:58Z

src/wp-includes/global-styles-and-settings.php

+	if ( WP_DEBUG ) {
+		$theme_has_support = null;
+	}
+
+	if ( null !== $theme_has_support ) {


See my comment in https://github.com/WordPress/wordpress-develop/pull/3887/files#r1084500610:

Suggested change

if ( WP_DEBUG ) {

$theme_has_support = null;

}

if ( null !== $theme_has_support ) {

if ( ! WP_DEBUG && null !== $theme_has_support ) {

Thinking more about the constants, I agree with @azaozz to also use WP_RUN_CORE_TESTS specifically for the tests. In this way, it provides clarity on the purpose of both constants, especially for the future when a "development mode" is introduced into Core.

d0ee044:

consolidates the WP_DEBUG check as requested

checks WP_RUN_CORE_TESTS per Cache wp_theme_has_theme_json via static variable #3887 (comment) and Cache wp_theme_has_theme_json via static variable #3887 (comment)

@azaozz and @felixarntz can you re-review please? Thanks!

azaozz · 2023-01-24T01:08:55Z

+1 to using WP_RUN_CORE_TESTS to invalidate/reset the cache when running tests. That's the primary use for this constant, right?

Regarding use of WP_DEBUG to invalidate the cache while developing. Like @felixarntz thinking this is okay for now and should be replaced once core has a (well established) way of setting development mode. There's a trac ticket for that: https://core.trac.wordpress.org/ticket/57487.

The only case that seems a bit unclear when using a static is whether invalidating the cache may be needed while PHP is running. Reading through the newer comments on https://core.trac.wordpress.org/ticket/56975, seems it will not be?

src/wp-includes/global-styles-and-settings.php

spacedmonkey · 2023-01-24T14:38:58Z

2021 theme.

Before

After

This saves 40+ calls to is_readable which saves 48ms per page request. This is worth doing.

hellofromtonya · 2023-01-24T15:12:27Z

Awesome! Thanks @spacedmonkey for running this before and after analysis #3887 (comment) 👏

hellofromtonya

This patch LTGM as it implements the lean performance approach discussed in the Trac ticket. There seems to be good consensus for it 👍

felixarntz

Thanks @oandregal!

azaozz

Using WP_DEBUG (until better indication for "development mode" exists in core), and ! WP_RUN_CORE_TESTS looks good imho.

hellofromtonya · 2023-01-25T19:39:11Z

Thank you everyone for your contributions to land on this solution. Committed via changeset https://core.trac.wordpress.org/changeset/55138.

ribaricplusplus · 2023-03-25T08:13:18Z

2021 theme.

Before

After

This saves 40+ calls to is_readable which saves 48ms per page request. This is worth doing.

@spacedmonkey Sorry for not directly relevant question, but could you let me know which tool you used for this profiling?

oandregal and others added 3 commits January 22, 2023 19:31

Cache wp_theme_has_theme_json via static variable

42bca5e

Clean static cache upon events

51f5d3a

Remove cache reset

f8d09bc

Reintroduce WP_DEBUG to reset static cache

a67086a

hellofromtonya reviewed Jan 23, 2023

View reviewed changes

hellofromtonya requested review from felixarntz and azaozz January 23, 2023 19:56

felixarntz reviewed Jan 23, 2023

View reviewed changes

spacedmonkey self-requested a review January 24, 2023 12:48

Ignore cache when in dev or test mode

d0ee044

hellofromtonya requested a review from felixarntz January 24, 2023 14:08

spacedmonkey reviewed Jan 24, 2023

View reviewed changes

src/wp-includes/global-styles-and-settings.php Show resolved Hide resolved

hellofromtonya approved these changes Jan 24, 2023

View reviewed changes

felixarntz approved these changes Jan 24, 2023

View reviewed changes

azaozz approved these changes Jan 24, 2023

View reviewed changes

spacedmonkey approved these changes Jan 25, 2023

View reviewed changes

hellofromtonya closed this Jan 25, 2023

felixarntz mentioned this pull request Jan 25, 2023

Re-add caching to wp_theme_has_theme_json(), but only when not in a WP Admin request #3867

Closed

hellofromtonya mentioned this pull request Jan 26, 2023

Trac 56975 Stub functions in noop and restore caching #3872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache `wp_theme_has_theme_json` via static variable #3887

Cache `wp_theme_has_theme_json` via static variable #3887

oandregal commented Jan 22, 2023

hellofromtonya commented Jan 23, 2023

hellofromtonya Jan 23, 2023

hellofromtonya Jan 23, 2023

felixarntz Jan 23, 2023

felixarntz Jan 23, 2023

felixarntz Jan 23, 2023

hellofromtonya Jan 24, 2023

hellofromtonya Jan 24, 2023

azaozz commented Jan 24, 2023

spacedmonkey commented Jan 24, 2023

hellofromtonya commented Jan 24, 2023

hellofromtonya left a comment

felixarntz left a comment

azaozz left a comment

hellofromtonya commented Jan 25, 2023

ribaricplusplus commented Mar 25, 2023 •

edited

Loading

2021 theme.

Before

After

Cache wp_theme_has_theme_json via static variable #3887

Cache wp_theme_has_theme_json via static variable #3887

Conversation

oandregal commented Jan 22, 2023

hellofromtonya commented Jan 23, 2023

hellofromtonya Jan 23, 2023

Choose a reason for hiding this comment

hellofromtonya Jan 23, 2023

Choose a reason for hiding this comment

felixarntz Jan 23, 2023

Choose a reason for hiding this comment

felixarntz Jan 23, 2023

Choose a reason for hiding this comment

felixarntz Jan 23, 2023

Choose a reason for hiding this comment

hellofromtonya Jan 24, 2023

Choose a reason for hiding this comment

hellofromtonya Jan 24, 2023

Choose a reason for hiding this comment

azaozz commented Jan 24, 2023

spacedmonkey commented Jan 24, 2023

2021 theme.

Before

After

hellofromtonya commented Jan 24, 2023

hellofromtonya left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

azaozz left a comment

Choose a reason for hiding this comment

hellofromtonya commented Jan 25, 2023

ribaricplusplus commented Mar 25, 2023 • edited Loading

2021 theme.

Before

After

Cache `wp_theme_has_theme_json` via static variable #3887

Cache `wp_theme_has_theme_json` via static variable #3887

ribaricplusplus commented Mar 25, 2023 •

edited

Loading