WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 13 days ago

#21989 new defect (bug)

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

Reported by: MikeSchinkel Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: dev-feedback has-patch
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 (1)

21989.diff (1.3 KB) - added by Denis-de-Bernardy 13 days ago.
Fallback immediately on non-existent options in update_option() and update_site_option()

Download all attachments as: .zip

Change History (10)

comment:1 nacin19 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 MikeSchinkel19 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 nacin19 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 totels11 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 11 months ago by totels (previous) (diff)

comment:5 jeremyfelt3 months ago

  • Component changed from Administration to Options and Meta

comment:6 follow-up: Denis-de-Bernardy13 days 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 ericlewis13 days 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-Bernardy13 days ago

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

comment:8 Denis-de-Bernardy13 days ago

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

comment:9 Denis-de-Bernardy13 days 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.

Note: See TracTickets for help on using tickets.