Flag Not setting a value in cache when a failure happens

[david-binda]

As in people only cache on success.

[GaryJones]

Please provide some more context here, such as example code that should be reported, and example code that shouldn't.

[david-binda]

Yeah, this one is a bit tricky to implement.

function my_theme_get_posts() {
    $posts = wp_cache_get( 'mykey', 'mygroup' )
    if ( false !== $posts ) {
        return $posts;
    }
    $expensive_query = new WP_Query( $args );
    $posts = $expensive_query->posts;
    if ( $expensive_query->posts ) {
        wp_cache_set( 'mykey', $expensive_query->posts, 'mygroup', 300 );
    }
    return $posts;
}

The problem of the above code is that in case the $expensive_query does not find any posts, no wp_cache_set is being called, thus the expensive query is happening on every pageload.

Ideally, there should be some else which caches, eg.: an empty array, possibly with some lower expiration. Both of the following examples are correct:

function my_theme_get_posts() {
    $posts = wp_cache_get( 'mykey', 'mygroup' )
    if ( false !== $posts ) {
        return $posts;
    }
    $expensive_query = new WP_Query( $args );
    $posts = $expensive_query->posts;
    wp_cache_set( 'mykey', $expensive_query->posts, 'mygroup', 300 );
    return $posts;
}

function my_theme_get_posts() {
    $posts = wp_cache_get( 'mykey', 'mygroup' )
    if ( false !== $posts ) {
        return $posts;
    }
    $expensive_query = new WP_Query( $args );
    $posts = $expensive_query->posts;
    if ( $expensive_query->posts ) {
        wp_cache_set( 'mykey', $expensive_query->posts, 'mygroup', 300 );
    } else {
        wp_cache_set( 'mykey', $expensive_query->posts, 'mygroup', 60 );
    }
    return $posts;
}

As I said, it might not be easy to catch those, but perhaps flagging any odd (vs. even in case else part is being used) conditional wp_cache_set call as a warning for manual investigation might be a good start?

btw: it's not just an expensive query, but also failing remote requests on other expensive calls which might not get cached in case they fail.

[GaryJones]

This would be a good coding practice for everyone though - perhaps best for this to go in WPCS (WordPress-Extra), even if VIPCS changes the severity / violation type?