Opened 21 months ago
Last modified 20 months ago
#57743 new enhancement
Add `$found` parameter to `wp_cache_get_multiple()`
Reported by: | peterwilsoncc | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Cache API | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
wp_cache_get()
includes the forth parameter $found
. It's a boolean passed by reference to allow developers to determine whether the returned value false
indicates a populated cache with the value false
.
<?php wp_cache_set( 'ticket-57741', false ); $found = false; $result = wp_cache_get( 'ticket-57741', '', false, $found ); var_dump( $found, $result ); // true, false $found = false; $result = wp_cache_get( 'invalid-key', '', false, $found ); var_dump( $found, $result ); // false, false
wp_cache_get_multiple()
does not include a similar parameter and could benefit from one. Instead of been a boolean, it would be an array passed by reference.
At the conclusion of the function running, the $found
array would be of the form:
array( 'set-key' => true, 'unset-key' => false, )
Attachments (2)
Change History (20)
#1
@
21 months ago
- Keywords needs-patch needs-unit-tests added
- Milestone changed from Awaiting Review to Future Release
- Type changed from defect (bug) to enhancement
#3
@
20 months ago
Thank you for creating the patch @thakkarhardik
On the line following the existing @since
tag, a note will need to be added to indicate the version in which the parameter was added. As this is currently marked as for a future release, you can hold off on that for now.
It would be good to add some unit tests for the new parameter, do you have any experience with writing tests in WordPress core?
When adding tests, it's easiest if you create a pull request on the WordPress-Develop GitHub repository as the tests will run on your pull request.
If you don't have experience writing tests, you can create a pull request with the patch as is and another contributor will be able to help out with some suggestions.
#4
@
20 months ago
Extended PHPUnit tests for wp_cache_get_multiple: added 4th parameter to track found keys.
Test fails without 57743.patch and succeeds with it applied.
This ticket was mentioned in PR #4258 on WordPress/wordpress-develop by @peterwilsoncc.
20 months ago
#5
- Keywords has-unit-tests added; needs-unit-tests removed
Combination of patches on trac ticket WP#57743
@spacedmonkey commented on PR #4258:
20 months ago
#6
@peterwilsoncc Do memcache and redis support this?
@peterwilsoncc commented on PR #4258:
20 months ago
#7
@peterwilsoncc Do memcache and redis support this?
Memcache does, I've updated the implementation in the test suite to do so.
@tillkruess do you know if redis supports it?
@peterwilsoncc commented on PR #4258:
20 months ago
#8
Sorry, wrong username, @tillkruss do you know if redis supports passing found by reference when getting multiple objects?
@tillkruess commented on PR #4258:
20 months ago
#9
Yes, it could be added to the plugins. But Litespeed, W3TC and others are pretty stale and won't support this for a long time.
@tillkruess commented on PR #4258:
20 months ago
#10
Does this serve a purpose in core?
How could this be used in the real world, isn't the false value already giving us this information?
@peterwilsoncc commented on PR #4258:
20 months ago
#11
Does this serve a purpose in core?
Mainly, it makes the getter API for single and multiple cache objects consistent.
How could this be used in the real world, isn't the false value already giving us this information?
Often but not always, the real world implication is when the cached object is false
. In the example below, the slow function will be run for foo
even when the result is cached.
Core caches false values in some places but not many. In some circumstances, it's quite common that a false needs to be cached.
{{{php
function pw_slow_function( $result ) {
sleep( 2 );
return $result;
}
wp_cache_set( 'foo', pw_slow_function( false ) );
wp_cache_set( 'bar', pw_slow_function( true ) );
$values = wp_cache_get_multiple( [ 'foo', bar' ] );
foreach ( $values as $key => $values ) {
if ( ! $value ) {
$value = pw_slow_function( /* as above */ );
wp_cache_set( $key, $value );
}
}
}}}
@tillkruess commented on PR #4258:
20 months ago
#12
The consistency makes sense.
Just for public record, storing falsy values is always risky and often leads to silent errors.
@tillkruess commented on PR #4258:
20 months ago
#13
I don't think Memcached and Redis can reliably support this, because if a user stores false
or null
as a cache value, we don't know whether the result is false
or if we didn't get a cache value.
Checking for existence first doubles to lookup. It's like running file_exists()
and file_get_contents()
which is possible is two operations per key, not one.
Typically WordPress expects non existing cache keys to return false
, so storing false
values won't allow us to distinguish between these.
I'm not sure this PR makes sense, the root issue is what the user code does.
@peterwilsoncc commented on PR #4258:
20 months ago
#14
I'll investigate further to make sure it's working as expected.
For ::get()
, the WP persistent cache implementation checks the result code and defines $found
based on that:
For get multi, maybe it's possible to use array_key_exists()
but I'll need to add some tests.
Does Redis have an equivalent, I notice the docs reference the return value (nil)
for non-existent keys but I'm unclear if that's a magic string or a return code.
@spacedmonkey commented on PR #4258:
20 months ago
#15
I do think that redis could support this functionality. See this method. It might tricky, but I see how it could be done.
Consider that I am really struggling to get cache plugins even support wp_cache_get_multiple.
https://wordpress.org/support/topic/implement-function-wp_cache_get_multiple/
https://github.com/tlovett1/simple-cache/issues/144
https://github.com/humanmade/wp-redis-predis-client/issues/12
https://github.com/litespeedtech/lscache_wp/issues/259
https://github.com/BoldGrid/w3-total-cache/issues/239
I have more this
@tillkruess commented on PR #4258:
20 months ago
#16
I do think that redis could support this functionality. See this method. It might tricky, but I see how it could be done.
That's just using Redis's MGET
which will return false
if a key doesn't exist in a GET
or MGET
call. Predis is a bit better and returns null
for non-existent keys.
But it's just ambitious to store falsy
values in the cache.
I have more this
😞
@tillkruess commented on PR #4258:
20 months ago
#17
I do think that redis could support this functionality. See this method. It might tricky, but I see how it could be done.
That's just using Redis's MGET
which will return false
if a key doesn't exist in a GET
or MGET
call. Predis is a bit better and returns null
for non-existent keys.
But it's just ambitious to store falsy
values in the cache.
I have more this
😞
@tillkruess commented on PR #4258:
20 months ago
#18
I do think that redis could support this functionality. See this method. It might tricky, but I see how it could be done.
That's just using Redis's MGET
which will return false
if a key doesn't exist in a GET
or MGET
call. Predis is a bit better and returns null
for non-existent keys.
But it's just ambitious to store falsy
values in the cache.
I have more this
😞
@peterwilsoncc I have added a patch which adds
$found
array to thewp_cache_get_multiple()
function.