Standardize hooks across the transient and options APIs

[dd32]

Currently get_transient has 2 filters:

• $pre = apply_filters( 'pre_transient_' . $transient, false );
• return apply_filters('transient_' . $transient, $value);

I'd like to suggest that set and delete transient have some filters too, update_transient_<safe_name> possibly, and the same for delete..

..Also, I noticed that the pre_transient_<transient> use of false means that the filter cant be used to return (bool)false through the filter, Perhaps that should be reworked to use Null instead perhaps?

And yet another questioning of the code, What is the point of $safe_transient in set_transient? It appears as its supposed to be the name that the transient uses.. but.. not entirely sure.

As it is, It looks to me that the update_option code block may not actually be triggered as expected (as may not $transient == $safe_transient)

[nacin]

Replying to dd32:

I'd like to suggest that set and delete transient have some filters too, update_transient_<safe_name> possibly, and the same for delete..

Patch attached to add set_transient_$transient and delete_transient_$transient filters. Both use the not-safe name like pre_transient_$transient and transient_$transient.

..Also, I noticed that the pre_transient_<transient> use of false means that the filter cant be used to return (bool)false through the filter, Perhaps that should be reworked to use Null instead perhaps?

Agreed, but needs dev feedback. Converting to null now could break plugins that currently use false.

And yet another questioning of the code, What is the point of $safe_transient in set_transient? It appears as its supposed to be the name that the transient uses.. but.. not entirely sure.

I'm not entirely sure either, but most _option and _transient functions require that the name is not already SQL-escaped. (get_option appears to be the only exception.)

[nacin]

Looking at this further, it looks like in get_transient() there are instances of an SQL-escaped option name being passed to delete_option, when only get_option wants SQL-escaped option names. Second patch addresses this.

The handling of the safe transient looks good in set_transient().

[dd32]

My comments on this:

• in my opinion, Transients filters should match the useable filter set of options. Options may have old ilters for back compat still which would be pointless for the transients..
• I think it'd be useful to be able to return false on the filters and have it handled properly. A prime example is that i mean "This value has expired" even if it exists in the cache. At present, Thats not possible.
  • It will break some plugins to change this, I'm not aware of what plugins would be using it however.

Can we get some dev-feedback on this? Is this a 3.0 set? or can we get a fleshed out hook set for 2.9? (even if that just means the addition of filters, given esc_sql() is mentioned in that patch)

[nacin]

Looks like the three transient functions could use a decent overhaul to bring them in line with the Option API.

• Fix apparent issue with escaping in get_transient. (in a patch)
• Add pre_set_transient_* filter handling to bring it in line with the filters available to options. (needs patch). Also copy over other hooks as appropriate.
• Change pre_transient_* check from !==false to !==null (dev feedback as could cause back compat issues)

That an accurate todo roundup?

[nacin]

First pass on potential overhaul of options/transients.

[nacin]

Actual first pass on potential overhaul of options/transients.

[nacin]

This is probably too big for 2.9, and with the merge in 3.0, some of this may become irrelevant (or important). That said, I wanted to get it out there:

Latest patch standardizes actions and filters across all 14 option and transient functions (including their MU counterparts). Phpdoc, whitespace, and code cleanup as well.

• The pre_* filters (some of which I think were added) now require null instead of false to bypass per dd32's suggestion. Presents a back compat issue, however.
• All action option/transient functions (update/set/add/delete) now run the same three actions, one before (i.e. delete_option) and, on success, two after (e.g. delete_option_$option and deleted_option).
• Standardizes references to the word "option" (previously, various functions used "option name", "key" and "setting" in parameters, variables and phpdoc). Likewise, a parameter of "newvalue" would now be "value," etc.
• reference to $wpdb removed from transient functions, which don't use it.
• Numerous actions were added to MU wrapper functions that would need to be added to their MU counterparts.

Hook and logic changes function by function:

Option Changes

get_option() -

• pre_option_$option now uses null instead of false to continue. (Back compat issue)

update_option() -

• If add_option is used, then return result of add_option, not simply true.

add_option() -

• pre_add_option_$option filter added. (Exists in add_site_option already.)
• autoload parameter is now true/false, not 'yes' and 'no', but still accepts 'yes' and 'no'.
• now returns true on success and false on failure, instead of alwauys returning null.

delete_option() -

• delete_option_$option action added.

get_site_option() -

• pre_site_option_$option filter now uses null instead of false. (Back compat issue)

add_site_option() -

• add_site_option, added_site_option actions added. (add_site_option_$option already existed.)

delete_site_option() -

• delete_site_option, deleted_site_option actions added. (delete_site_option_$option already existed.)

update_site_option() -

• Now includes logic (from update_option) that allows it to call add_site_option, instead of allowing update_option to call add_option, if the option doesn't exist. (Also now returns false if the value isn't changed, like update_option.)
• update_site_option, updated_site_option actions added. (update_site_option_$option already existed.)

Transient Changes

delete_transient(), delete_site_transient() -

• delete_(site_)transient, delete_(site_)transient_$transient, and deleted_(site_)transient actions added.

get_transient(), get_site_transient() -

• pre_(site_)transient_$transient filter now uses null instead of false. (Back compat issue)

set_transient(), set_site_transient() -

• pre_set_(site_)transient_$transient and pre_set_(site_)transient_expiration_$transient filters added.
• set_(site_)transient, set_(site_)transient_$transient, and setted_(site_)transient actions added.

[dd32]

The pre_* filters (some of which I think were added) now require null instead of false to bypass per dd32's suggestion. Presents a back compat issue, however.

To be honest: I thought the options already used null, and that it was just transients which didnt..

I think this is going to be a 3.0 thing now.. once i've seen the size of the patch.. Too late to make changes which affect back compat like that.

[nacin]

New patch does two things: 1) Adds additional phpdoc (@uses do_action()) and does some clean up on existing phpdoc. 2) Reverts null check back to a false check.

Since get_option has returned false since the beginning of time if the option doesn't exist or has no value, it doesn't make much sense to change that now and introduce backwards compatibility issues.

Thus, this patch only adds phpdoc and standardizes hooks (by adding lots of 'em).

Also, my escaping bug from earlier was actually invalid. get_option and delete_option (and their _site_ counterparts) want escaped option/transient names, while all other option and transient functions want them unescaped. (Maybe this should be standardized with some sort of maybe_esc_sql function.)

[nacin]

Second pass.

[dd32]

Since get_option has returned false since the beginning of time if the option doesn't exist or has no value, it doesn't make much sense to change that now and introduce backwards compatibility issues.

Right now, Its impossible to return false of the option function set via the pre_option_* filter, thats what i originally meant. Never intended for it to change the way it operated in returning false.

[nacin]

Replying to dd32:

Right now, Its impossible to return false of the option function set via the pre_option_* filter, thats what i originally meant. Never intended for it to change the way it operated in returning false.

Correct. Oh, now I fully grasp the implications. Since false means that the option is empty or doesn't exist, then that's exactly why you'd want to use the pre_* filter to return false.

This seems hackish, but to maintain backwards compatibility, what if we left it as false, but if you returned null to the filter, it'd return the option function as false? (It'd only break if someone was deliberately returning null, and even then, they'd get false -- which might be what they had wanted in the first place but couldn't do.)

$pre = apply_filters( "pre_option_{$option}", false );
	if ( null === $pre )
		return false;
	if ( false !== $pre )
		return $pre;

The only downside, I think, is that it is confusing. (I guess it'll need some phpdoc.)

[nacin]

Scratch that. You can still force a return false using the (option|transient)_$option filters if you really needed to. I'd venture to guess that this aspect is best left untouched.

[nacin]

I think this makes sense for early 3.0, before the merge.

[nacin]

Patch will need refresh after [12656] and multisite merging.

[nacin]

Against [12933]

[nacin]

Giant patch attempts to standardize the hooks in the options and transients APIs (including site options and transients). Adds lots of inline documentation and standardizes whatever it can.

This is against trunk, r12933. Probably will need some additional passes, but there's a ton of green and red in this diff already.

I've gone through it line by line for typos and tested all the functions, though it could use a sanity check.

The only backwards incompatibility is as follows: Actions that are run after an option is added, deleted, or updated should only fire when the db query was successful. (@todo - add_option()). Actions in add_site_option() and delete_site_option() currently run even on fail, but in this patch they would be brought inside a if ( $results ) statement like the rest of them.

[nacin]

Against [12933], fixes a typo in get_site_transient()

[nacin]

(In [13139]) Whitespace, phpdoc, standard variable names, better return values for options and transients APIs. See #10788

[nacin]

(In [13142]) More cleanups of the options/transients APIs. More inline documentation, better return values (always true on success, false on failure). Only call actions that are after wpdb delete/update operations if the operation was successful. See #10788

[nacin]

(In [13148]) Add hooks across options and transients APIs. Focuses on site transients/options functions. Attempts to standardize where possible. See #10788

[nacin]

There's still some quirkiness in add_site_option and update_site_option related to serialized data being passed as an option value to a hook.

In add_site_option(), it's consistently serialized. add_option() has the same issue. Not ideal, but better than update_site_option(), which will receive serialized data if multisite, but unserialized data if not.

[nacin]

(In [13158]) Fix var name conflict in delete_site_option(), see #10788

[nacin]

Consistently pass unserialized values to hooks in update_site_option(). Change add_option() and add_site_option() to do the same. Any plugin using maybe_unserialize() would work fine, as the value wouldn't need to be unserialized any longer.

[nacin]

(In [13433]) Consistently pass unserialized values to hooks in update_site_option(). Change add_option() and add_site_option() to do the same. Any plugin using maybe_unserialize() would continue to work as the value would no longer need to be unserialized. fixes #10788

[nacin]

(In [13439]) Small fix to [13433], see #10788

[Denis-de-Bernardy]

See also #12416

[nacin]

(In [13508]) Don't pass serialized values to hooks in options API. see #10788 see #12416