Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug when comparing options that have a pre filter. #5313

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions src/wp-includes/option.php
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,24 @@ function update_option( $option, $value, $autoload = null ) {
*/
$value = apply_filters( 'pre_update_option', $value, $option, $old_value );

/*
* To get the actual raw old value from the database, any existing pre filters need to be temporarily disabled.
* Immediately after getting the raw value, they are reinstated.
* The raw value is only used to determine whether a value is present in the database. It is not used anywhere
* else, and is not passed to any of the hooks either.
*/
if ( has_filter( "pre_option_{$option}" ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the pre_option filter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacedmonkey I agree that in principle there could be the same bug due to usage of pre_option too, but I intentionally did not consider the general pre_option filter, because it may have wider reaching implications, so I think unhooking it temporarily is too risky.

There's a good chance that a site that uses that filter has some special mechanisms in place for retrieving and storing options that shouldn't be unhooked.

That is different with the pre_option_{$option} filter, which is clearly about specific options.

global $wp_filter;

$old_filters = $wp_filter[ "pre_option_{$option}" ];
unset( $wp_filter[ "pre_option_{$option}" ] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unset( $wp_filter[ "pre_option_{$option}" ] );
remove_all_filters( "pre_option_{$option}" );

Could we use this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacedmonkey I tried it, but the problem is that it also calls the remove_all_filters() method on the WP_Hook object in $wp_filter[ "pre_option_{$option}" ]. So we then cannot reinstate the filters that were previously there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment saying why we didnt use remove_all_filters.


$raw_old_value = get_option( $option );
$wp_filter[ "pre_option_{$option}" ] = $old_filters;
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
} else {
$raw_old_value = $old_value;
}

/** This filter is documented in wp-includes/option.php */
$default_value = apply_filters( "default_option_{$option}", false, $option, false );

Expand All @@ -787,11 +805,11 @@ function update_option( $option, $value, $autoload = null ) {
*
* See https://core.trac.wordpress.org/ticket/38903 and https://core.trac.wordpress.org/ticket/22192.
*/
if ( $old_value !== $default_value && _is_equal_database_value( $old_value, $value ) ) {
if ( $raw_old_value !== $default_value && _is_equal_database_value( $raw_old_value, $value ) ) {
return false;
}

if ( $old_value === $default_value ) {
if ( $raw_old_value === $default_value ) {

// Default setting for new options is 'yes'.
if ( null === $autoload ) {
Expand Down
55 changes: 54 additions & 1 deletion tests/phpunit/tests/option/option.php
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,6 @@ public function test_update_loosey_options_from_refreshed_cache( $old_value, $ne
}
}


/**
* Data provider.
*
Expand Down Expand Up @@ -581,4 +580,58 @@ static function () use ( $default_value ) {

$this->assertSame( 1, $actual );
}

/**
* Tests that a non-existing option is added even when its pre filter returns a value.
*
* @ticket 22192
*
* @covers ::update_option
*/
public function test_update_option_with_pre_filter_adds_missing_option() {
// Force a return value of integer 0.
add_filter( 'pre_option_foo', '__return_zero' );

/*
* This should succeed, since the 'foo' option does not exist in the database.
* The default value is false, so it differs from 0.
*/
$this->assertTrue( update_option( 'foo', 0 ) );
}

/**
* Tests that an existing option is updated even when its pre filter returns the same value.
*
* @ticket 22192
*
* @covers ::update_option
*/
public function test_update_option_with_pre_filter_updates_option_with_different_value() {
// Add the option with a value of 1 to the database.
add_option( 'foo', 1 );

// Force a return value of integer 0.
add_filter( 'pre_option_foo', '__return_zero' );

/*
* This should succeed, since the 'foo' option has a value of 1 in the database.
* Therefore it differs from 0 and should be updated.
*/
$this->assertTrue( update_option( 'foo', 0 ) );
}

/**
* Tests that calling update_option() does not permanently remove pre filters.
*
* @ticket 22192
*
* @covers ::update_option
*/
public function test_update_option_maintains_pre_filters() {
add_filter( 'pre_option_foo', '__return_zero' );
update_option( 'foo', 0 );

// Assert that the filter is still present.
$this->assertSame( 10, has_filter( 'pre_option_foo', '__return_zero' ) );
}
}
Loading