update_option return value ambiguous

[cloughit]

WordPress 4.8

The return value 'false' does not allow for differentiating if there was an error saving or if the option value already exists and is the same as the new value.

Consider this scenario: a plugin sends option values via AJAX to be updated. The AJAX function returns the return value of 'update_option' which determines the feedback provided to the user, such as a 'success' or 'failure' message.

Rather than returning 'false' if an option already exists and is the same as the new option, I suggest returning NULL. In this way the return value of 'update_option' can be checked as follows:

if ( is_null( update_option( $option, $value, $autoload) ) ) { ... } // option exists and value is the same as existing option value
if ( false === update_option( $option, $value, $autoload) ) { ... } // an error occured when saving the option
if ( update_option( $option, $value, $autoload) ) { ... } // option updated successfully

The 'update_option' function would need line 308 changed from:

return false;

to:

return NULL;

[alexvorn2]

Is your ticket the same issue as my ticket? #40007

It seems the bug is in update_option function.

[mdifelice]

Hi @cloughit I had the same problem, and I think that update_option should return NULL also when no option is set, but that kind of change would have such a big impact that it would be necessary to update a lot of files.

My problem happened using the cache functions. All options are cached by the object cache plugin (if you are using it), but if you save a false value in the option the cache plugin interprets that the options is not cached and it tries to fetch it from the database. So, any option with the false value is not being cached in practice.

The solution I found is using string values: "yes" or "no". In fact, WordPress uses those values for its boolean options and I guess that the reason to do that is to identify if the options was created or not. It is not ideal, because you are using more space to store and transfer the options but I guess this is the only solution for now.

[cloughit]

@mdifelice @alexvorn2 My issue differs as I am concerned with the return values of 'update_option' rather than trying to save a boolean (in your case 'false').

Consider a scenario where option 'foo' exists with value 'bar':

update_option( 'foo', 'ram' ) - returns true (Line 386)
update_option( 'foo', 'bar' ) - returns false (Line 308 - option exists and is the same value)
update_option( 1, 'ram' ) - returns false (Line 344 - error inserting in database)
update_option( ' ', 'ram' ) - returns false (Line 263 - empty option name)

If (as it is in my case) it is necessary to provide user feedback on the status of an option update (such as 'Settings Saved!), there is no way to differentiate if the returned 'false' value is due to option exists / same or an error occurred saving.

By returning NULL at Line 308 it would be possible to then differentiate and handle responses correctly without the overhead of calling 'get_option'. For example:

$update = update_option( 'foo', 'bar' );
if ( false === $update ) {
    {... show 'Error Saving Settings' message ...}
} else if ( is_null( $update ) ) {
    {... show 'No Changes Made' message ...}
} else {
    {... show 'Settings Saved!' message ...}
}

Currently the only way to achieve the desired result:

$update = ( get_option( 'foo' ) == 'bar' ) ? NULL : update_option( 'foo', 'bar' ); // returns either (bool)true, (bool)false or NULL
if ( false === $update ) {
    {... show 'Error Saving Settings' message ...}
} else if ( is_null( $update ) ) {
    {... show 'No Changes Made' message ...}
} else {
    {... show 'Settings Saved!' message ...}
}

I believe that this differentiation should occur within the 'update_option' function.

[mdifelice]

My mistake @cloughit. You are right, I was thinking in get_option() instead of update_option(). So sorry for that.

[alexvorn2]

I think in this case the function should return true, and that's all.

update_option( 'foo', 'ram' ) - should return true
update_option( 'foo', 'bar' ) - should return true
update_option( 1, 'ram' ) - should return false (Line 344 - error inserting in database)
update_option( ' ', 'ram' ) - should return false (Line 263 - empty option name)

To do this we need to change how the function works.

[travisnorthcutt]

It seems perfectly logical that update_option() should return an instance of WP_Error if updating failed. Changing existing behavior on the "no update" scenario (from false to null) seems somewhat non-backwards compatible... though anyone relying on false to indicate "no update" is already doing so unreliably, so ¯\_(ツ)_/¯.

The case where this would fail is if anyone is checking the return value non-strictly, because WP_Error is truthy.

[travisnorthcutt]

As a point of reference, when @cloughit refers to line 308, I believe they are referencing the return false at this line:
​https://github.com/WordPress/WordPress/blob/b3a9479bd30bee60e1486f2e23e4772d4957af7c/wp-includes/option.php#L308

if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
        return false;
}