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

update_option() strict checks can cause false negatives #5139

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
53 changes: 46 additions & 7 deletions src/wp-includes/option.php
Original file line number Diff line number Diff line change
Expand Up @@ -776,21 +776,23 @@ function update_option( $option, $value, $autoload = null ) {
*/
$value = apply_filters( 'pre_update_option', $value, $option, $old_value );

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

/*
* 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.
* An exception applies when no value is set in the database, i.e. the old value is the default.
* In that case, the new value should always be added as it may be intentional to store it rather than relying on the default.
*
* See https://core.trac.wordpress.org/ticket/38903
* See https://core.trac.wordpress.org/ticket/38903 and https://core.trac.wordpress.org/ticket/22192.
*/
if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
if ( $old_value !== $default_value && _is_equal_database_value( $old_value, $value ) ) {
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

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

// Default setting for new options is 'yes'.
if ( null === $autoload ) {
$autoload = 'yes';
Expand Down Expand Up @@ -2887,3 +2889,40 @@ function filter_default_option( $default_value, $option, $passed_default ) {

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

/**
* Determines whether two values will be equal when stored in the database.
*
* @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 ) {
$values = array(
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
'old' => $old_value,
'new' => $new_value,
);

foreach ( $values as $_key => &$_value ) {
// Cast scalars or null to a string so type discrepancies don't result in cache misses.
if ( null === $_value || is_scalar( $_value ) ) {
$_value = (string) $_value;
}
}

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 );
}
71 changes: 71 additions & 0 deletions tests/phpunit/tests/option/isEqualDatabaseValue.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php
/**
* Tests for _is_equal_database_value().
*
* @group option
*
* @covers ::_is_equal_database_value
*/
class Tests_Option_IsEqualDatabaseValue extends WP_UnitTestCase {

/**
* @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 bool $expected The expected result.
*/
public function test_is_equal_database_value( $old_value, $new_value, $expected ) {
$this->assertSame( $expected, _is_equal_database_value( $old_value, $new_value ) );
}

/**
* 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', false ),
array( false, '0', false ),
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 ),
);
}
}
57 changes: 57 additions & 0 deletions tests/phpunit/tests/option/option.php
Original file line number Diff line number Diff line change
Expand Up @@ -524,4 +524,61 @@ public function data_update_option_type_juggling() {
array( null, array(), true ),
);
}

/**
* Tests that update_option() stores an option that uses
* an unfiltered default value of (bool) false.
*
* @ticket 22192
*
* @covers ::update_option
*/
public function test_update_option_should_store_option_with_default_value_false() {
global $wpdb;

$option = 'update_option_default_false';
update_option( $option, false );

$actual = $wpdb->query(
$wpdb->prepare(
"SELECT option_name FROM $wpdb->options WHERE option_name = %s LIMIT 1",
$option
)
);

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

mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
/**
* Tests that update_option() stores an option that uses
* a filtered default value.
*
* @ticket 22192
*
* @covers ::update_option
*/
public function test_update_option_should_store_option_with_filtered_default_value() {
global $wpdb;

$option = 'update_option_custom_default';
$default_value = 'default-value';

add_filter(
"default_option_{$option}",
static function () use ( $default_value ) {
return $default_value;
}
);

update_option( $option, $default_value );

$actual = $wpdb->query(
$wpdb->prepare(
"SELECT option_name FROM $wpdb->options WHERE option_name = %s LIMIT 1",
$option
)
);

$this->assertSame( 1, $actual );
}
costdev marked this conversation as resolved.
Show resolved Hide resolved
}