From b1e26158dc83db456d51ae1af0ebbd026f7cc885 Mon Sep 17 00:00:00 2001 From: costdev Date: Fri, 13 Oct 2023 10:09:41 +0100 Subject: [PATCH 1/6] Introduce and implement `_get_raw_option_value()`. --- src/wp-includes/option.php | 127 ++++++++++++++++++++++--------------- 1 file changed, 77 insertions(+), 50 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 4df0b81458b68..e90bcae8b6d1c 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -776,27 +776,19 @@ 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 ); + /* - * 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}" ) ) { - global $wp_filter; - - $old_filters = $wp_filter[ "pre_option_{$option}" ]; - unset( $wp_filter[ "pre_option_{$option}" ] ); - - $raw_old_value = get_option( $option ); - $wp_filter[ "pre_option_{$option}" ] = $old_filters; - } else { - $raw_old_value = $old_value; + $raw_old_value = $old_value; + // Reviewers: I tried various conditions here. These are the only ones I could get working. + if ( false === $old_value && false !== $default_value ) { + $raw_old_value = _get_raw_option_value( $option, $default_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. * @@ -2144,45 +2136,14 @@ function update_network_option( $network_id, $option, $value ) { */ $value = apply_filters( "pre_update_site_option_{$option}", $value, $old_value, $option, $network_id ); + /** This filter is documented in wp-includes/option.php */ + $default_value = apply_filters( "default_site_option_{$option}", false, $option, $network_id ); + /* - * 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; - - /** This filter is documented in wp-includes/option.php */ - $default_value = apply_filters( "default_site_option_{$option}", false, $option, $network_id ); - - $has_site_filter = has_filter( "pre_site_option_{$option}" ); - $has_option_filter = has_filter( "pre_option_{$option}" ); - if ( $has_site_filter || $has_option_filter ) { - if ( $has_site_filter ) { - $old_ms_filters = $wp_filter[ "pre_site_option_{$option}" ]; - unset( $wp_filter[ "pre_site_option_{$option}" ] ); - } - - if ( $has_option_filter ) { - $old_single_site_filters = $wp_filter[ "pre_option_{$option}" ]; - unset( $wp_filter[ "pre_option_{$option}" ] ); - } - - if ( is_multisite() ) { - $raw_old_value = get_network_option( $network_id, $option ); - } else { - $raw_old_value = get_option( $option, $default_value ); - } - - if ( $has_site_filter ) { - $wp_filter[ "pre_site_option_{$option}" ] = $old_ms_filters; - } - if ( $has_option_filter ) { - $wp_filter[ "pre_option_{$option}" ] = $old_single_site_filters; - } - } else { - $raw_old_value = $old_value; - } + $raw_old_value = false === $old_value && false !== $default_value ? _get_raw_option_value( $option, $default_value, $network_id ) : $old_value; if ( ! is_multisite() ) { /** This filter is documented in wp-includes/option.php */ @@ -3032,3 +2993,69 @@ function _is_equal_database_value( $old_value, $new_value ) { */ return maybe_serialize( $old_value ) === maybe_serialize( $new_value ); } + +/** + * Attempts getting the raw value of an option from the database. + * + * For filtered options and options that do not exist in the database, + * the raw value is the default value. + * + * @since 6.4.0 + * @access private + * + * @param string $option The option's name. + * @param mixed $default_value The option's default value. + * @param int|null $network_id Optional. ID of the network. + * Can be null to default to the current network ID. + * Default null. + * @return mixed The raw value. + */ +function _get_raw_option_value( $option, $default_value, $network_id = null ) { + global $wpdb; + + // Do not check filtered option values. + if ( ! ( false === has_filter( "pre_option_{$option}" ) || false === has_filter( 'pre_option' ) ) ) { + return $default_value; + } + + /* + * Local options take precedence, even on Multisite. + * For reviewers, see Tests_L10n_GetLocale::test_local_option_should_take_precedence_on_multisite + */ + $from_db = $wpdb->get_row( + $wpdb->prepare( "SELECT option_value FROM $wpdb->options WHERE option_name = %s LIMIT 1", $option ) + ); + + if ( isset( $from_db->option_value ) ) { + return $from_db->option_value; + } + + if ( is_multisite() ) { + // Do not check filtered option values. + if ( false !== has_filter( "pre_site_option_{$option}" ) ) { + return $default_value; + } + + $network_id = (int) $network_id; + + // Fallback to the current network if a network ID is not specified. + if ( ! $network_id ) { + $network_id = get_current_network_id(); + } + + $from_db = $wpdb->get_row( + $wpdb->prepare( + "SELECT meta_value FROM $wpdb->sitemeta WHERE meta_key = %s AND site_id = %i LIMIT 1", + $option, + $network_id + ) + ); + + if ( isset( $from_db->meta_value ) ) { + return $from_db->meta_value; + } + } + + // There is no value in the database. Return the default value. + return $default_value; +} From ffa3c89d73fb12856d56d56341afae90577de39a Mon Sep 17 00:00:00 2001 From: costdev Date: Fri, 13 Oct 2023 10:10:23 +0100 Subject: [PATCH 2/6] Update Single Site pre-filter tests. --- tests/phpunit/tests/option/option.php | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/tests/phpunit/tests/option/option.php b/tests/phpunit/tests/option/option.php index 04b89c44296d8..c6d50848cbd16 100644 --- a/tests/phpunit/tests/option/option.php +++ b/tests/phpunit/tests/option/option.php @@ -725,42 +725,34 @@ static function () use ( $default_value ) { } /** - * Tests that a non-existent option is added even when its pre filter returns a value. + * Tests that a non-existent option is not added when its pre filter returns the same value. * * @ticket 22192 * * @covers ::update_option */ - public function test_update_option_with_pre_filter_adds_missing_option() { + public function test_update_option_with_pre_filter_should_not_add_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 ) ); + $this->assertFalse( update_option( 'foo', 0 ) ); } /** - * Tests that an existing option is updated even when its pre filter returns the same value. + * Tests that an existing option is not 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() { + public function test_update_option_with_pre_filter_should_not_update_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 ) ); + $this->assertFalse( update_option( 'foo', 0 ) ); } /** From 584ff529e215564142931a6bc15268b0402f0cc1 Mon Sep 17 00:00:00 2001 From: costdev Date: Fri, 13 Oct 2023 10:10:37 +0100 Subject: [PATCH 3/6] Update Multisite pre-filter tests. --- tests/phpunit/tests/option/networkOption.php | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index 2b0bed6504b5b..87fc50a4adcc2 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -690,33 +690,29 @@ public function data_stored_as_empty_string() { } /** - * Tests that a non-existent option is added even when its pre filter returns a value. + * Tests that a non-existent option is not 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() { + public function test_update_network_option_with_pre_filter_should_not_add_missing_option() { $hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo'; // Force a return value of integer 0. add_filter( $hook_name, '__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 ) ); + $this->assertFalse( update_network_option( null, 'foo', 0 ) ); } /** - * Tests that an existing option is updated even when its pre filter returns the same value. + * Tests that an existing option is not 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() { + public function test_update_network_option_with_pre_filter_should_not_update_option_with_different_value() { $hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo'; // Add the option with a value of 1 to the database. @@ -725,11 +721,7 @@ public function test_update_network_option_with_pre_filter_updates_option_with_d // Force a return value of integer 0. add_filter( $hook_name, '__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 ) ); + $this->assertFalse( update_network_option( null, 'foo', 0 ) ); } /** From f30f5e1bddc445328ac6d6a143eb3d02181c43dd Mon Sep 17 00:00:00 2001 From: costdev Date: Sat, 14 Oct 2023 01:02:29 +0100 Subject: [PATCH 4/6] Add Joe's tests. --- tests/phpunit/tests/option/option.php | 107 ++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/tests/phpunit/tests/option/option.php b/tests/phpunit/tests/option/option.php index c6d50848cbd16..400b73fe9e982 100644 --- a/tests/phpunit/tests/option/option.php +++ b/tests/phpunit/tests/option/option.php @@ -801,4 +801,111 @@ public function test_update_option_with_autoload_change_yes_to_no() { delete_option( 'foo' ); $this->assertFalse( get_option( 'foo' ) ); } + + /** + * Test cases for testing whether update_option() will add a non-existent option. + */ + public function data_option_values() { + return array( + array( '1' ), + array( 1 ), + array( 1.0 ), + array( true ), + array( 'true' ), + array( '0' ), + array( 0 ), + array( 0.0 ), + array( false ), + array( '' ), + array( null ), + array( array() ), + ); + } + + /** + * Tests that a non-existent option is added only when the pre-filter matches the default 'false'. + * + * @ticket 22192 + * @dataProvider data_option_values + * + * @covers ::update_option + */ + public function test_update_option_with_false_pre_filter_adds_missing_option( $option ) { + // Filter the old option value to `false`. + add_filter( 'pre_option_foo', '__return_false' ); + + /* + * When the option is equal to the filtered version, update option will bail early. + * Otherwise, The pre-filter will make the old option `false`, which is equal to the + * default value. This causes an add_option() to be triggered. + */ + if ( false === $option ) { + $this->assertFalse( update_option( 'foo', $option ) ); + } else { + $this->assertTrue( update_option( 'foo', $option ) ); + } + } + + /** + * Tests that a non-existent option is never added when the pre-filter is not 'false'. + * + * @ticket 22192 + * @dataProvider data_option_values + * + * @covers ::update_option + */ + public function test_update_option_with_truthy_pre_filter_does_not_add_missing_option( $option ) { + // Filter the old option value to `true`. + add_filter( 'pre_option_foo', '__return_true' ); + + $this->assertFalse( update_option( 'foo', $option ) ); + } + + /** + * Tests that an existing option is updated even when its pre filter returns the same value. + * + * @ticket 22192 + * @dataProvider data_option_values + * + * @covers ::update_option + */ + public function test_update_option_with_false_pre_filter_updates_option( $option ) { + // Add the option with a value that is different than any updated. + add_option( 'foo', 'bar' ); + + // Force a return value of false. + add_filter( 'pre_option_foo', '__return_false' ); + + /* + * This should succeed, since the 'foo' option has a value of 0 in the database. + * Therefore it differs from true and should be updated. + */ + $this->assertTrue( update_option( 'foo', $option ) ); + } + + /** + * Tests that an existing option is updated even when its pre filter returns the same value. + * + * @ticket 22192 + * @dataProvider data_option_values + * + * @covers ::update_option + */ + public function test_update_option_with_true_pre_filter_updates_option( $option ) { + // Add the option with a value that is different than any updated. + add_option( 'foo', 'bar' ); + + // Force a return value of true. + add_filter( 'pre_option_foo', '__return_true' ); + + /* + * Option `true` is the same as the filtered old value, so it will fail. + * All other options should update. + */ + if ( true === $option ) { + $this->assertFalse( update_option( 'foo', $option ) ); + } else { + $this->assertTrue( update_option( 'foo', $option ) ); + } + } } From 945a2dfe71fa74899aebfc0cc6a1b186fe6bf803 Mon Sep 17 00:00:00 2001 From: costdev Date: Sat, 14 Oct 2023 01:03:40 +0100 Subject: [PATCH 5/6] Skip equal check for filtered option values. --- src/wp-includes/option.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index e90bcae8b6d1c..edc4c95a2f244 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -800,6 +800,9 @@ function update_option( $option, $value, $autoload = null ) { if ( $value === $raw_old_value || ( + // Do not check filtered option values. + false === has_filter( "pre_option_{$option}" ) && + false === has_filter( 'pre_option' ) && $raw_old_value !== $default_value && _is_equal_database_value( $raw_old_value, $value ) ) From f5fbdecb1a891037aa0dbe9a57b6f29c53427550 Mon Sep 17 00:00:00 2001 From: costdev Date: Fri, 13 Oct 2023 11:10:46 +0100 Subject: [PATCH 6/6] Yoast BC fix (don't commit with this in the PR) --- src/wp-includes/option.php | 2 +- tests/phpunit/tests/option/networkOption.php | 50 ++++++-------------- 2 files changed, 15 insertions(+), 37 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index edc4c95a2f244..6ec636b0fc068 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2181,7 +2181,7 @@ function update_network_option( $network_id, $option, $value ) { return false; } - if ( $default_value === $raw_old_value ) { + if ( false === $old_value ) { return add_network_option( $network_id, $option, $value ); } diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index 87fc50a4adcc2..9f6f860c39e92 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -795,9 +795,12 @@ public function test_update_network_option_should_conditionally_apply_site_and_o public function test_update_network_option_should_add_option_with_filtered_default_value() { global $wpdb; - $option = 'foo'; - $default_site_value = 'default-site-value'; - $default_option_value = 'default-option-value'; + if ( ! is_multisite() ) { + $this->markTestSkipped(); + } + + $option = 'foo'; + $default_site_value = 'default-site-value'; add_filter( "default_site_option_{$option}", @@ -806,40 +809,15 @@ static function () use ( $default_site_value ) { } ); - add_filter( - "default_option_{$option}", - static function () use ( $default_option_value ) { - return $default_option_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.' ); - - if ( is_multisite() ) { - $actual = $wpdb->get_row( - $wpdb->prepare( - "SELECT meta_value FROM $wpdb->sitemeta WHERE meta_key = %s LIMIT 1", - $option - ) - ); - } else { - $actual = $wpdb->get_row( - $wpdb->prepare( - "SELECT option_value FROM $wpdb->options WHERE option_name = %s LIMIT 1", - $option - ) - ); - } + $this->assertFalse( update_network_option( null, $option, false ), 'update_network_option() should have returned false.' ); - $value_field = is_multisite() ? 'meta_value' : 'option_value'; + $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( $value_field, $actual, "The '$value_field' property was not included." ); - $this->assertSame( '', $actual->$value_field, 'The new value was not stored in the database.' ); + $this->assertNull( $actual, 'The option was added to the database.' ); } }