Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#20004 closed defect (bug) (fixed)

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

Reported by: andy Owned by: ryan
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: Cache API Keywords: has-patch dev-feedback
Focuses: Cc:

Description (last modified by 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.

Attachments (3)

20004.1.diff (5.4 KB) - added by andy 4 years ago.
20004.2.diff (4.5 KB) - added by andy 4 years ago.
Now without $this->last_result and wp_cache_found()
20004.notices.diff (506 bytes) - added by duck_ 4 years ago.

Download all attachments as: .zip

Change History (15)

@andy4 years ago

comment:1 @scribu4 years ago

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.

comment:2 @andy4 years ago

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.

Last edited 4 years ago by andy (previous) (diff)

comment:3 @nacin4 years ago

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.

comment:4 @andy4 years ago

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.

comment:5 @andy4 years ago

  • Description modified (diff)

comment:6 @nacin4 years ago


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.

comment:7 @scribu4 years ago

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

comment:8 @ryan4 years ago

  • Milestone changed from Awaiting Review to 3.4

@andy4 years ago

Now without $this->last_result and wp_cache_found()

comment:9 @ryan4 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [20089]:

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

@duck_4 years ago

comment:10 @duck_4 years ago

In [20091]:

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

comment:11 @nacin3 years ago

Follow-up: #21320

comment:12 @nacin3 years ago

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.

Note: See TracTickets for help on using tickets.