Option nonexistence should not be checked against false

[GregLone]

Hello.

In 3.4, was introduced a new filter "default_option_{$option_name}" in get_option(). If used, it might cause problems.
Example:

• I want my option 'foobar' to return an empty array by default instead of false: add_filter('default_option_foobar', '__return_empty_array').
• My option does not exist in the database yet.
• For simpler code, I use update_option(), it will fallback to add_option() if my option does not exist anyway.

Let's see the important parts in update_option():

$old_value = get_option( $option );
// ...
if ( false === $old_value )
    return add_option( $option, $value );

In this case, I've filtered get_option() to return an empty array, not false. So update_option() will never trigger add_option(). As a result, the option will never be created.
We have the same test in add_option(), so the same kind of problem.

Thanks.

[GregLone]

Ho, I forgot the important part, the possible solution x)
Imho, it should be:

$old_value = get_option( $option );
// ...
if ( apply_filters( 'default_option_' . $option, false ) === $old_value )
    return add_option( $option, $value );

[tyxla]

Taking advantage of the default_option_* filter in update_option() and add_option().

[tyxla]

I was able to reproduce this issue, and I wonder how this wasn't found and fixed earlier.

The solution that @GregLone suggests fixes the issue, as this way the default value is always accurate, instead of assuming it is always false.

Attached a patch that addresses this issue in both update_option() and add_option().

[SergeyBiryukov]

Related: #22192

[boonebgorges]

I wonder how this wasn't found and fixed earlier.

Me too.

31047.2.patch​ adds unit tests demonstrating the problem and solution. Let's fix this for 4.2.

[nacin]

Is #22192 a duplicate?

[Denis-de-Bernardy]

Replying to nacin:

Is #22192 a duplicate?

It doesn't seem so. As I understand the two tickets, the one you highlighted is related to a cache issue, whereas this one is related to comparing apples (false) to oranges (the actual default) when checking for a default value.

My gut reaction when I initially read the ticket was to wonder whether it was valid at all: it's convenient in some sense to standardize around false, as it currently means the option doesn't exist, because it's not currently possible or practical to store false as a value using the options API. If I understand the proposed patch correctly, we're going to potentially add a DB query for any get_option() call with a non-existent value that has the function's default value parameter set. So that point is worth checking for. I'm not entirely sure of the other potential side-effects of the patch off the top of my head.

[boonebgorges]

Is #22192 a duplicate?

No. #22192 is a typecasting issue. Scalar option/meta values are always going to be cast to string when pulled from the database and stored in the cache. So when we compare old values against new values in the update_ functions (eg at https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/meta.php#L190) we should either (a) do == instead of ===, or (b) if ( is_scalar( $meta_value ) ) $_meta_value = (string) $meta_value; (I'm guessing (b) is safer, but we need unit tests to describe it.)

This issue is related to the use of the "default_option_$option" filter. The bug is that if you use this filter, and the 'alloptions' cache is not populated, it's impossible to add/update the option.

[boonebgorges]

In 31473:

Respect 'default_option_' filters during early sanity checks in add_option() and update_option().

add_option() and update_option() both call get_option() to compare the
value passed to the function with any existing value for the given option name.
When a 'default_option_' filter is in place to change the default value of
an option, add_option() and update_option() ought to check against the
filtered value, rather than a hardcoded false, in order to determine whether
a prior value exists.

Props GregLone, tyxla.
Fixes #31047.