WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#31047 closed defect (bug) (fixed)

Option nonexistence should not be checked against false

Reported by: GregLone Owned by: boonebgorges
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.4
Component: Options, Meta APIs Keywords: has-patch needs-testing
Focuses: Cc:
PR Number:

Description

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.

Attachments (2)

31047.patch (928 bytes) - added by tyxla 5 years ago.
Taking advantage of the default_option_* filter in update_option() and add_option().
31047.2.patch (2.4 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (10)

#1 @GregLone
5 years ago

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
5 years ago

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

#2 @tyxla
5 years ago

  • Keywords has-patch needs-testing added

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().

#4 @boonebgorges
5 years ago

  • Milestone changed from Awaiting Review to 4.2

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.

#5 follow-up: @nacin
5 years ago

Is #22192 a duplicate?

#6 in reply to: ↑ 5 @Denis-de-Bernardy
5 years ago

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.

#7 @boonebgorges
5 years ago

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.

#8 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.