-
Notifications
You must be signed in to change notification settings - Fork 54
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
GH-63: Implement wp_cache_get_multiple() #88
Conversation
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
============================================
+ Coverage 77.75% 79.38% +1.62%
- Complexity 145 158 +13
============================================
Files 1 1
Lines 454 485 +31
============================================
+ Hits 353 385 +32
+ Misses 101 100 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
$this->group_ops_stats( 'get_multiple', $uncached_keys_list, $group, null, $elapsed ); | ||
|
||
foreach ( $uncached_keys as $id => $key ) { | ||
$found = array_key_exists( $key, $values ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memcache::get()
will return the stored value which may be null
. isset()
returns false
for null values.
$return[ $id ] = $value; | ||
$return_cache[ $key ] = [ | ||
'value' => $value, | ||
'found' => $found, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false !== $value
will produce an incorrect result if we have stored false
for the given key.
$key = $this->key( $id, $group ); | ||
|
||
if ( isset( $this->cache[ $key ] ) && ( ! $force || $no_mc ) ) { | ||
$value = $this->cache[ $key ]['found'] ? $this->cache[ $key ]['value'] : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a nice feature: when set()
fails, we may end up with some value and found
set to false
.
I chose to return false
because the value is not in Memcached. Returning something else may break add/increment scenarios.
I just tagged some folks to whom this may be relevant. |
Wanted to mention that last time I played around with the "send an array of keys to get()" functionality, it turned out it handled some edge cases differently - particularly So in |
My tests show that we have issues with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments of not too much substance. The only part that makes me apprehensive is the $flags
handling, but maybe I'm just not fully understanding the implications.
Another thing is is tests we only use small values which is not indicative of what users actually do. Maybe it can be a subject of a separate PR though.
6f2488d
to
0e8645c
Compare
Co-authored-by: Rinat K <rinat@automattic.com>
Kudos, SonarCloud Quality Gate passed!
|
This PR adds support for the
wp_cache_get_multiple()
function introduced in WordPress 5.5.Fixes #63