Control whether theme (e.g. theme.json) cache usage is enabled through a dedicated function

[felixarntz]

What problem does this address?

Several places (e.g. #45372, #45679, #45882) now check for the WP_DEBUG constant to decide whether to use WP_Object_Cache to cache expensive logic around theme.json parsing etc. It is still unclear whether that is the best approach, but most importantly, as this becomes more and more spread out, we should bring that into a single dedicated function. This function can then be used in all of these places, so that we can focus the conversation on the exact approach (even potential changes over the next few weeks) on just that function and not tons of other PRs.

Being able to bypass cache is crucial in these situations for theme developers while they are developing a theme. It is probably almost never relevant in production; e.g. when a theme is updated, we can manually invalidate cache anyway as that is detectable within WordPress. The problem is that direct file modifications e.g. during theme development are not detectable so we cannot invalidate cache reliably for that. To provide a good theme developer experience, we should therefore allow bypassing these specific caches.

What is your proposed solution?

• Introduce a function e.g. wp_theme_cache_enabled()
• Use either WP_DEBUG or in_array( wp_get_environment_type(), array( 'local', 'development' ), true ) for the baseline value. I would now lean towards the latter, as this is less about debugging than it is about working in a development environment (as you are developing a theme, that's when this is most relevant).
• Include a filter e.g. wp_theme_cache_enabled to allow for this to be modified specifically or at runtime.
• Use that function in all the above code instead of the "manual" WP_DEBUG checks, similar for any future code may add more use of WP_Object_Cache around theme related logic.

Let's discuss the exact logic within the function; but I think the overall function approach to have this centralized would definitely be a big plus already.

cc @mmtr @oandregal @spacedmonkey

[oandregal]

My main concern is what the developer experience is like. I'd recommend against using a filter for this reason, as it requires people to write a callback to the hook. In comparison, setting a constant is a widespread practice: it just works out of the box.

The second question is whether we should use WP_DEBUG or WP_ENVIRONMENT_TYPE or a specific one.

• I lean towards not creating more things to learn: unless creating a new variable provides a super high impact value, I'd reuse some of the existing.
• It's the first time I encounter the WP_ENVIRONMENT_TYPE variable. I suppose I'm not alone, so familiarity may be an issue. After having looked at its usage in the codebases (gutenberg, core) I see it is used to control some tests and different environments. This is problematic. For example, we still want caching enabled in a local environment, just not when you're debugging. I haven't looked at tests, but it sounds like it may be difficult to have a fine-grained control of the cache if the test require to use local environment, for example. It sounds to me that the goal we want to achieve here (being able to disable cache in some circumstances) is orthogonal to what this variable is meant for.

I don't see an issue with using WP_DEBUG, so I don't think we need to spend any time "fixing" or changing something that works.

[spacedmonkey]

Massive +1 to this. Is really bad to use WP_DEBUG here. Debug flag is that, a debug mode. Not this is designed to denote if you are in a local environment or in development mode. The big issue is, when profiling and debugging, I am unable to see what happens on production. If I want to debug an issue with the WP_DEBUG set to true, I can't because this code willl not let it work.

I vote for either a new const or use wp_get_environment_type. All places where we misuse use WP_DEBUG like this also be fixed.

[felixarntz]

@oandregal

My main concern is what the developer experience is like. I'd recommend against using a filter for this reason, as it requires people to write a callback to the hook. In comparison, setting a constant is a widespread practice: it just works out of the box.

You can set a constant based on my proposal. My proposal is to use an approach that caters for both simplicity and flexibility: Feel free to use a constant if that's sufficient for your use-case, or use a filter if you need a more dynamic approach. The constant would define what is the default value for the filter.

FWIW, writing a callback is not necessary if you want to use the filter for a simple way: You can simply use something like add_filter( 'wp_theme_cache_enabled', '__return_false' ) which isn't notably harder or more than define( 'WP_DEBUG', true ).

• I lean towards not creating more things to learn: unless creating a new variable provides a super high impact value, I'd reuse some of the existing.

+1, which is why I'm proposing a solution that allows reusing something existing.

• It's the first time I encounter the WP_ENVIRONMENT_TYPE variable. I suppose I'm not alone, so familiarity may be an issue. After having looked at its usage in the codebases (gutenberg, core) I see it is used to control some tests and different environments. This is problematic. For example, we still want caching enabled in a local environment, just not when you're debugging. I haven't looked at tests, but it sounds like it may be difficult to have a fine-grained control of the cache if the test require to use local environment, for example. It sounds to me that the goal we want to achieve here (being able to disable cache in some circumstances) is orthogonal to what this variable is meant for.

I don't see an issue with using WP_DEBUG, so I don't think we need to spend any time "fixing" or changing something that works.

I think either may be a reasonable approach, depending on what you're doing. I'm leaning towards WP_DEBUG being more useful, but there are circumstances where it is not the right solution and where for example WP_ENVIRONMENT_TYPE may be more useful. Let's not base this on (what I would say is very subjective) familiarity with constants, but rather what the purpose of the constants are.

To be clear, I'm still undecided myself which constant is more appropriate here, I'm slightly leaning towards WP_DEBUG. My main point though is: Neither of these constants is concretely for controlling theme cache, and that is a problem. Discussing which constant is the right one is missing this nuance IMO.

Using an existing constant as a baseline for controlling theme cache is good, as it allows to reuse what's familiar. Having that existing constant be the only way though lacks flexibility, since the constant has a way broader purpose and furthermore is a constant so can't be dynamically altered at runtime. This is why I'm proposing a function which would be a combination of constant (whichever we decide on) plus filter.

[oandregal]

So, if I understand correctly, your current view is that we should do wp_theme_json_cache_enabed and its implementation would be something along the lines of:

function wp_theme_json_cache_enabled() {
  return apply_filter( 'wp_theme_json_cache_enabled', WP_DEBUG );
}

Does that sound about right?

I don't see anything that would qualify as a blocker, though my main question is: why? Why do we need this special setup for the caches related to theme.json and not for the other caches core uses?

[felixarntz]

@oandregal

So, if I understand correctly, your current view is that we should do wp_theme_json_cache_enabed and its implementation would be something along the lines of:

function wp_theme_json_cache_enabled() {
  return apply_filter( 'wp_theme_json_cache_enabled', WP_DEBUG );
}

Does that sound about right?

Correct, something like that is what I am proposing.

I don't see anything that would qualify as a blocker, though my main question is: why? Why do we need this special setup for the caches related to theme.json and not for the other caches core uses?

Because the cache around theme.json is different from any cache that exists in WordPress core today: It is a cache around logic, which is controlled by code. Pretty much all object cache usage in core today is around database queries (or rather their results). There is a big difference in how these two cases need to be handled to support development: For database caches, this mostly doesn't matter since you changing the code doesn't change what's in the database (and thus no need to invalidate the cache around database results). That's different for the theme.json cache since you changing the code may change what the result of what is currently in the cache should be (and we can't invalidate that automatically during development since we don't detect PHP file changes). Also see what I wrote in the issue description:

The problem is that direct file modifications e.g. during theme development are not detectable so we cannot invalidate cache reliably for that. To provide a good theme developer experience, we should therefore allow bypassing these specific caches.

[oandregal]

Because the cache around theme.json is different from any cache that exists in WordPress core today: It is a cache around logic, which is controlled by code. Pretty much all object cache usage in core today is around database queries (or rather their results).

Interesting point, thanks for clarifying.

I went to look at another place I know that caches data coming from files and not database: WP_Theme. What I found is interesting: it uses a filter (but no constant) to decide whether the cache should be persistent across requests or not. There may be more places I am not familiar with. Given there is precedent, it does not seem too off to create a function specific for theme.json code, provided there is a constant theme authors can set.

Note that I insist in the constant as the primary API because of its proven good developer experience: its value does not rely on being an one-liner (like some filter would), but on working on multiple situations. For example: one line in wp-config.php, as an argument to some CLI calls (e.g.: WP_DEBUG=true <some-command>, they work well in a CI environment, etc.

[felixarntz]

@oandregal That's interesting indeed - also that the persistent caching there is off by default. It looks like another use-case than what we're doing around theme.json, but worth investigating further.

For reference, this was introduced a very long time ago in https://core.trac.wordpress.org/changeset/20020, as part of https://core.trac.wordpress.org/ticket/20103. Unfortunately no information provided really on what use-case would justify enabling the filter.

[felixarntz]

Leaving a note here to re-add the test that was removed in #45993, since this function will enable testing this again.

[felixarntz]

Cross-posting from the PR for this (#46791 (comment)):

@oandregal Circling back on this: Since the latest changes in Gutenberg (also for core e.g. WordPress/wordpress-develop#3556) is changing these caches to be non-persistent, the whole idea of having the WP_DEBUG check is not really useful anymore, given that the caches do not last beyond a single request. In other words, theme developers will not be impacted by a non-persistent WP core cache.

What this PR will eventually be useful for is once we can bring back persistent caching (by fixing all the quirks that previously led us to choose the non-persistent cache approach). Once we can use persistent caching without problems, we will need such a way to disable the cache again for theme developers, and only at that point this PR would become relevant really.

Can you please give this a look already (more importantly this comment than the concrete code in here)?

[joemcgill]

Now that WP_DEVELOPMENT_MODE is available, along with relevant helper functions, do we need to do any follow-up here?

[felixarntz]

Thanks @joemcgill for the reminder, this can indeed be closed now that we have WP_DEVELOPMENT_MODE. See https://make.wordpress.org/core/2023/07/14/configuring-development-mode-in-6-3/