get_transient doesn't check the return of get_option

[jamesgol]

When calling get_transient on a key that doesn't exist, notoptions[$key] is set so it won't do any further queries, but it doesn't go far enough. Since the get_option( $transient_timeout ) doesn't check the return well enough it will always cause 2 more queries. (SELECT autoload FROM wp_options ...)

If get_options( $transient_timeout ) is returning false, what is the point of trying to delete it.

[voldemortensen]

This patch worked for me. I created a faux plugin that called get_transient( 'randomstuff' ) and used the query monitor before and after the patch. The two delete_option calls were indeed eliminated. Saved about 0.003 seconds.

[iseulde]

has-patch 4.2-early so moving to 4.2.

[ocean90]

The patch makes sense at first glance, but it's worth mentioning that false < time() is true, means $transient_option will be deleted from the database. Call it a clean up for broken transients. Maybe we should only avoid the delete_option( $transient_timeout ); call?

[jamesgol]

I've never seen a case where a non autoloaded option did not have a timeout. If there is an issue with that, perhaps the source of the problem should be fixed.

[afercia]

Replying to ocean90:

The patch makes sense at first glance, but it's worth mentioning that false < time() is true, means $transient_option will be deleted from the database.

This caused me some headaches while trying to understand why the featured posts in Twenty Fourteen were firing lots of queries. They've been someway improved since then see #26744. With the old implementation, my scenario was:

No posts match the featured tag, fallback to sticky posts
the transient, though intended to be autoloaded, didn't exist yet after a settings change
get_transient() will assume: "If option is not in alloptions, it is not autoloaded and thus has a timeout"
it checks if transient is in $alloptions
it is not so it will be considered a transient with expiration
get_option() will cache its non-existence in the 'notoptions' cache
get_option( $transient_timeout ) will return false
so it will check if ( false < time() ) :) which evaluates true
WordPress now thinks it has to deal with an expired transient and will try to delete it
finally, will set $value = false and return false

This caused also a second round of queries due to the old get_featured_post_ids() implementation. Have to say I've checked this some months ago, not sure about the current behavior but I fear there's some failing assumption here that doesn't cover edge cases or bad implementations.

[jamesgol]

@ocean90 If broken transients are really something to worry about then I would say something called occasionally from cron to clean them up would be better than something that can be called many times for every request.

[DrewAPicture]

Patch still applies. @ocean90: Can you please recommend to pursue or punt for 4.2?

[DrewAPicture]

Thinking it's too late in the cycle to mess with transients. Punting.

@ocean90: Feel free to pull this one back if you think it's OK.

[MikeHansenMe]

This patch still applies r32676

[johnbillion]

Can we get some tests on here?

[ericmann]

Updated patch file, with tests

[ericmann]

I've refreshed the patch file, and also added some tests to verify that transient options are deleted (or not deleted) under the conditions explained in the ticket.

[obenland]

@ocean90, do you want to look the last patch over and see if we can get that in?

[ocean90]

patch-transient-wtests.diff​ uses closures for the tests which won't work in in PHP 5.2.
@ericmann would you like to update the tests to make them compatible with 5.2?

[ericmann]

Replying to ocean90:

would you like to update the tests to make them compatible with 5.2?

Core's test framework already has closures and is thus incompatible with 5.2. Is that a hard requirement here? I think it will add unnecessary complexity to these tests and, since we aren't running tests "in production," shouldn't be tied to WP's minimum version requirements.

[ocean90]

Replying to ericmann:

Core's test framework already has closures and is thus incompatible with 5.2.

That's odd, since we're running tests against 5.2 too and they aren't failing, see ​https://travis-ci.org/aaronjorbin/develop.wordpress/builds/69821650.

[dd32]

FWIW there are 4 phpunit files which are marked as PHP 5.3+ only, one which is a Closure-specific test, and the others are the Image Editors (which I can't see why are 5.3+ at first glance)

https://core.trac.wordpress.org/browser/trunk/phpunit.xml.dist#L14

If no 5.3-specific things are required, and removing it isn't difficult, tests should aim for the same support as core IMHO.

[ericmann]

Refactor the tests to remove closures (to make things PHP 5.2-compatible)

[ericmann]

Replying to dd32:

If no 5.3-specific things are required, and removing it isn't difficult, tests should aim for the same support as core IMHO.

I don't necessarily agree with you here, but that's an argument for a different day and venue. Updated patch attached.

[ocean90]

patch-transient-wtests-5.2.diff​ looks good. Only add_action( 'delete_option', array( $a, 'delete_option' ) ); should be add_action( 'delete_option', array( $a, 'action' ) ); since a delete_option method doesn't exist.

[ocean90]

In 33110:

Transients: If get_option( $transient_timeout ) returns false, don't bother trying to delete the transient in get_transient().

props jamesgol, ericmann.
fixes #30380.

[ericmann]

Replying to ocean90:

patch-transient-wtests-5.2.diff​ looks good. Only add_action( 'delete_option', array( $a, 'delete_option' ) ); should be add_action( 'delete_option', array( $a, 'action' ) ); since a delete_option method doesn't exist.

Good catch. Since the action is only added to verify that it's not invoked, the tests pass either way. Thanks for catching it, though! :-)

[johnbillion]

Tests_Option_Transient::test_nonexistent_key_old_timeout() is ​failing on Travis. It's not failing for me locally though, when run in isolation or when running the whole suite.

[johnbillion]

In 33125:

Skip the Tests_Option_Transient::test_nonexistent_key_old_timeout() tests on multisite for now.

See #31130
Fixes #30380

[ocean90]

#23881 was marked as a duplicate.