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

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

Open
dd32 opened this issue Jun 18, 2020 · 4 comments

Comments

@dd32
Copy link
Member

dd32 commented Jun 18, 2020

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

dd32 added a commit to dd32/Cavalcade that referenced this issue Jun 18, 2020
… to rely upon the `$found` flag that's not available in all object caches.

This fixes job query caching when running on a persistent cache which doesn't implement the `$found` parameter or doesn't consider in-memory caches as "found" (see Automattic/wp-memcached#61).
jenkoian pushed a commit to jenkoian/wp-memcached that referenced this issue Jun 18, 2020
jenkoian pushed a commit to jenkoian/wp-memcached that referenced this issue Jun 18, 2020
jenkoian pushed a commit to jenkoian/wp-memcached that referenced this issue Jun 18, 2020
jenkoian pushed a commit to jenkoian/wp-memcached that referenced this issue Jun 18, 2020
jenkoian pushed a commit to jenkoian/wp-memcached that referenced this issue Jun 18, 2020
@jenkoian
Copy link
Contributor

Added failing test for this here if it helps: #62

@jenkoian
Copy link
Contributor

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
Copy link
Member Author

dd32 commented Jun 19, 2020

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 pushed a commit to jenkoian/wp-memcached that referenced this issue Jun 19, 2020
jenkoian pushed a commit to jenkoian/wp-memcached that referenced this issue Jun 19, 2020
@jenkoian
Copy link
Contributor

@dd32 updated #62 so it now passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants