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_network_option() strict checks can cause false negatives #5434

Closed
wants to merge 22 commits into from
Closed

update_network_option() strict checks can cause false negatives #5434

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
8febe83
Update production code
mukeshpanchal27 Oct 9, 2023
1eccc72
Update unit tests
mukeshpanchal27 Oct 9, 2023
feb21d6
Update code for nullable check
mukeshpanchal27 Oct 9, 2023
daa3f24
Revert changes for nullable values
mukeshpanchal27 Oct 9, 2023
2a1d706
Remove `is_scalar` check from unit tests
mukeshpanchal27 Oct 9, 2023
2671d63
Add check for `pre_site_option_`
mukeshpanchal27 Oct 9, 2023
cedb654
Add unit tests for pre filter check
mukeshpanchal27 Oct 9, 2023
5b42a03
Use is_multisite() instead of variable
mukeshpanchal27 Oct 9, 2023
5c81311
Check for `pre_option_` for single site
mukeshpanchal27 Oct 9, 2023
11ef49f
Address review feedback
mukeshpanchal27 Oct 9, 2023
c4ac154
Apply suggestions from code review
mukeshpanchal27 Oct 9, 2023
8df882e
Add changes for default value and update unit test for pre_filter
mukeshpanchal27 Oct 9, 2023
9a2dccd
Address review feedback
mukeshpanchal27 Oct 9, 2023
f44a845
Added additional tests for pre_ filter
mukeshpanchal27 Oct 9, 2023
9b67e57
Ensure `default_value` is set in Single Site.
Oct 9, 2023
20ba3a6
Update call count `default_option_{$option}`.
Oct 9, 2023
f204d20
Tests: Improve `$message` parameters.
Oct 9, 2023
4ad219b
Separate `$default_value` from `pre_` filters.
Oct 9, 2023
f7730c8
Improve inline documentation for `null`.
Oct 9, 2023
b97bbce
Blame @joemcgill 😉
Oct 9, 2023
219f839
Tests: Reduce test skipping to minimal.
Oct 9, 2023
c61dd4b
Tests: Improve `$message` parameters - again.
Oct 9, 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
44 changes: 37 additions & 7 deletions src/wp-includes/option.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2125,7 +2125,7 @@ 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 );

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

/*
mukeshpanchal27 marked this conversation as resolved.
Show resolved Hide resolved
* 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.
*/
global $wp_filter;

$default_value_hook = is_multisite() ? "default_site_option_{$option}" : "default_option_{$option}";
$default_value = apply_filters( $default_value_hook, false, $option, $network_id );
$pre_option_hook = is_multisite() ? "pre_site_option_{$option}" : "pre_option_{$option}";

if ( has_filter( $pre_option_hook ) ) {
$old_filters = $wp_filter[ $pre_option_hook ];
unset( $wp_filter[ $pre_option_hook ] );

$raw_old_value = is_multisite() ? get_network_option( $network_id, $option ) : get_option( $option, $default_value );
costdev marked this conversation as resolved.
Show resolved Hide resolved

$wp_filter[ $pre_option_hook ] = $old_filters;
} else {
$raw_old_value = $old_value;
costdev marked this conversation as resolved.
Show resolved Hide resolved
}
felixarntz marked this conversation as resolved.
Show resolved Hide resolved

felixarntz marked this conversation as resolved.
Show resolved Hide resolved
/*
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
* 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/44956
* See https://core.trac.wordpress.org/ticket/44956 and https://core.trac.wordpress.org/ticket/22192 and https://core.trac.wordpress.org/ticket/59360
*/
if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
if (
$value === $raw_old_value ||
(
false !== $raw_old_value &&
// Site meta values are nullable. Don't check if the values are loosely equal to null.
costdev marked this conversation as resolved.
Show resolved Hide resolved
( ! is_multisite() || ( null !== $raw_old_value && null !== $value ) ) &&
_is_equal_database_value( $raw_old_value, $value )
)
) {
return false;
}

if ( false === $old_value ) {
if ( $default_value === $raw_old_value ) {
return add_network_option( $network_id, $option, $value );
}

Expand Down
202 changes: 184 additions & 18 deletions tests/phpunit/tests/option/networkOption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,17 +399,15 @@ public function data_loosely_equal_values_that_should_update() {
* @param mixed $old_value The old value.
* @param mixed $new_value The new value to try to set.
*/
public function test_update_option_should_not_update_some_values_from_cache( $old_value, $new_value ) {
public function test_update_network_option_should_not_update_some_values_from_cache( $old_value, $new_value ) {
add_network_option( null, 'foo', $old_value );

$num_queries = get_num_queries();

// Comparison will happen against value cached during add_option() above.
$updated = update_network_option( null, 'foo', $new_value );

$expected_queries = $old_value === $new_value || ! is_multisite() ? 0 : 1;
$this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." );

$this->assertSame( $num_queries, get_num_queries(), 'No additional queries should have run.' );
$this->assertFalse( $updated, 'update_network_option() should have returned false.' );
}

Expand Down Expand Up @@ -441,14 +439,7 @@ public function test_update_network_option_should_not_update_some_values_from_db

$updated = update_network_option( null, 'foo', $new_value );

// Mimic the behavior of the database by casting the old value to string.
if ( is_scalar( $old_value ) ) {
$old_value = (string) $old_value;
}

$expected_queries = $old_value === $new_value || ! is_multisite() ? 1 : 2;
$this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." );

$this->assertSame( 1, get_num_queries() - $num_queries, 'One additional query should have run to update the value.' );
$this->assertFalse( $updated, 'update_network_option() should have returned false.' );
}

Expand Down Expand Up @@ -480,9 +471,7 @@ public function test_update_network_option_should_not_update_some_values_from_re
* Strictly equal old and new values will cause an early return
* with no additional queries.
*/
$expected_queries = $old_value === $new_value || ! is_multisite() ? 0 : 1;
$this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." );

$this->assertSame( $num_queries, get_num_queries(), 'No additional queries should have run.' );
$this->assertFalse( $updated, 'update_network_option() should have returned false.' );
}

Expand Down Expand Up @@ -574,7 +563,7 @@ public function data_strictly_equal_values() {
*
* @covers ::update_network_option
*/
public function test_update_option_should_handle_a_null_new_value_from_cache() {
public function test_update_network_option_should_handle_a_null_new_value_from_cache() {
add_network_option( null, 'foo', '' );

$num_queries = get_num_queries();
Expand Down Expand Up @@ -606,7 +595,7 @@ public function test_update_option_should_handle_a_null_new_value_from_cache() {
*
* @covers ::update_network_option
*/
public function test_update_option_should_handle_a_null_new_value_from_db() {
public function test_update_network_option_should_handle_a_null_new_value_from_db() {
add_network_option( null, 'foo', '' );

$num_queries = get_num_queries();
Expand Down Expand Up @@ -642,7 +631,7 @@ public function test_update_option_should_handle_a_null_new_value_from_db() {
*
* @covers ::update_network_option
*/
public function test_update_option_should_handle_a_null_new_value_from_refreshed_cache() {
public function test_update_network_option_should_handle_a_null_new_value_from_refreshed_cache() {
add_network_option( null, 'foo', '' );

// Delete and refresh cache from DB.
Expand Down Expand Up @@ -699,4 +688,181 @@ public function data_stored_as_empty_string() {
'null' => array( null ),
);
}

/**
* Tests that a non-existent option is added even when its pre filter returns a value.
*
* @ticket 59360
*
* @covers ::update_network_option
*/
public function test_update_network_option_with_pre_filter_adds_missing_option() {

if ( is_multisite() ) {
costdev marked this conversation as resolved.
Show resolved Hide resolved
$this->markTestSkipped( 'This test should only run on Single Site.' );
}

// 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_network_option( null, 'foo', 0 ) );
}

/**
* Tests that an existing option is updated even when its pre filter returns the same value.
*
* @ticket 59360
*
* @covers ::update_network_option
*/
public function test_update_network_option_with_pre_filter_updates_option_with_different_value() {

if ( is_multisite() ) {
$this->markTestSkipped( 'This test should only run on Single Site.' );
}

// Add the option with a value of 1 to the database.
update_network_option( null, '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_network_option( null, 'foo', 0 ) );
}

/**
* Tests that calling update_network_option() does not permanently remove pre filters.
*
* @ticket 59360
*
* @covers ::update_network_option
*/
public function test_update_network_option_maintains_pre_filters() {
if ( is_multisite() ) {
$this->markTestSkipped( 'This test should only run on Single Site.' );
}

add_filter( 'pre_option_foo', '__return_zero' );
update_network_option( null, 'foo', 0 );

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

/**
* Tests that a non-existent option is added even when its pre filter returns a value.
*
* @ticket 59360
*
* @covers ::update_network_option
*/
public function test_multisite_update_network_option_with_pre_filter_adds_missing_option() {
if ( ! is_multisite() ) {
$this->markTestSkipped( 'This test should only run on Multisite.' );
}

// Force a return value of integer 0.
add_filter( 'pre_site_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_network_option( null, 'foo', 0 ) );
}

/**
* Tests that an existing option is updated even when its pre filter returns the same value.
*
* @ticket 59360
*
* @covers ::update_network_option
*/
public function test_multisite_update_network_option_with_pre_filter_updates_option_with_different_value() {
if ( ! is_multisite() ) {
$this->markTestSkipped( 'This test should only run on Multisite.' );
}

// Add the option with a value of 1 to the database.
update_network_option( null, 'foo', 1 );

// Force a return value of integer 0.
add_filter( 'pre_site_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_network_option( null, 'foo', 0 ) );
}

/**
* Tests that calling update_network_option() does not permanently remove pre filters.
*
* @ticket 59360
*
* @covers ::update_network_option
*/
public function test_multisite_update_network_option_maintains_pre_filters() {
if ( ! is_multisite() ) {
$this->markTestSkipped( 'This test should only run on Multisite.' );
}

add_filter( 'pre_site_option_foo', '__return_zero' );
update_network_option( null, 'foo', 0 );

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

/**
* Tests that update_network_option() adds a non-existent option that uses a filtered default value.
*
* @ticket 59360
*
* @covers ::update_network_option
*/
public function test_multisite_update_network_option_should_add_option_with_filtered_default_value() {
global $wpdb;

if ( ! is_multisite() ) {
$this->markTestSkipped( 'This test should only run on Multisite.' );
}

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

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

/*
* For a non existing option with the unfiltered default of false, passing false here wouldn't work.
* Because the default is different than false here though, passing false is expected to result in
* a database update.
*/
$this->assertTrue( update_network_option( null, $option, false ), 'update_network_option() should have returned true.' );

$actual = $wpdb->get_row(
$wpdb->prepare(
"SELECT meta_value FROM $wpdb->sitemeta WHERE meta_key = %s LIMIT 1",
$option
)
);

$this->assertIsObject( $actual, 'The option was not added to the database.' );
$this->assertObjectHasProperty( 'meta_value', $actual, 'The "meta_value" property was not included.' );
$this->assertSame( '', $actual->meta_value, 'The new value was not stored in the database.' );
}
}