Why a filter for this? I realize there is wp_cache_themes_persistently in WP core, but if we don't reuse that same filter here, I don't see the benefit of a new filter. Where was introducing this filter discussed?

One of the reason to introduce the theme_files cache group instead of using the existing themes cache group is that the latter is subject to such a filter and therefore not persistent in most WordPress sites. This reduces the performance benefit at scale a ton. If we now introduce a similar filter here, that decision would have been pointless.

As far as I recall, from what we previously discussed, anything in the theme_files group should always be persistently cached. We were talking about making the expiration filterable, but not whether to persistently cache it.

From my perspective, the main purpose of this filter is to allow the TTL value to be adjusted for these caches, which is something we want to allow. We should still have that filter in place for that purpose, even if we don't allow the filter to be used as a way of disabling persistent caching of the theme_files group. If so, we should probably rename the filter to something like wp_cache_theme_files_ttl or something.

That said, I would prefer to leave the filter as a way to disable persistent caching for theme_files group entirely because it makes the caching APIs for themes a bit more consistent. The main reason we introduced a separate group is because the themes group is NOT cached persistently by default—requiring an opt-in to get the performance benefit, whereas the theme_files group WILL be persistent by default. Adding a filter does not invalidate the discussion from Trac 59719 unless the default value is false.

@joemcgill I would be in favor of limiting the filter to purely controlling the TTL. Since it's a separate cache group that already behaves differently from the themes group, I don't think consistency with the other filter is important here. I don't see a good reason why we would want to allow opting out of persistent caching for this group. There is no such filter for any other WP core cache groups either.

In that case, maybe even adding a filter for controlling the TTL of this group is unnecessary. Currently, the only way to filter the TTL for the cache_expiration property of the WP_Theme class is by passing an int value to the wp_cache_themes_persistently filter, so unless we want more granular control over the theme_files TTL than other theme related caches, the additional filter is probably overkill.

@joemcgill @felixarntz I have updated filter to wp_cache_themes_persistently and only using it for updating TTL.

As agreed we have gone ahead and only implemented TTL-based control to manage cache_expiry for now.

Not a must have for this PR, but given that there are other places to use the theme_files cache group with the transient fallback, it may be worth introducing 3 private methods in this class to abstract away this if else and thus make it more intuitive. For example:

• get_files_cache( $key )
• set_files_cache( $key, $data, $expiration )
• delete_files_cache( $key )

That way the dedicated methods here could just call that, and it would make it more obvious that other theme files caching logic should reuse this. $key would be 'wp_theme_patterns_' . $this->stylesheet in the code here.

I agree with this. I think we should wait and address that once we're sure we can add caches for block templates and template parts, which I think can be handled in a separate PR.

This if statement isn't required.

Transients automatically use the object cache if it's available

🔢 Applies throughout so I'll avoid repetition.

@peterwilsoncc As part of the ticket, it was decided to use the object cache functions when a persistent cache available, as storing in a transient cache results in different behavior. So this if clause is intentional, what you're proposing would not result in the same behavior.

@felixarntz Are you able to explain the different behavior you are worried about? I realise it would change the cache group but otherwise the timeout seems the same.

I think the conditional makes it overly complex for site maintainers and owners.

Using a transient allows a single command be used to flush the cache regardless of the site details wp transient delete [key] --network or the PHP equivalent for plugins.

Requiring different approaches depending on the site internals makes it quite unintuitive.

I think the conditional makes it overly complex for site maintainers and owners.

Not sure I agree with that statement. If you're the developer / technical maintainer, you should know whether the site uses a persistent object cache. If you're the non-technical site owner, you probably wouldn't deal with cache invalidation in the first place, let alone use WP-CLI.

To me the consistency of having it always as a transient doesn't justify the lack of granular cache control a dedicated cache group provides.

In other words: Since using a transient here is only a fallback for sites not using a persistent object cache, I wouldn't want to make the experience worse for sites that do have a persistent object cache, just to align with the "lowest common denominator".

@peterwilsoncc the main purpose of this PR and the related ticket, is to reduce certain expensive operations related to parsing block theme template files (i.e., patterns, templates, and template parts).

Initially, this was implemented using wp_cache_* functions and later added to a theme_files group to make it easier to flush this set of data when invalidation needs to be handled granularly. However, right now these caches only benefits sites that employ a persistent object cache. What we're trying to do here is extend the benefits of that persistence to all sites, while retaining the granularity that object caches already have.

Given that these caches are getting deleted any time WP_Theme::cache_delete() is called, maybe retaining that granularity isn't really needed? If so, then I think relying on the site-transient group for persistence could be fine. In practice, I'm not sure how many people are actively using the new theme_files group since it was just introduced.

Ideally, we would have a much friendlier way to flush a group of transients so that these theme related caches could be cleared in a consistent manner regardless of what system was being used to handle the caches.

To me the consistency of having it always as a transient doesn't justify the lack of granular cache control a dedicated cache group provides.

I'm not sure it provides the advantage though.

Both Core contributors and plugin developers will need to account for both situations when clearing the cache. Using a forked approach risks situations in which the cache is dumped for some systems and not others. Intermittent bugs like that are difficult to diagnose at the best of times, when it's due to system dependencies outside the developers control, it's even more difficult.

I used the WP CLI approach as a low level example: in reality maintainers and extenders will use th PHP functions delete_site_transient and wp_cache_delete(), etc to manage the caching. This will happen no matter how well documented it is to use the method in this class.

After a detailed discussion on slack over whether to use a forked method or just rely on the transient functions wp_cache_*() , we have concluded that transient functions provide all the benefits of a normal cache function, with the exception that we can't clear the cache by the "theme_files" group.

For those who wish to clear the cache based on a group, they can do so by the using "site-transient" group. This does have a downside in that it would clear more than just the theme file cache, but it would still not affect "high frequency" caches like posts, etc.

I have modified the code in favour of this discussion.

Per https://github.com/WordPress/wordpress-develop/pull/6137/files#r1520119762, I think the filter should be limited to only control cache expiration which is what we had originally discussed in the ticket:

Done.

This if statement isn't required.

Transients automatically use the object cache if it's available

🔢 Applies throughout so I'll avoid repetition.

It's probably worth hashing the stylesheet name for the cache key.

It's a super edge case but if the stylesheet is longer than 130ish characters you'll exceed the maximum allowed length for a transient.

Done.

We can't reuse that filter here, as it does not behave the same (the existing filter uses a boolean while here we only support integers). Additionally, the filter name does not make sense for a filter that controls the cache TTL. The current name is more applicable for a "boolean" decision whether or not to cache, but that's not what we're doing here.

How about wp_theme_files_cache_ttl for the new filter's name?

I think we should do a bit of validation after the filter. For example, it could return 0 or even negative values, which should not be used.

I think the simplest solution is to reuse the original value if the filter returns something invalid. As in:

if ( $cache_expiration <= 0 ) {
	$cache_expiration = self::$cache_expiration;
}

Agreed, I have also added a doing_it_wrong warning in case someone adds a below 1 or a non-int value

Since we're changing the cache keys in this PR already, it would make more sense to align it with how the class otherwise handles this:

• Use $this->cache_hash instead of md5( $this->stylesheet ). This is not only more efficient, but also more reliable as it prevents edge-case bugs where the same theme may be loaded from a different themes folder (since $this->cache_hash also takes the "theme root" into account).
• Use wp_theme_patterns- (ending in a hyphen instead of underscore). This is just a nit-pick, but brings consistency with how cache keys are connected to the hash elsewhere in the class.

Done.

I don't think this is a blocker, but I don't think we should disallow setting these to never expire. I think a better validation would be to not cast the return value above and throw a doing it wrong if the return value is not an int. That would keep someone returning false thinking they were turning off caching from accidentally caching this indefinitely. Happy to go with this for now and consider as a follow-up.

@joemcgill That would work for me, though my follow up question would be how you would "sanitize" the false then? Simply use the original self::$cache_expiration?

Yep, we'd basically do the same thing we're already doing, but instead of if ( $cache_expiration <= 0 ) { ... we'd do if ( ! is_int( $cache_expiration ) { ....

@kt-12 had originally implemented it this way actually and I had suggested to be more "forgiving" and cast to integer instead of warning when not returning an integer. Your argument around false makes sense to me, but my concern was more about someone returning '30' and that value failing, which really it doesn't have to fail since a string containing a number could be perfectly fine to let through (and cast to an integer).

But I'm onboard with your suggestion, and even with my caveat, there's always a benefit to use strict typing anyway.