markjaquith / WP-TLC-Transients

GNU General Public License v2.0
341 stars 37 forks source link

If failure in callback then transient content is wiped #7

Closed l3rady closed 11 years ago

l3rady commented 12 years ago

Lets say my callback is:

    public static function my_callback( $query ) {

        $response = wp_remote_retrieve_body( wp_remote_get( $query, array( 'timeout' => 30 ) ) );

        if( is_wp_error( $response ) )
            return false;

        return $response;
    }

If the call to the remote website fails to get new content then I'm going to want to retain the existing content not blank it out. Is it possible to setup the callback that if it returns false then keep the existing content and retry after the cache limit?

Currently only way around this is to read the transient inside the callback on failure and return the transient content.

mintindeed commented 12 years ago

This can be done in a simplistic fashion pretty simply.

In TLC_Transient::fetch_and_cache() Change:

try {
    $data = call_user_func_array( $this->callback, $this->params );
    $this->set( $data );
} catch( Exception $e ) {

To:

try {
    $data = call_user_func_array( $this->callback, $this->params );
    if ( $data ) {
        $this->set( $data );
    }
} catch( Exception $e ) {

The problem is, you will keep trying the same operation repeatedly, putting additional load on your server and probably banging your head against the wall of theirs. For example, if you've reached Twitter's API rate limit, then continually trying every couple seconds isn't going to help you.

I think the callback being responsible for returning data from the transient if the call fails is really the best approach here.

l3rady commented 12 years ago

@mintindeed Yes I see what you are saying:

In the event of no data from the remote server every page load will ask the server remote server for content, which you don't want.

What I was suggesting is that if no data was returned (Boolean False) then the data is kept in the transient but the transient timeout is reset to the cache length.

So for example I have a cache of 30 seconds. On the second refresh the server returns false (lets say we hit the rate limit of twitter). False is returned and the cache is kept and the transient is set to retry in 30 seconds.

So the check for false, I feel needs to happen in TLC_Transient::set() not in TLC_Transient::fetch_and_cache()

markjaquith commented 11 years ago

My approach was to add an ->extend_on_fail( integer ) method. I thought that you might not want to extend it for another whole TTL cycle. You might just want a mini-extension (say, to wait until the external web service sorts out its issues).

Pretty basic, but should handle most HTTP-fetch failures. This acts as a nice throttle, and solves the issue @mintindeed mentioned about continuously retrying.

Thoughts?

mintindeed commented 11 years ago

I dig it.