Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete Cache by Group #2368

Closed
Closed
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
8a7dbb4
Added clearing of cache groups
pbearne Jan 4, 2022
bdca39b
Update src/wp-includes/cache.php
pbearne Jan 6, 2022
f87e341
removed full cache if group cache fails
pbearne Jan 7, 2022
a9b1db9
whitespace
pbearne Jan 7, 2022
576d82c
comments
pbearne Jan 7, 2022
21e6eac
refactor function and new test for new functions
pbearne Jan 10, 2022
f92ef14
dumped the function down
pbearne Jan 20, 2022
7abf5d6
removed not needed tests
pbearne Jan 20, 2022
ee6f101
removed not needed function
pbearne Feb 2, 2022
3d89efd
Merge branch 'trunk' into INITS-115-Support_flushing_cache_groups
pbearne Mar 1, 2022
baa5f32
Update src/wp-includes/cache.php
pbearne Mar 1, 2022
9305962
Update src/wp-includes/class-wp-object-cache.php
pbearne Mar 1, 2022
63730d5
Update src/wp-includes/class-wp-object-cache.php
pbearne Mar 1, 2022
a9a9e4d
Added fallback to cache-compat.php
pbearne Mar 1, 2022
ea41652
Merge remote-tracking branch 'origin/INITS-115-Support_flushing_cache…
pbearne Mar 1, 2022
095ab96
Added wp_cache_flush_group to get_pluggable_function_signatures test
pbearne Mar 1, 2022
8dcc405
Added array return if array is passed
pbearne Mar 1, 2022
a7bc773
removed not needed test wp_cache_get_linked_meta
pbearne Mar 2, 2022
670e754
removed whitespace changes
pbearne Mar 2, 2022
fb650ec
Update src/wp-includes/cache.php
pbearne Mar 8, 2022
2bcc284
Update src/wp-includes/cache.php
pbearne Mar 8, 2022
c65c89c
Update src/wp-includes/cache-compat.php
pbearne Mar 8, 2022
c03b962
Merge branch 'WordPress:trunk' into INITS-115-Support_flushing_cache_…
pbearne Mar 10, 2022
fcf0c8f
Update tests/phpunit/tests/cache.php
pbearne Mar 10, 2022
a65bc31
Update tests/phpunit/tests/cache.php
pbearne Mar 14, 2022
26fcbd8
Update tests/phpunit/tests/cache.php
pbearne Mar 15, 2022
7d715b7
Update src/wp-includes/cache-compat.php
pbearne Mar 21, 2022
3f26680
moved helper class to includes
pbearne Mar 21, 2022
b8c19b0
Merge branch 'WordPress:trunk' into INITS-115-Support_flushing_cache_…
pbearne Mar 24, 2022
e890990
removed method exists check
pbearne Mar 24, 2022
a0f699d
removed method exists check
pbearne Mar 24, 2022
47a5b5c
Merge branch 'WordPress:trunk' into INITS-115-Support_flushing_cache_…
pbearne Apr 5, 2022
d3ceb58
whitespaces
pbearne Apr 5, 2022
593dc79
added flush_group to memcache test code
pbearne Apr 5, 2022
4ecb1de
added flush_group to memcache test code
pbearne Apr 5, 2022
a26df6d
added check for method before calling
pbearne Apr 5, 2022
adac259
added check for method before calling
pbearne Apr 6, 2022
1829e23
Update src/wp-includes/cache.php
pbearne Apr 6, 2022
e76618b
Update src/wp-includes/cache.php
pbearne Apr 18, 2022
d5e7ed8
Update src/wp-includes/cache-compat.php
pbearne Apr 18, 2022
5e4433d
Update src/wp-includes/cache-compat.php
pbearne Apr 18, 2022
63228eb
Update src/wp-includes/cache.php
pbearne Apr 18, 2022
7f69e33
Update src/wp-includes/class-wp-object-cache.php
pbearne Apr 18, 2022
73fbd0b
added constant WP_OBJECT_CACHE_SUPPORTS_GROUP_FLUSH
pbearne Apr 19, 2022
156af11
Update src/wp-includes/cache.php
pbearne Apr 19, 2022
85d09da
Update src/wp-includes/cache-compat.php
pbearne Apr 19, 2022
f3f143a
Added WP_OBJECT_CACHE_SUPPORTS_GROUP_FLUSH to the memcache test cache…
pbearne Apr 19, 2022
3020e38
fixed whitespace
pbearne Apr 19, 2022
cd279c0
Update src/wp-includes/cache-compat.php
pbearne Apr 19, 2022
91a50b8
Update tests/phpunit/tests/cache.php
pbearne Apr 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/wp-includes/cache-compat.php
Expand Up @@ -141,3 +141,29 @@ function wp_cache_flush_runtime() {
return wp_using_ext_object_cache() ? false : wp_cache_flush();
}
endif;

if ( ! function_exists( 'wp_cache_flush_group' ) ) :
/**
* Removes all cache items in a group.
*
* @since 6.0.0
*
* @see WP_Object_Cache::flush_group()
* @global WP_Object_Cache $wp_object_cache Object cache global instance.
*
* @param string|array $group name(s) of group to remove from cache.
*
* @return bool|Array True or array of bool in array passed on success, false on failure group not found.
pbearne marked this conversation as resolved.
Show resolved Hide resolved
*/
function wp_cache_flush_group( $group ) {
global $wp_object_cache;
pbearne marked this conversation as resolved.
Show resolved Hide resolved

if ( $wp_object_cache && method_exists( $wp_object_cache, 'flush_group' ) ) {
return $wp_object_cache->flush_group( $group );
}

_doing_it_wrong( __FUNCTION__, __( 'Your implementation of object caching does not support flush by group.' ), '6.0.0' );

return wp_cache_flush();
Copy link
Member

@dd32 dd32 Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a dangerous fallback for this method, having plugin code expecting to flush a specific cache group(s) and then flushing the entire cache is not going to end well.

Additionally, IMHO it's not a _doing_it_wrong() E_USER_DEPRECATED, possibly a E_USER_NOTICE level warning instead though?
(I say this, because with many object caches, it's not possible to flush a specific group, as key-value stores often don't have a "flush keys with this prefix" - Memcache object cache dropin gets around this by prefixing the timestamp to keys, so that it can just switch to a new set of prefixes when flushed)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The worry here, is that if you call this function can expect and clear your group and it doesn't we need to warn developer this is happening. Maybe we should return a WP_Error object?

So you could do this in your own code.

$result = wp_cache_flush_group( 'posts' );
if( is_wp_error( $result ) ) {
    wp_cache_flush();
}

But give devs more control here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the WP_Error approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think return false would suffice myself if it wasn't going to flush everything.. "Clear this cache group, sorry, that failed.".

But as mentioned in #2368 (comment) variants whose cache server that don't support clearing by prefix, could rotate a prefix to flush the group.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that means that false, could mean a lot of things. Unsupported and the cache group flush failed. That is unclear for developers that using this function, why the failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some feedback from @felixarntz:

What I would suggest is considering making support of this more explicitly optional. For example

  • Not put the flush_group() method in the base class, or alternatively put it in the base class as a no-op that just returns false.
  • Introduce wp_cache_flush_group(), but explicitly document it that it may not be supported and therefore returns a boolean to indicate success, which is critical to check here (other than maybe on other cache functions).
  • In the function, depending on my first point, either check if the method exists on the cache implementation and if not return false, or otherwise just call the method and return through its boolean result.

Alternatively, we could make it even more explicit to clarify that developers need to be aware that this may not work, e.g. by having the boolean function just as a check that has to be explicitly called (e.g. wp_cache_can_flush_group()) before calling wp_cache_flush_group(). The latter could cause a _doing_it_wrong() if not supported or throw an exception.

So TLDR whichever way we go, I definitely think it should be explicitly built and marked as optional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixarntz's comments here reflect statement I have made throughout this code review. These needs to be a way for developers to know if running this function is going to work. They have to know if the object cache implement supports it.

I recommend adding a const, like WP_CACHE_SUPPORTS_GROUP_FLUSH, so developers of object cache drop-ins can "opt-in" to support this feature. If this const is not defined, then, developers know not to use this function.

See these simplier examples.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest is using WP_OBJECTCACHE_* since WP_CACHE refers to WP's "page caching" mechanism.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a half thought in a Slack thread that I'll leave in full detail here so it doesn't get lost.

For a lot of groups currently, WP uses the last_changed cache key to determine when the group was last changed. It acts as a faux group flushing mechanism.

wp_cache_set( 'last_changed', microtime(), 'posts' );

For drop-ins that support the flush_group() method, WP could call that and trust the drop in developer to have checked their persistent cache supports it.

For drop-ins that do not include the method, WP could include something similar to polyfill the feature.

// Within caches
if ( ! method_exists( ... )  && 'wp_group_suffiixes' !== $group ) {
   $group = $group . '::' . wp_cahce_group_suffix( $group );
}

// Function
function wp_cahce_group_suffix( $group, $update = false ) {
  $suffix = wp_cache_get( $group, 'wp_group_suffiixes' );
  if ( ! $update ) {
    return $suffix;
  }
  
  $new_suffix = $suffix;
  while ( $new_suffix === $suffix ) {
     $new_suffix = wp_generate_password( 8, true, true );
  }

  return $new_suffix;
}

At this stage, I'd call this a thought bubble and it absolutely needs further thinking through. One of the issues Dion mentioned in Slack is that there is potential to hit key length issues in some persistent caches.

It's also possible this is a terrible idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the discussion for this ticket should move back to the original tickets. GH is a code review tool and not best designed for this.

pbearne marked this conversation as resolved.
Show resolved Hide resolved
}
endif;
28 changes: 28 additions & 0 deletions src/wp-includes/cache.php
Expand Up @@ -132,6 +132,34 @@ function wp_cache_set_multiple( array $data, $group = '', $expire = 0 ) {
return $wp_object_cache->set_multiple( $data, $group, $expire );
}

/**
* Removes all cache items in a group.
*
* @param string|array $group name(s) of group to remove from cache.
*
* @return bool|Array True or array of bool in array passed on success, false on failure group not found.
* @global WP_Object_Cache $wp_object_cache Object cache global instance.
*
* @since 6.0.0
*
* @see WP_Object_Cache::flush_group()
pbearne marked this conversation as resolved.
Show resolved Hide resolved
*/
function wp_cache_flush_group( $group ) {
pbearne marked this conversation as resolved.
Show resolved Hide resolved
global $wp_object_cache;

// if group is an array loop and call each key in the array
if ( is_array( $group ) ) {
return array_map( 'wp_cache_flush_group', array_values( $group ) );
}
if ( $wp_object_cache && method_exists( $wp_object_cache, 'flush_group' ) ) {

return $wp_object_cache->flush_group( $group );
} else {

return $wp_object_cache->flush();
}
pbearne marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Retrieves the cache contents from the cache by key and group.
*
Expand Down
12 changes: 12 additions & 0 deletions src/wp-includes/class-wp-object-cache.php
Expand Up @@ -290,6 +290,18 @@ public function set_multiple( array $data, $group = '', $expire = 0 ) {
return $values;
}

/**
* Clears the object cache of all data for the given group.
*
* @since 6.0.0
*
* @return true Always returns true.
pbearne marked this conversation as resolved.
Show resolved Hide resolved
*/
public function flush_group( $group ) {
unset( $this->cache[ $group ] );

return true;
}
/**
* Retrieves the cache contents, if it exists.
*
Expand Down
49 changes: 49 additions & 0 deletions tests/phpunit/tests/cache.php
Expand Up @@ -415,4 +415,53 @@ public function test_wp_cache_delete_multiple() {

$this->assertSame( $expected, $found );
}

/**
* @ticket 4476
* @ticket 9773
*
* test wp_cache_flush_group
*
* @covers ::wp_cache_flush_group
*/
public function test_wp_cache_flush_group() {
$key = 'my-key';
pbearne marked this conversation as resolved.
Show resolved Hide resolved
$val = 'my-val';

wp_cache_set( $key, $val, 'group-test' );
wp_cache_set( $key, $val, 'group-kept' );
$this->assertSame( $val, wp_cache_get( $key, 'group-test' ), 'test_wp_cache_flush_group: group-test should contain my-val' );

$results = wp_cache_flush_group( 'group-test' );
$this->assertTrue( $results );
$this->assertFalse( wp_cache_get( $key, 'group-test' ), 'test_wp_cache_flush_group: group-test should return false' );
$this->assertSame( $val, wp_cache_get( $key, 'group-kept' ), 'test_wp_cache_flush_group: group-kept should still contain my-val' );
Comment on lines +436 to +438
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->assertTrue( $results );
$this->assertFalse( wp_cache_get( $key, 'group-test' ), 'test_wp_cache_flush_group: group-test should return false' );
$this->assertSame( $val, wp_cache_get( $key, 'group-kept' ), 'test_wp_cache_flush_group: group-kept should still contain my-val' );
if (WP_OBJECT_CACHE_SUPPORTS_GROUP_FLUSH) {
$this->assertTrue( $results );
$this->assertFalse( wp_cache_get( $key, 'group-test' ), 'test_wp_cache_flush_group: group-test should return false' );
$this->assertSame( $val, wp_cache_get( $key, 'group-kept' ), 'test_wp_cache_flush_group: group-kept should still contain my-val' );
} else {
$this->assertWPError( $results );
$this->assertSame( 'unsupported', $results->get_error_code() );
}

}

/**
* @ticket 4476
* @ticket 9773
*
* test wp_cache_flush_group with an array of groups
*
* @covers ::wp_cache_flush_groups
*/
public function test_wp_cache_flush_groups() {
$key = 'my-key';
pbearne marked this conversation as resolved.
Show resolved Hide resolved
$val = 'my-val';

wp_cache_set( $key, $val, 'group-test' );
wp_cache_set( $key, $val, 'group-test2' );
wp_cache_set( $key, $val, 'group-kept' );
$this->assertSame( $val, wp_cache_get( $key, 'group-test' ), 'test_wp_cache_flush_groups: group-test should contain my-val' );
$this->assertSame( $val, wp_cache_get( $key, 'group-test2' ), 'test_wp_cache_flush_groups: group-test2 should contain my-val' );

$results = wp_cache_flush_group( array( 'group-test', 'group-test2' ) );

$this->assertIsArray( $results );
$this->assertCount( 2, $results );
$this->assertFalse( wp_cache_get( $key, 'group-test' ), 'test_wp_cache_flush_groups: group-test should return false' );
$this->assertFalse( wp_cache_get( $key, 'group-test2' ), 'test_wp_cache_flush_groups: group-test2 should return false' );
$this->assertSame( $val, wp_cache_get( $key, 'group-kept' ), 'test_wp_cache_flush_groups: group-kept should still contain my-val' );
pbearne marked this conversation as resolved.
Show resolved Hide resolved
}
}
1 change: 1 addition & 0 deletions tests/phpunit/tests/pluggable.php
Expand Up @@ -332,6 +332,7 @@ public function get_pluggable_function_signatures() {
'wp_cache_add_non_persistent_groups' => array( 'groups' ),
'wp_cache_switch_to_blog' => array( 'blog_id' ),
'wp_cache_reset' => array(),
'wp_cache_flush_group' => array( 'group' ),
)
);
}
Expand Down