Automattic / wp-memcached

Memcached Object Cache for WordPress.
https://wordpress.org/plugins/memcached/
GNU General Public License v2.0
160 stars 55 forks source link

$found parameter to wp_cache_get() is incorrect when object is in local cache in a non-persistent group #61

Open dd32 opened 4 years ago

dd32 commented 4 years ago

On the back of #40 & #43 that changed the behaviour of the $found parameter to reflect the status of the data in memcache, however, it breaks a different-but-similar case: non-persistent groups.

For example:

wp> wp_cache_add_non_persistent_groups( [ 'example' ] );
NULL

wp> wp_cache_set( 'example', 'example', 'example' );
bool(true)

wp> wp_cache_get( 'example', 'example', false, $found );
string(7) "example"

wp> $found
bool(false)

In this case, the value was never going to exist within memcache and it was found in the cache, so I would argue that $found should be true in this case.

I personally ran into this with Cavalcade, where it relies upon the $found parameter working with non-persistent groups: https://github.com/humanmade/Cavalcade/blob/master/inc/class-job.php#L366-L368

jenkoian commented 4 years ago

Added failing test for this here if it helps: https://github.com/Automattic/wp-memcached/pull/62

jenkoian commented 4 years ago

It's an interesting one, because the value is set in memory cache but non in memcache. So as mad as it sounds there is some logic in set() returning true and $found being false?

dd32 commented 4 years ago

So as mad as it sounds there is some logic in set() returning true and $found being false?

Correct, that was a side effect of a deliberate choice of #43 - $found is set to false in the cache, because it's not yet in Memcache, and the $found flag would be toggled once it was in memcache. But when it's a non-persistent group, that fails as it never ends up in memcache so $found stays false.

I think https://github.com/Automattic/wp-memcached/blob/master/object-cache.php#L512 just needs $this->cache[ $key ]['found'] = true; added.. if this suggested behaviour seems correct.

jenkoian commented 4 years ago

@dd32 updated https://github.com/Automattic/wp-memcached/pull/62 so it now passes.