pantheon-systems / wp-redis

WordPress Object Cache using Redis.
https://wordpress.org/plugins/wp-redis/
GNU General Public License v2.0
225 stars 68 forks source link

Allow filtering / removal of global cache groups #330

Closed JanThiel closed 2 years ago

JanThiel commented 2 years ago

Hi there,

we have a setup where several of the global_cache_groups must not be global. Currently there is no way to remove anything from the preconfigured global cache groups.

Global Cache Groups are set here as input from the outside: https://github.com/pantheon-systems/wp-redis/blob/7dae4488e68ac71a40d7208581651e1ad758ac1b/object-cache.php#L425-L435

The only time global cache groups are set is this call in WP Core:

    if ( function_exists( 'wp_cache_add_global_groups' ) ) {
        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' ) );
        wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) );
    }

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

There is no way of hooking into this process. No filters, nothing. This might be due to the fact, that the caching happens early and WP isnt fully loaded.

To solve this we currently patched this code in the wp-redis object cache:

if ( ! defined( 'WP_REDIS_DEFAULT_EXPIRE_SECONDS' ) ) {
        define( 'WP_REDIS_DEFAULT_EXPIRE_SECONDS', 0 );
}

if ( ! defined( 'WP_REDIS_IGNORE_GLOBAL_GROUPS' ) ) {
        define( 'WP_REDIS_IGNORE_GLOBAL_GROUPS', array() );
}
        /**
         * Sets the list of global groups.
         *
         * @param array $groups List of groups that are global.
         */
        public function add_global_groups( $groups ) {
                $groups = (array) $groups;

                // Filter global groups
                $groups              = array_diff_key( $groups, WP_REDIS_IGNORE_GLOBAL_GROUPS );

                $groups              = array_fill_keys( $groups, true );
                $this->global_groups = array_merge( $this->global_groups, $groups );
        }

This allows one to configure an array of cache groups being disallowed as globals. Due to the array in the constant it works only with PHP >7.

If there are smarter ways to solve this, we are not picky. Just looking for a reasonable working solution for this :-)

Thanks so far and best regards, Jan

danielbachhuber commented 2 years ago

Thanks for the report, @JanThiel.

I'm amenable to your proposed change. I think this is a very niche issue, and can't think of a better way to solve for it.

To avoid unnecessarily breaking < PHP 7 compatibility, let's do something like this:

 if ( ! defined( 'WP_REDIS_IGNORE_GLOBAL_GROUPS' ) ) {
        define( 'WP_REDIS_IGNORE_GLOBAL_GROUPS', false );
}
if ( is_array( WP_REDIS_IGNORE_GLOBAL_GROUPS ) ) {
        $groups = array_diff_key( $groups, WP_REDIS_IGNORE_GLOBAL_GROUPS );
}

Want to submit a pull request for this? We should probably have a test around it too.

JanThiel commented 2 years ago

Thanks @danielbachhuber :-) Just finishing the PR right now ;-)

What about this?

if ( ! defined( 'WP_REDIS_IGNORE_GLOBAL_GROUPS' ) ) {
    if ( version_compare( PHP_VERSION, '7.0.0') >= 0 ) {
            define( 'WP_REDIS_IGNORE_GLOBAL_GROUPS', array() );
    } else {
        // PHP < 7 does not support array as values for constants. Use simple switch instead
        define( 'WP_REDIS_IGNORE_GLOBAL_GROUPS', false );
    }
}
danielbachhuber commented 2 years ago

Probably too complex for what we need, to be honest. I'd keep it simple.

Also, it'd be worthwhile to open a Core Trac ticket about this too. I don't expect it to ever be solved in Core, but we'd want to keep tabs on any Core discussion that might impact what's in WP Redis.

JanThiel commented 2 years ago

FYI @danielbachhuber https://core.trac.wordpress.org/ticket/54303

danielbachhuber commented 2 years ago

Thanks @JanThiel ! Great ticket 😄