WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#25213 closed defect (bug) (fixed)

set_site_transient() incorrectly passes prefixed transient names to hooks when not using an object cache

Reported by: dd32 Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.0
Component: General Keywords: has-patch
Focuses: Cc:

Description (last modified by dd32)

When using the hooks within set_site_transient(), developers are forced to cover two different cases, Object caching, and no object caching.

This is because the value of the $transient is overwritten in the no-object cache case, ending up with two different action names fired:

For those not using an object cache: set_site_transient__site_transient_update_plugins
For those using an object cache: set_site_transient_update_plugins.

The setted_site_transient hook also receives a prefixed value in the event of no object caching, ie, update_plugins for an object cache, _site_transient_update_plugins for no object cache.

This appears to stretch back to [13148]

Attached is a patch for review, with some back-compat actions commented out for now

Attachments (1)

25213.diff (1.4 KB) - added by dd32 7 years ago.

Download all attachments as: .zip

Change History (10)

@dd32
7 years ago

#1 @dd32
7 years ago

  • Description modified (diff)

#2 @dd32
7 years ago

Current thoughts on the back-compat:

  1. We should handle back-compat as we always do
  2. Firing both versions of the action may confuse plugins only expecting one version of the action to fire.
  3. Firing only the "correct" hook would be best, BUT, the correct hook was being fired in the using-object-cache mode only, leading most developers to have been using the bad filter (If anyone is actually using it)

So really, we have back compat issues either way, Either we fire only the proper action, or we fire both.
Firing both seems like it'll be the lesser of 2 evils in this case, most existing code should still work, but may cause duplicate processing for some plugin code until fixed.

#3 @nacin
7 years ago

I think we can also keep in mind this is the set_site_transient() function, not set_transient(). As it is, when we moved certain transients from set_transient() to set_site_transient(), we changed the hooks being used for numerous transients. I pointed this out somewhere (no idea if it was Trac, IRC, or a mailing list) but we decided to not take any action. I never saw a single complaint.

And as for this bug: No one has reported this previously. It's an obscure hook in an obscure level (site_transient, versus transient) of the transients API. If they wanted to use it, they immediately would have realized something was very, very wrong. And the "correct" way to use it would have been to also account for an external object cache.

I have a feeling that firing only the correct hook here would be OK. My instinct is to pull the band-aid off, do a make/core post on it (3.7-compat tag), and move on.

#4 @dd32
7 years ago

I have a feeling that firing only the correct hook here would be OK. My instinct is to pull the band-aid off, do a make/core post on it (3.7-compat tag), and move on.

I think we're in agreement here really, my previous comment was being way too cautious on compatibility issues, and i'd also say these actions are not commonly used, otherwise we'd have heard about it already.

#5 @dd32
7 years ago

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

In 25350:

Fix the action that set_site_transient() fires so as not to include the private option prefix. This brings set_site_transient() back in line with it's documented behaviour, and the behaviour of all other transient functions. Fixes #25213

#6 follow-up: @johnbillion
7 years ago

Worth a grep of the plugin repo for set_site_transient_? With the trailing underscore there it should just flag up any plugins that are using that hook.

#7 in reply to: ↑ 6 @nacin
7 years ago

Replying to johnbillion:

Worth a grep of the plugin repo for set_site_transient_? With the trailing underscore there it should just flag up any plugins that are using that hook.

Sure, done. Nearly every result found for set_site_transient_ is for the pre_set_site_transient_ filter (almost 70 instances). Two exceptions:

  • pento's automatic-updater plugin, which attached a callback to both versions of the action.
  • dd32's wordpress-background-update plugin, which didn't account for the "proper" action. DUDE.

#8 follow-up: @dd32
7 years ago

dd32's wordpress-background-update plugin, which didn't account for the "proper" action. DUDE.

I had forgotten that plugin ever existed..
But I knew I'd seen this stupidity before! - I guess I didn't realise that the action was so messed up (or firing 2 different actions) and just copy-pasted it.

#9 in reply to: ↑ 8 @nacin
7 years ago

Replying to dd32:

dd32's wordpress-background-update plugin, which didn't account for the "proper" action. DUDE.

I had forgotten that plugin ever existed..
But I knew I'd seen this stupidity before! - I guess I didn't realise that the action was so messed up (or firing 2 different actions) and just copy-pasted it.

Yeah, yeah — caught red-handed. :-)

Note: See TracTickets for help on using tickets.