WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 4 days ago

#21989 new defect (bug)

update_option() calls sanitize_option() twice when option does not exist

Reported by: MikeSchinkel Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: dev-feedback has-patch needs-testing
Focuses: Cc:

Description

I just spent several hours tracking down an issue when using the Settings API where sanitize_option() is called twice which is unnecessary execution especially if sanitization includes calling an external API for username/password authorization (this was how another developer set it up for a plugin I was debugging.)

What happens is that a call to update_option() will call sanitize_option() and then if the option wasn't previously in the options table update_option() will delegate to add_option() which calls santize_option() a second time. This would normally be easy to workaround by first calling get_option() and testing for false and calling add_option() instead of update_option() if false, but not when the Settings API chooses how to call it.

I've looked at the problem and can envision several different ways to solve it such but don't know which the core developers would choose (or if they'd choose yet another option I haven't envisioned) so I didn't submit a patch:

  • Adding a 3rd parameter $mode to sanitize_option() that identifies the mode ('add', 'edit', default = 'unknown') and thus allow the hook to ignore one of the options (this would be more backward compatible but would put the onus on the developer to know to do this),
  • Adding a 5th parameter $bypass_sanitize to add_option() defaulted to false that is only passed as true in update_option() allowing update_option() to disable the call to sanitize_option() found in add_option() (this would be less backward compatible and hacky, but seemless to the developer)
  • Adding a 3rd parameter $bypass_sanitize to update_option() defaulted to false that is only passed as true from /wp-admin/options.php allowing update_option() to bypass the call to sanitize_option() if an add_option() will happen (this would be less backward compatible and hacky, but seemless to the developer)
  • Have /wp-admin/options.php test for no pre-existing option and then call add_option() or update_option(), respectively (this would be seemless to the developer but less backward compatible and not hacky, and it wouldn't handle the general case.)

So is this worth fixing to save other users and developers debugging time, and to reduce the chance of subtle errors in new code? If yes, how best to approach?

Attachments (2)

21989.diff (1.3 KB) - added by Denis-de-Bernardy 3 months ago.
Fallback immediately on non-existent options in update_option() and update_site_option()
21989.2.diff (1.6 KB) - added by rmccue 4 days ago.
Add new parameter to add_option

Download all attachments as: .zip

Change History (15)

comment:1 nacin22 months ago

  • Summary changed from update_option('example') calls sanitize_option('example') twice when false===get_option('example') to update_option() calls sanitize_option() twice when option does not exist

comment:2 MikeSchinkel22 months ago

As a quick update about how I worked around what was causing the problem, I moved the API authentication code into admin_init() and read and updated $_POST directly making sure to only do so on an update for this one plugin's admin settings page.

Just FYI for anyone else who might have the same issue prior to this getting fixed in core or if it does not get fixed in core.

comment:3 nacin22 months ago

I imagine the issue here is specifically related to the saving of a settings screen, and not usage of update_option() at the API level. In that case, it seems like the onus is on the developer to ensure such an option is initialized in the database. There are two schools of thought, but an option like this should certainly be initialized if that kind of side effect could occur.

If there were any possible route to an elegant solution here, I probably wouldn't be suggesting essentially a wontfix. But there isn't one, alas.

comment:4 totels13 months ago

Wouldn't it be easiest to just refactor and move the check for the old value and the call for add_option() before the sanitize_option() call in update_option()?

diff --git wp-includes/option.php wp-includes/option.php
index 722d1f3..4df07d8 100644
--- wp-includes/option.php
+++ wp-includes/option.php
@@ -224,17 +224,18 @@ function update_option( $option, $newvalue ) {
 	if ( is_object($newvalue) )
 		$newvalue = clone $newvalue;
 
-	$newvalue = sanitize_option( $option, $newvalue );
 	$oldvalue = get_option( $option );
+
+	if ( false === $oldvalue )
+		return add_option( $option, $newvalue );
+
+	$newvalue = sanitize_option( $option, $newvalue );
 	$newvalue = apply_filters( 'pre_update_option_' . $option, $newvalue, $oldvalue );
 
 	// If the new and old values are the same, no need to update.
 	if ( $newvalue === $oldvalue )
 		return false;
 
-	if ( false === $oldvalue )
-		return add_option( $option, $newvalue );
-
 	$notoptions = wp_cache_get( 'notoptions', 'options' );
 	if ( is_array( $notoptions ) && isset( $notoptions[$option] ) ) {
 		unset( $notoptions[$option] );}}}
Last edited 13 months ago by totels (previous) (diff)

comment:5 jeremyfelt6 months ago

  • Component changed from Administration to Options and Meta

comment:6 follow-up: Denis-de-Bernardy3 months ago

Any odds this can get fixed in WP 3.9? (If so, do you need the above patch in diff format, or can you pick it up from there?)

comment:7 in reply to: ↑ 6 ericlewis3 months ago

Replying to Denis-de-Bernardy:

Any odds this can get fixed in WP 3.9? (If so, do you need the above patch in diff format, or can you pick it up from there?)

Given the Milestone is "Awaiting Review" on the ticket, it's not slated for 3.9 (or "Future Release" for that matter).

However, a diff as an attachment on this ticket is preferred rather than in a comment.

Denis-de-Bernardy3 months ago

Fallback immediately on non-existent options in update_option() and update_site_option()

comment:8 Denis-de-Bernardy3 months ago

  • Keywords has-patch added; needs-patch 2nd-opinion removed

comment:9 Denis-de-Bernardy3 months ago

Aside on this: the pre_update_ filters and not-option wrangling don't get done consistently in update_option() and update_site_option(). Probably for a second ticket, but worth noting regardless.

comment:10 wonderboymusic2 weeks ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 4.0

comment:11 follow-up: rmccue4 days ago

Just hit this myself.

Disagree with nacin that it's the developer's responsibility to check this. If I do something like register_setting( 'group', 'opt_name', 'nonidempotent_func' ), it doesn't make sense to me that the first save (that creates the option) should behave differently to the second save (that updates the option). As far as I'm concerned as the API user, I shouldn't need to worry whether it's using add_option or update_option internally.

Both totels and ddb's patches break backwards compatibility with the pre_update_option_{$option} and pre_update_option filters, which currently always get called when using update_option. However, both operate on the sanitized version of the value, so using them would be strange.

Choices I can think of, in preferred order:

  1. Add extra parameter to add_option to skip sanitization: 21989.2.diff handles this
  2. Skip the pre_update_option filters when adding options via update_option (should probably add pre_add_option_{$option} and pre_add_option filters as well; this also breaks BC with the filters): 21989.diff handles this

Thoughts? It'd be nice to fix this.

Last edited 4 days ago by rmccue (previous) (diff)

rmccue4 days ago

Add new parameter to add_option

comment:12 in reply to: ↑ 11 ; follow-up: Denis-de-Bernardy4 days ago

Replying to rmccue:

Both totels and ddb's patches break backwards compatibility with the pre_update_option_{$option} and pre_update_option filters, which currently always get called when using update_option. However, both operate on the sanitized version of the value, so using them would be strange.

Hold... Did you just seriously argue that maintaining backwards compat on broken was important? ;-)

Choices I can think of, in preferred order:

  1. Add extra parameter to add_option to skip sanitization: 21989.2.diff handles this
  2. Skip the pre_update_option filters when adding options via update_option (should probably add pre_add_option_{$option} and pre_add_option filters as well; this also breaks BC with the filters): 21989.diff handles this

Thoughts? It'd be nice to fix this.

I for one ink that adding a new argument borders on crazy, so I'd rather see my own patch get checked in.

As an aside, don't miss what I highlighted as I submitted it: the order of these filters and such differ in update_option and update_site_option -- and other options and transient related functions, for that matter.

comment:13 in reply to: ↑ 12 rmccue4 days ago

Replying to Denis-de-Bernardy:

Hold... Did you just seriously argue that maintaining backwards compat on broken was important? ;-)

The brokenness here only occurs for options with a sanitization callback; others are unaffected. Breaking a working hook for them here would be bad. :)

I for one ink that adding a new argument borders on crazy, so I'd rather see my own patch get checked in.

The new argument was so that we can maintain BC with the pre_update_option hooks rather than passing through early. With that said:

As an aside, don't miss what I highlighted as I submitted it: the order of these filters and such differ in update_option and update_site_option -- and other options and transient related functions, for that matter.

Interesting, I had missed that, apologies. update_site_option calls pre_update_option_{$option} but not pre_update_option. These should all be made consistent, obviously (that's probably a separate ticket, IMO).

update_site_option calls sanitize_option after pre_update_option_{$option}, so that's broken. Given that it's broken there, we might as well ignore the BC break and go with the filter-then-sanitize choice for consistency's sake. (Although multisite use with update_site_option is less common, filter-then-sanitize feels like the more correct way to do it.)

In which case, I endorse 21989.diff. :) We should also fix these crazy filters.

Note: See TracTickets for help on using tickets.