Fix NULL and FALSE in WP_Object_Cache and make found/not-found unambiguous.

[andy]

WP_Object_Cache mistreats NULL as a stored value by converting it to "". To handle it correctly, the existence check must not use isset():

$cache[ $key ] = NULL;
isset( $cache[ $key ] ); // false
array_key_exists( $key, $cache ); // true

The attached patch corrects the treatment of NULL by storing it as-is and using array_key_exists() to test the existence of a key.

As a value returned from wp_cache_get, FALSE is ambiguous. It could mean that the value stored and retrieved was literally FALSE or that the key was not found.

The attached patch moves the found/not-found information to a pass-by-reference parameter, $found, which is set to TRUE after a cache hit or FALSE after a cache miss. This allows storage and retrieval of FALSE without ambiguity and reduces the temptation to use confusing patterns such as !$value, $value === false, and empty( $value ).

The attached patch permits a clearer coding style:

$value = wp_cache_get( $key, $group, false, $found );
if ( ! $found ) {
    $value = $wpdb->get_results( $query );
    wp_cache_set( $key, $value, $group );
}

However, because object cache drop-ins may lag behind core in adding support for the $found parameter, the following pattern may be preferred for backwards compatibility:

$value = wp_cache_get( $key, $group, false, $found );
if ( ! $value && ! $found ) {
    $value = $wpdb->get_results( $query );
    wp_cache_set( $key, $value, $group );
}

I also added a function, wp_cache_found(), to determine whether the previous retrieval was a hit or a miss.

$value = wp_cache_get( $key, $group );
if ( ! wp_cache_found() ) {
    $value = $wpdb->get_results( $query );
    wp_cache_set( $key, $value, $group );
}

Again, this function may not be present in drop-ins. Until such time as old drop-ins are forced into obsolescence, use function_exists before calling wp_cache_found in core code.

It would behoove us to devise a drop-in API versioning and update system to assist with this.

The additional pass-by-ref variable makes sense when you want to be able to store both FALSE and NULL as explicit values, but it's more verbose.

I like the wp_cache_found() function though and I don't see why both wp_cache_found() and $found would be needed.

I included both the function and the pass-by-ref param because I could not choose a clear winner. Each one wins in different code contexts.

The backwards-compatible use case with the function call is even more verbose than with the pass-by-ref:

$value = wp_cache_get( $key, $group );
if ( ! $value && ( function_exists( 'wp_cache_found' ) && ! wp_cache_found() ) ) {
    $value = $wpdb->get_results( $query );
    wp_cache_set( $key, $value, $group );
}

If you are counting $found = null; you should consider that you only have to define $found this way once. You can reuse it in subsequent calls to wp_cache_get() in the same lexical context.

What about wp_cache_exists( $key, $group )? No last-resultness or by-ref variable necessary. It would function similar to metadata_exists() and simply return whether the key exists in the group, rather than its actual value.

Also, as long as the reference is declared with the function, and is not call-time pass-by-reference, PHP will not issue a notice here. No $found = null necessary.

You can test this as follows:

error_reporting( -1 );
function test( &$found ) {}
test( $found );
var_dump( $found ); // NULL, no notice.

You will, however, end up with a notice the moment you try to ascertain $found before it is passed into the function:

error_reporting( -1 );
function test( &$found ) {}
var_dump( $found ); // E_NOTICE
test( $found );
var_dump( $found ); // NULL, no notice.

Nacin, thanks for the bit about the Notice. I had triggered a notice when the code was written differently. You are correct. I will remove the extra lines from the ticket.

As to wp_cache_exists(), I am extremely reluctant to encourage anyone to check existence separately from fetching the key. Atomicity is important in high-concurrency environments like WordPress.com. If you were to check exists, then do a get, what would be the expected result? With found (internally called last_result) there is less room for ambiguity, race conditions, and errors.

Atomicity

All that needed to be said. You're absolutely right.

To me, the by-ref sounds like a better solution. It will also allow someone to do $found === true, $found === false, $found === null to determine whether the key was found, it wasn't found, or it hasn't been implemented for the object cache yet, the last one additionally causing a notice. It's elegant, if you ask me.

Given the back-compat considerations, I agree we should go with $found.

Now without $this->last_result and wp_cache_found()

In [20089]:

Fix NULL and FALSE in WP_Object_Cache and make found/not-found unambiguous. Props andy. fixes #20004

In [20091]:

Prevent notices by checking if the $group key isset(). See #20004.

Follow-up: #21320

In [21285]:

Improve the performance of WP_Object_Cache's _exists() method.

Results showed a performance improvement on one admin screen of 90ms (~2%).

fixes #21320. see #20004.