Make WordPress Core

Opened 12 months ago

Last modified 11 months ago

#57743 new enhancement

Add `$found` parameter to `wp_cache_get_multiple()`

Reported by: peterwilsoncc's profile 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)

57743.patch (2.5 KB) - added by thakkarhardik 12 months ago.
57743-phpunit.patch (1011 bytes) - added by cadic 11 months ago.

Download all attachments as: .zip

Change History (20)

#1 @johnbillion
12 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

#2 @thakkarhardik
12 months ago

  • Keywords has-patch added; needs-patch removed

@peterwilsoncc I have added a patch which adds $found array to the wp_cache_get_multiple() function.

#3 @peterwilsoncc
11 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 @cadic
11 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.


11 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:


11 months ago
#6

@peterwilsoncc Do memcache and redis support this?

@peterwilsoncc commented on PR #4258:


11 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:


11 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:


11 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:


11 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:


11 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:


11 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:


11 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:


11 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:

https://github.com/WordPress/wordpress-develop/blob/464ad2021812d7df7b32ad9f42dbc99ffdb8ff4b/tests/phpunit/includes/object-cache.php#L1468-L1471

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:


11 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:


11 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:


11 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:


11 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

😞

Note: See TracTickets for help on using tickets.