Make WordPress Core

Opened 3 years ago

Last modified 23 months ago

#54303 new enhancement

[Object Cache] Allow global caching groups to be filterable / changed

Reported by: janthiel's profile janthiel Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 5.8.1
Component: Cache API Keywords: dev-feedback has-patch has-unit-tests
Focuses: multisite Cc:

Description

The Object Caching system differentiates between "local cache groups" and "global cache groups". This is essential for Multisite setups.

Global cache groups store cached objects - as the name implies - globally. There is one cached object for all sites in the network. Whereas the local cache groups are stored individually per site.

The cache groups are setup using the wp_cache_... API. Like https://developer.wordpress.org/reference/functions/wp_cache_add/

Which groups are actually global is setup in the Object Cache bootstrapping code here:

https://github.com/WordPress/WordPress/blob/e791d7f5db8252ba66c7777c456ed80343a450aa/wp-includes/load.php#L732-L735

wp_cache_add_global_groups( array( 'users', 'userlogins', 'usermeta', 'user_meta', 'useremail', 'userslugs', 'site-transient', 'site-options', 'blog-lookup', 'blog-details', 'site-details', 'rss', 'global-posts', 'blog-id-cache', 'networks', 'sites', 'blog_meta' ) );

This is hardcoded and not filterable in any (reasonable) way. Currently you have to rely on the actual Object Caching implementation to allow one to change the global cache groups.

The current globals setup is fine for the assumption of a default WordPress installation. But as it is with customizable software like WordPress, this is not true in any case.

For example: If one would split the user information and move it from the (global) network level to the individual sites, one would have to remove all the user related groups from the global cache groups.

This is currently not possible at all.

There are some possible solutions:

  1. Make the array of global cache groups filtereable
  • This might be a timing issue because the object cache system is loaded very early. But it might work.
  1. Add a function to the Object Cache API to actually remove cache groups from the globals list.
public function remove_global_groups( $groups ) {
    $groups = (array) $groups;
 
    $this->global_groups = array_diff_key( $this->global_groups, $groups );
}

This could then be leveraged from plugins at any point of time. Yet it still might kick in too late due to the early nature of the object cache system.

  1. If timing issues are an issue, instead of the filters, one could make the filtering available using a constant ( WP_OBJECT_CACHE_NON_GLOBAL_GROUPS => array( 'users', 'usermeta' ) ). Yet this depends on PHP7+
  1. Configure the global cachegroups late when filters are available and allow default WP filtering

Is this an Edge case? Yes! But it still is quite a burden on the flexibility of the Object cache that could be solved :-)

Change History (18)

This ticket was mentioned in PR #2149 on WordPress/wordpress-develop by JanThiel.


2 years ago
#1

  • Keywords has-patch has-unit-tests added; needs-patch removed

Allow force ignoring of global caching groups in WP_Object_Cache by adding constant in wp-config.php

Trac ticket: [](https://core.trac.wordpress.org/ticket/54303)

JanThiel commented on PR #2149:


2 years ago
#2

Where would the required constant be placed best in the Unit tests? In the bootstrap.php ?

JanThiel commented on PR #2149:


2 years ago
#3

@tillkruss Thanks for your feedback. The QS check made me remember, why I did it this way:
array() as constant content is a PHP7+ feature. I believe we must not assume PHP7 as a min requirement for WordPress at the moment.

tillkruss commented on PR #2149:


2 years ago
#4

The QS check made me remember, why I did it this way: array() as constant content is a PHP7+ feature. I believe we must not assume PHP7 as a min requirement for WordPress at the moment.

Right, that's annoying. Not sure what the core standards are here. Maybe supporting comma separated strings?

peterwilsoncc commented on PR #2149:


2 years ago
#5

I believe we must not assume PHP7 as a min requirement for WordPress at the moment.

Hold the thought for now, @hellofromtonya and I have had some very preliminary discussions about whether to re-propose a minimum version bump.

Not sure what the core standards are here. Maybe supporting comma separated strings?

I'm not sure there is one. A few popular memcached plugins use a simple global variable for defining multiple servers in the config file. A comma seperated value exploded and trimmed could work but doesn't seem the most optimal code.


I was going to propose a filter but then remembered that would be too late for the object cache. A constant is most likely the best option.

JanThiel commented on PR #2149:


2 years ago
#6

Hold the thought for now, @hellofromtonya and I have had some very preliminary discussions about whether to re-propose a minimum version bump.

That would be a good thing to do for a 6.0 major release :-)

I'm not sure there is one. A few popular memcached plugins use a simple global variable for defining multiple servers in the config file. A comma seperated value exploded and trimmed could work but doesn't seem the most optimal code.

As written in the ticket, this feature is a very edge case and will most probably only be used by people who "know what they do" and thus do not use PHP5 anymore for a long time ;-)
If PHP7 will be the new min version, the current version of the patch is the way to go.

I was going to propose a filter but then remembered that would be too late for the object cache. A constant is most likely the best option.

Exactly ;-) Tricky timing there...

What about this: I will revert back to default the constant to false allowing anyone with PHP7 to add an array to their wp-config. When the WP min PHP version gets bumped, I will change this code accordingly to the correct type. This would allow a short term merge and avoid relying on something that might possibly not happen at all. Although I would fully support the PHP min version bump personally.

hellofromtonya commented on PR #2149:


2 years ago
#7

Summarizing the chat @JanThiel and I had in Slack DM:

The goal: give advanced devs the ability to configure which global groups they want to use.
Frequency: edge case

Limitations:

  • Sites on PHP 5.6 cannot use this feature. Why? A constant array value can only be used when running on PHP 7 or above.
  • A constant is not testable with automated tests. This code change must be manually tested. See reasons below.

Risks:

  • The configuration mechanism must be implemented in each 3rd party object cache implementation. This means a dev may configure it in wp-config.php but it might not be used by their or their web host's 3rd party object cache.
  • No automated tests to validate the code functions as expected with both default value and a group array dataset. (see the reasons below.


## A constant is not testable
A PHP constant is not testable as it can only be defined once and never changed during the execution lifecycle. This include running tests in a separate process. Why not testable? Because a constant's default value can be reset between tests.

## How to make it testable and available to devs across all supported PHP versions?
Use a global variable instead of a constant. I don't like global variables either because they are easily changeable in plugins and themes and therefore require more guarding considerations. But in this case, it does fit the need.

Why?

  1. This is a low level area that runs before very early in the execution cycle, meaning less risk of improper manipulation by a plugin or theme.
  2. No limitations for devs to use it. Why? An array in a global variable works on all of WordPress' current supported PHP versions including PHP 5.6. And for those projects that need to support earlier PHP versions, it works there too.
  3. A global variable is testable. This means automated tests can be built and run to ensure (a) it works with the default value and a group dataset, (b) there are no future regressions, and (c) no impact on other tests.

## Recommendation and discussion
I'm leaning towards recommending a global variable to be added to the wp-config.php file instead of a PHP constant (for the reasons above).

@peterwilsoncc @JanThiel what do you think?

tillkruss commented on PR #2149:


2 years ago
#8

An optional global variable in the format of the existing $wp_object_cache_xyz makes sense.

JanThiel commented on PR #2149:


2 years ago
#9

Thank you very much @hellofromtonya.
Your arguments are very compelling and I do not see any reason against the global variable as this mechanism is used by many plugins for configs as well.

I will change the PR accordingly.

hellofromtonya commented on PR #2149:


2 years ago
#10

@JanThiel can you rebase your PR against trunk too please? Once done and the global implemented to replace the constant, then the failing tests can be debugged.

hellofromtonya commented on PR #2149:


2 years ago
#11

Whoops, didn't mean to close it. Re-opening 🤦‍♀️

JanThiel commented on PR #2149:


2 years ago
#12

@hellofromtonya Trunk is merged and the open codestyle fix is also applied

tillkruss commented on PR #2149:


2 years ago
#13

@JanThiel: I'm just looking at #2368, should we maybe rename the constant from WP_OBJECT_CACHE_IGNORE_GLOBAL_GROUPS to WP_CACHE_IGNORE_GLOBAL_GROUPS?

tillkruss commented on PR #2149:


2 years ago
#14

@hellofromtonya Is there anything we can do to push this forward?

@spacedmonkey Any feedback?

peterwilsoncc commented on PR #2149:


23 months ago
#15

So I am having some half-thoughts here...

The plugin API is a really need standalone library that can be included in any PHP code base, not just WordPress. As such, it can even be bootstrapped really early in the wp-config.php file

{{{php
/* wp-config.php */

Load the plugin API (like add_action etc) early.
require_once DIR . '/wp-includes/plugin.php';

Database settings - You can get this info from your web host
/ The name of the database for WordPress */
define( 'DB_NAME', 'database_name_here' );

/ Database username */
define( 'DB_USER', 'username_here' );

/ Database password */
define( 'DB_PASSWORD', 'password_here' );

ETC, ETC
}}}

WordPress currently has the enable_loading_advanced_cache_dropin filter that runs before plugins, mu-plugins, themes or anything a common configuration can use. However, it is possible to make use of it by including the plugin API in your wp-config file and adding a filter. _(Aside: please do not do this unless you know what you are doing.)_

As Jan mentioned above, this is a feature likely to be used by people who know what they are doing. Is it worth making the global and persistent groups filterable only if the site owner includes the plugin API in their wp-config file? _(Aside: please do not do this unless you know what you are doing.)_

With the WordPress Performance focus, there is a chance some of the non-persistent and global groups will be changing (for example Jonny has me reviewing #2478) so a filter would be more robust if additional groups are added to the existing default. If a variable is set to group_1, group_2 and WP then adds group_3, it is impossible to know the developers intent if the group is absent.

Using a filter will allow a developer who knows what they are doing to add or remove groups without risk.

Thoughts?

tillkruss commented on PR #2149:


23 months ago
#16

@peterwilsoncc: I'm with you on the logic, but the enable_loading_advanced_cache_dropin filter is only checked once. Using a filter here allow changes to the "global groups" to change during each request, which will cause havoc, a constant wouldn't 😕

peterwilsoncc commented on PR #2149:


23 months ago
#17

@tillkruss Isn't the idea to allow users to define the default values in these lines, they only run once during wp_start_object_cache() too?

https://github.com/WordPress/wordpress-develop/blob/f2697ea0426e0e900e22502b8fe48b5553b7cb11/src/wp-includes/load.php#L732-L735

Placing a filter within wp_cache_add_global_groups() or wp_cache_add_non_persistent_groups() could potentially be problematic (or at least need some really considered architecture) so, you are correct, it's probably best avoided.

tillkruss commented on PR #2149:


23 months ago
#18

That would be great! 🤙

Note: See TracTickets for help on using tickets.