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.

Sign up for GitHub

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

update_option() strict checks can cause false negatives #5139

Closed
wants to merge 23 commits into from
Closed

update_option() strict checks can cause false negatives #5139

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0d02f09
Update patch
mukeshpanchal27 Sep 5, 2023
00a66a7
Address review feedback and minor changes
mukeshpanchal27 Sep 11, 2023
e02ac1c
Fix unit test
mukeshpanchal27 Sep 11, 2023
194cdf6
Revert BC changes
mukeshpanchal27 Sep 12, 2023
6497e62
Add util function is_equal_database_value
mukeshpanchal27 Sep 12, 2023
e5a5eac
Add/update unit tests
mukeshpanchal27 Sep 12, 2023
1f48d90
Remove local variable and update function name
mukeshpanchal27 Sep 12, 2023
233c605
Address review feedback and add more unit test data
mukeshpanchal27 Sep 13, 2023
900bf38
Revert changes for update_network_option()
mukeshpanchal27 Sep 15, 2023
b40f757
Merge branch 'WordPress:trunk' into fix/22192-strict-checks
mukeshpanchal27 Sep 20, 2023
9f7b4f7
Added unit test to check option behaviour
mukeshpanchal27 Sep 20, 2023
bf1ef31
Address review feedback
mukeshpanchal27 Sep 21, 2023
ccf6435
Check option value after update_option in unit tests
mukeshpanchal27 Sep 21, 2023
1c9ef85
Address review feedback
mukeshpanchal27 Sep 21, 2023
ea8adb3
Added unit test to check option behaviour
mukeshpanchal27 Sep 21, 2023
5e233ff
Add unit test from 5250
mukeshpanchal27 Sep 21, 2023
a924b31
Merge branch 'trunk' into fix/22192-strict-checks
mukeshpanchal27 Sep 21, 2023
85840cc
Remove duplicate Data provider
mukeshpanchal27 Sep 21, 2023
9adbccf
Remove duplicate unit tests
mukeshpanchal27 Sep 21, 2023
2425818
Update logic based on feedback
mukeshpanchal27 Sep 21, 2023
c4159c3
Update unit tests
mukeshpanchal27 Sep 21, 2023
48e5bcb
Apply suggestions from code review
mukeshpanchal27 Sep 22, 2023
7e8a64f
Merge branch 'trunk' into fix/22192-strict-checks
felixarntz Sep 25, 2023
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
79 changes: 56 additions & 23 deletions src/wp-includes/option.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,8 +754,9 @@ function update_option( $option, $value, $autoload = null ) {
$value = clone $value;
}

$value = sanitize_option( $option, $value );
$old_value = get_option( $option );
$value = sanitize_option( $option, $value );
$old_value = get_option( $option );
$option_exists = false !== $old_value;

/**
* Filters a specific option before its value is (maybe) serialized and updated.
Expand All @@ -782,16 +783,8 @@ function update_option( $option, $value, $autoload = null ) {
*/
$value = apply_filters( 'pre_update_option', $value, $option, $old_value );

/*
* If the new and old values are the same, no need to update.
*
* Unserialized values will be adequate in most cases. If the unserialized
* data differs, the (maybe) serialized data is checked to avoid
* unnecessary database calls for otherwise identical object instances.
*
* See https://core.trac.wordpress.org/ticket/38903
*/
if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
// If the new and old values are the same, no need to update.
if ( $option_exists && is_equal_database_value( $old_value, $value ) ) {
return false;
}

Expand Down Expand Up @@ -2086,7 +2079,8 @@ function update_network_option( $network_id, $option, $value ) {

wp_protect_special_option( $option );

$old_value = get_network_option( $network_id, $option, false );
$old_value = get_network_option( $network_id, $option, false );
$option_exists = false !== $old_value;

/**
* Filters a specific network option before its value is updated.
Expand All @@ -2105,16 +2099,8 @@ function update_network_option( $network_id, $option, $value ) {
*/
$value = apply_filters( "pre_update_site_option_{$option}", $value, $old_value, $option, $network_id );

/*
* If the new and old values are the same, no need to update.
*
* Unserialized values will be adequate in most cases. If the unserialized
* data differs, the (maybe) serialized data is checked to avoid
* unnecessary database calls for otherwise identical object instances.
*
* See https://core.trac.wordpress.org/ticket/44956
*/
if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
// If the new and old values are the same, no need to update.
if ( $option_exists && is_equal_database_value( $old_value, $value ) ) {
return false;
}

Expand Down Expand Up @@ -2893,3 +2879,50 @@ function filter_default_option( $default_value, $option, $passed_default ) {

return $registered[ $option ]['default'];
}

/**
* Check if two database values are equal.
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
*
* @since 6.4.0
* @access private
*
* @param mixed $old_value The old value to compare.
* @param mixed $new_value The new value to compare.
* @return bool True if the values are equal, false otherwise.
*/
function is_equal_database_value( $old_value, $new_value ) {
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
$values = array(
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
'old' => $old_value,
'new' => $new_value,
);

foreach ( $values as $_key => &$_value ) {
// Special handling for false-ish values.
if ( false === $_value ) {
$_value = '0';

// phpcs:ignore Generic.WhiteSpace.ScopeIndent.Incorrect
// Empty strings in the database should be seen as equivalent to false-ish cache values.
} elseif ( 'old' === $_key && '' === $_value ) {
$_value = '0';

// phpcs:ignore Generic.WhiteSpace.ScopeIndent.Incorrect
// Cast scalars to a string so type discrepancies don't result in cache misses.
} elseif ( is_scalar( $_value ) ) {
$_value = (string) $_value;
}
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
}

if ( $values['old'] === $values['new'] ) {
return true;
}

/*
* Unserialized values will be adequate in most cases. If the unserialized
* data differs, the (maybe) serialized data is checked to avoid
* unnecessary database calls for otherwise identical object instances.
*
* See https://core.trac.wordpress.org/ticket/38903
*/
return maybe_serialize( $old_value ) === maybe_serialize( $new_value );
}
69 changes: 69 additions & 0 deletions tests/phpunit/tests/option/isEqualDatabaseValue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php
/**
* Test is_equal_database_value().
*
* @covers ::is_equal_database_value
*/
class Tests_Is_Equal_Database_Value extends WP_UnitTestCase {
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved

/**
* @ticket 22192
*
* @dataProvider data_is_equal_database_value
*
* @param mixed $old_value The old value to compare.
* @param mixed $new_value The new value to compare.
* @param int $expected The expected result.
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
*/
public function test_is_equal_database_value( $old_value, $new_value, $expected ) {
$this->assertEquals( $expected, is_equal_database_value( $old_value, $new_value ) );
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Data provider.
*
* @return array
*/
public function data_is_equal_database_value() {
return array(
// Equal values.
array( '123', '123', true ),

// Not equal values.
array( '123', '456', false ),

// Truthy.
array( 1, '1', true ),
array( 1.0, '1', true ),
array( '1', '1', true ),
array( true, '1', true ),
array( '1.0', '1', false ),
array( ' ', '1', false ),
array( array( '0' ), '1', false ),
array( new stdClass(), '1', false ),
array( 'Howdy, admin!', '1', false ),
joemcgill marked this conversation as resolved.
Show resolved Hide resolved

// False-ish values and empty strings.
array( 0, '0', true ),
array( 0.0, '0', true ),
array( '0', '0', true ),
array( '', '0', true ),
array( false, '0', true ),
array( null, '0', false ),
array( array(), '0', false ),

// Object values.
array( (object) array( 'foo' => 'bar' ), (object) array( 'foo' => 'bar' ), true ),
array( (object) array( 'foo' => 'bar' ), (object) array( 'foo' => 'baz' ), false ),
array( (object) array( 'foo' => 'bar' ), serialize( (object) array( 'foo' => 'bar' ) ), false ),
array( serialize( (object) array( 'foo' => 'bar' ) ), (object) array( 'foo' => 'bar' ), false ),
array( serialize( (object) array( 'foo' => 'bar' ) ), (object) array( 'foo' => 'baz' ), false ),

// Serialized values.
array( array( 'foo' => 'bar' ), serialize( array( 'foo' => 'bar' ) ), false ),
array( array( 'foo' => 'bar' ), serialize( array( 'foo' => 'baz' ) ), false ),
array( serialize( (object) array( 'foo' => 'bar' ) ), serialize( (object) array( 'foo' => 'bar' ) ), true ),
array( serialize( (object) array( 'foo' => 'bar' ) ), serialize( (object) array( 'foo' => 'baz' ) ), false ),
);
}
}
147 changes: 147 additions & 0 deletions tests/phpunit/tests/option/option.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,4 +312,151 @@ public function data_option_autoloading() {
array( 'autoload_false', false, 'no' ),
);
}

/**
* @ticket 22192
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
*
* @covers ::add_option
*/
public function test_add_option_with_value_of_false_should_store_false_in_the_cache() {
add_option( 'foo', false );
$a = wp_cache_get( 'alloptions', 'options' );
$this->assertSame( false, $a['foo'] );
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @ticket 22192
*
* @covers ::add_option
*/
public function test_add_option_with_value_of_false_should_store_empty_string_in_the_database() {
add_option( 'foo', false );

// Delete cache to ensure we pull from the database.
wp_cache_delete( 'alloptions', 'options' );

$this->assertSame( '', get_option( 'foo' ) );
}

/**
* @ticket 22192
*
* @covers ::add_option
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
* @covers ::update_option
*
* @dataProvider data_update_option_type_juggling
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
*
* @param mixed $old_value One of the values to compare.
* @param mixed $new_value The other value to compare.
*/
public function test_update_option_should_hit_cache_when_loosely_equal_to_existing_value_and_cached_values_are_faithful_to_original_type( $old_value, $new_value ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test appears to have been included in an earlier patch on the ticket. Despite its name, I'm still not sure what this test is actually testing.

update_option() is a "setter" function, so the number of queries should change as the database is updated. This test reads more like it's testing a "getter" function, where the number of queries should be reduced when a value exists in the cache.

Aside from that, why are we testing that the cache is hit? What do we expect to happen to the cache?

Copy link
Member

Choose a reason for hiding this comment

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

I think this test as written is trying to confirm behavior which I think is wrong. From what I can tell, this is essentially ensuring that if an option is set and then updated to a value that is loosely equal, but not strictly equal (e.g., true and 1), then the option will not be updated. However, I think that's wrong because there are valid reasons why you may want to update an option value from one type to another.

Instead, what I think this ticket is trying to ensure, is that if you try to update an option with the same value that is strictly equal, then a database query will not be run because update_option will find that the value already exists in cache, whereas now, the cached value is not strictly equal for some reason.

Maybe I'm misunderstanding?

Copy link
Member Author

Choose a reason for hiding this comment

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

I run test_update_option_should_hit_cache_when_loosely_equal_to_existing_value_and_cached_values_are_faithful_to_original_type unit test for old and new/updated update_option function with dataset. Here is the observation.

For all this dataset value it hit DB query in old update_option and it will not run any additional query in updated function.

('1', 1)
('1', 1.0)
('1', true)
(1, 1)
(1, 1.0)
(1, true)
(1.0, 1)
(1.0, 1.0)
(1.0, true)
(true, 1)
(true, 1.0)
(true, true)
('0', 0)
('0', 0.0)
('0', false)
(0, 0)
(0, 0.0)
(0, false)
(0.0, 0)
(0.0, 0.0)
(0.0, false)

For all this dataset values it update the option in old update_option function but not in updated function.

('0', false)
(0, false)
(0.0, false)

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, what I think this ticket is trying to ensure, is that if you try to update an option with the same value that is strictly equal, then a database query will not be run because update_option will find that the value already exists in cache, whereas now, the cached value is not strictly equal for some reason.

That's right yeah, though the ticket also says:

[The functions] should be smart enough to recognize that "1" == 1 == true and "0" == 0 == false, and that any numeric string is also equal to a properly cast integer.

So that's where the if loosely equal, don't update comes from.

From what I can tell, this is essentially ensuring that if an option is set and then updated to a value that is loosely equal, but not strictly equal (e.g., true and 1), then the option will not be updated. However, I think that's wrong because there are valid reasons why you may want to update an option value from one type to another.

This also occurred to me when looking at the ticket and PR, and I had the same opinion as you but I didn't have any example use cases in mind.

Copy link
Member

Choose a reason for hiding this comment

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

Spending some more time with this issue and it does seem like I misunderstood the details of what the current behavior is and what we are trying to achieve here. I've created an example PR that includes the basic unit tests from this PR along with a complete set of truthy and falsey use cases. You can see that there are 30 use cases where the tests fail (i.e, updates are happening when we don't expect them)—all of which are currently falsey use cases. So it seems to me that the original issue as reported might already be mostly fixed.

Also, (except for some falsey use cases) the current behavior already shows that you are not able to update the value of an option to a loosely equivalent scalar value of a different type (e.g., from '1' to true), So the goal here seems to be to further reduce the times where falsey values are being compared but result in a DB update when values are loosely equivalent.

If there are falsey use cases that MUST keep the current behavior, then we should also add a unit test to ensure those are not effected by this, or future changes.

Copy link
Member

Choose a reason for hiding this comment

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

If it would be helpful, I think it would be fine to add a passing version of these tests into trunk prior to any changes here. That said, I think it would be ok to do it all as a part of this ticket. What is not super clear to me from what I'm observing from the tests in my PR is whether any of the use cases that are currently failing can actually be fixed without introducing backwards compatibility breaks.

Ideally, we should have a complete set of use cases that all pass with the expected behavior of not doing a DB update when the new value is loosely equal to the existing value. For any cases where we MUST do DB updates even on loosely equivalent values, we should also have tests that confirm that behavior with some inline docs explaining why these can't be changed.

Copy link
Member

Choose a reason for hiding this comment

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

@joemcgill I think either way works, but committing these tests standalone to trunk would clarify (also to future reviewers of this PR) that those changes continue to pass. Adding them here does not make it clear whether those tests would have also passed prior, i.e. whether they cover new or existing behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Let me update the other PR so that it confirms only current behavior and I'll link it to that ticket so we can decide to commit it or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

committing these tests standalone to trunk would clarify (also to future reviewers of this PR) that those changes continue to pass. Adding them here does not make it clear whether those tests would have also passed prior, i.e. whether they cover new or existing behavior.

+1

Also that by having tests already in trunk, if any of the tests are intended to change, those changes will be clear to see in this PR and changeset diff when this PR is committed. That means if there are any BC breaks we didn't catch during review, finding the problem doesn't mean to looking at every dataset, but only the changed ones.

Copy link
Member

Choose a reason for hiding this comment

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

Now that a set of tests confirming current behavior are in trunk, via https://core.trac.wordpress.org/changeset/56648, is this test still needed?

add_option( 'foo', $old_value );
$num_queries = get_num_queries();

$updated = update_option( 'foo', $new_value );

$this->assertFalse( $updated, 'update_option should not return true when values are loosely equal.' );
$this->assertSame( $num_queries, get_num_queries(), 'The number of database queries should not change.' );
}
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved

/**
* @ticket 22192
*
* @covers ::add_option
* @covers ::update_option
*
* @dataProvider data_update_option_type_juggling
*
* @param mixed $old_value One of the values to compare.
* @param mixed $new_value The other value to compare.
*/
public function test_update_option_should_hit_cache_when_loosely_equal_to_existing_value_and_cached_values_are_pulled_from_the_database( $old_value, $new_value ) {
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
add_option( 'foo', $old_value );
wp_cache_delete( 'alloptions', 'options' );
wp_load_alloptions();

$num_queries = get_num_queries();

$updated = update_option( 'foo', $new_value );

$this->assertFalse( $updated, 'update_option should not return true when values are loosely equal.' );
$this->assertSame( $num_queries, get_num_queries(), 'The number of database queries should not change.' );
}

/**
* Data provider.
*
* @return array
*/
public function data_update_option_type_juggling() {
return array(
// Truthy.
array( '1', '1' ),
array( '1', intval( 1 ) ),
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
array( '1', floatval( 1 ) ),
array( '1', true ),
array( 1, '1' ),
array( 1, intval( 1 ) ),
array( 1, floatval( 1 ) ),
array( 1, true ),
array( floatval( 1 ), '1' ),
array( floatval( 1 ), intval( 1 ) ),
array( floatval( 1 ), floatval( 1 ) ),
array( floatval( 1 ), true ),
array( true, '1' ),
array( true, intval( 1 ) ),
array( true, floatval( 1 ) ),
array( true, true ),

// Falsey.
array( '0', '0' ),
array( '0', intval( 0 ) ),
array( '0', floatval( 0 ) ),
array( '0', false ),
array( 0, '0' ),
array( 0, intval( 0 ) ),
array( 0, floatval( 0 ) ),
array( 0, false ),
array( floatval( 0 ), '0' ),
array( floatval( 0 ), intval( 0 ) ),
array( floatval( 0 ), floatval( 0 ) ),
array( floatval( 0 ), false ),
);
}

/**
* @ticket 22192
*
* @covers ::add_option
* @covers ::update_option
*
* @dataProvider data_update_option_type_falsey_values
*
* @param mixed $old_value One of the values to compare.
* @param mixed $new_value The other value to compare.
* @param int $expected The expected result.
*/
public function test_update_option_with_falsey_values( $old_value, $new_value, $expected ) {
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
add_option( 'foo', $old_value );
$num_queries = get_num_queries();

$updated = update_option( 'foo', $new_value );

$this->assertSame( $expected, $updated, 'update_option should not match expeted value when values are loosely equal.' );
$this->assertSame( 1, get_num_queries() - $num_queries, 'The number of database queries should not change.' );
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
}

mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
/**
* Data provider.
*
* @return array
*/
public function data_update_option_type_falsey_values() {
return array(
array( false, '0', true ),
array( false, intval( 0 ), true ),
array( false, floatval( 0 ), true ),
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
array( false, false, false ),
);
}
costdev marked this conversation as resolved.
Show resolved Hide resolved
}