From 8febe8393a05505c5825f9341d43d9bc9cb27880 Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 10:37:39 +0530 Subject: [PATCH 01/22] Update production code --- src/wp-includes/option.php | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index d2ffa675b1854..67b12673ae576 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2147,13 +2147,18 @@ function update_network_option( $network_id, $option, $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. + * 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 === $old_value || + ( + false !== $old_value && + _is_equal_database_value( $old_value, $value ) + ) + ) { return false; } From 1eccc726bdbf39b235a0783a6d4facf427e2e2ea Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 10:59:03 +0530 Subject: [PATCH 02/22] Update unit tests --- tests/phpunit/tests/option/networkOption.php | 54 ++++++-------------- 1 file changed, 15 insertions(+), 39 deletions(-) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index 7ae19abbeeaee..0e084d0484aaf 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -407,9 +407,7 @@ public function test_update_option_should_not_update_some_values_from_cache( $ol // 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.' ); } @@ -446,9 +444,7 @@ public function test_update_network_option_should_not_update_some_values_from_db $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.' ); } @@ -480,9 +476,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.' ); } @@ -567,8 +561,8 @@ public function data_strictly_equal_values() { * On Single Site, this will result in no additional queries as * the option_value database field is not nullable. * - * On Multisite, this will result in one additional query as - * the meta_value database field is nullable. + * On Multisite, this will result in no additional query because + * of early return. * * @ticket 59360 * @@ -582,14 +576,8 @@ public function test_update_option_should_handle_a_null_new_value_from_cache() { // Comparison will happen against value cached during add_option() above. $updated = update_network_option( null, 'foo', null ); - $expected_queries = is_multisite() ? 1 : 0; - $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." ); - - if ( is_multisite() ) { - $this->assertTrue( $updated, 'update_network_option() should have returned true.' ); - } else { - $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); - } + $this->assertSame( $num_queries, get_num_queries(), 'No additional queries should have run.' ); + $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); } /** @@ -599,8 +587,8 @@ public function test_update_option_should_handle_a_null_new_value_from_cache() { * On Single Site, this will result in only 1 additional query as * the option_value database field is not nullable. * - * On Multisite, this will result in two additional queries as - * the meta_value database field is nullable. + * On Multisite, this will result in only 1 additional queries as + * the meta_value database field is not nullable. * * @ticket 59360 * @@ -618,14 +606,8 @@ public function test_update_option_should_handle_a_null_new_value_from_db() { $updated = update_network_option( null, 'foo', null ); - $expected_queries = is_multisite() ? 2 : 1; - $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." ); - - if ( is_multisite() ) { - $this->assertTrue( $updated, 'update_network_option() should have returned true.' ); - } else { - $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); - } + $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.' ); } /** @@ -635,8 +617,8 @@ public function test_update_option_should_handle_a_null_new_value_from_db() { * On Single Site, this will result in no additional queries as * the option_value database field is not nullable. * - * On Multisite, this will result in one additional query as - * the meta_value database field is nullable. + * On Multisite, this will result in no additional query as + * the meta_value database field is not nullable. * * @ticket 59360 * @@ -652,14 +634,8 @@ public function test_update_option_should_handle_a_null_new_value_from_refreshed $num_queries = get_num_queries(); $updated = update_network_option( null, 'foo', null ); - $expected_queries = is_multisite() ? 1 : 0; - $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." ); - - if ( is_multisite() ) { - $this->assertTrue( $updated, 'update_network_option() should have returned true.' ); - } else { - $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); - } + $this->assertSame( $num_queries, get_num_queries(), 'No additional queries should have run.' ); + $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); } /** From feb21d67ded91e4af3455b5a3f13c66bbeb3216d Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 12:15:38 +0530 Subject: [PATCH 03/22] Update code for nullable check --- src/wp-includes/option.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 67b12673ae576..cc33117d901cd 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2144,6 +2144,7 @@ function update_network_option( $network_id, $option, $value ) { */ $value = apply_filters( "pre_update_site_option_{$option}", $value, $old_value, $option, $network_id ); + $is_multisite = is_multisite(); /* * If the new and old values are the same, no need to update. * @@ -2156,6 +2157,11 @@ function update_network_option( $network_id, $option, $value ) { $value === $old_value || ( false !== $old_value && + /* + * Multisite uses the `meta_value` database field, which is nullable. + * Don't check for values that are equal to `null`. + */ + ( ! $is_multisite || ( null !== $old_value && null !== $value ) ) && _is_equal_database_value( $old_value, $value ) ) ) { @@ -2174,7 +2180,7 @@ function update_network_option( $network_id, $option, $value ) { wp_cache_set( $notoptions_key, $notoptions, 'site-options' ); } - if ( ! is_multisite() ) { + if ( ! $is_multisite ) { $result = update_option( $option, $value, 'no' ); } else { $value = sanitize_option( $option, $value ); From daa3f24dcf06d4428f368abf2f57a1eb7212a41f Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 12:18:02 +0530 Subject: [PATCH 04/22] Revert changes for nullable values --- tests/phpunit/tests/option/networkOption.php | 42 ++++++++++++++------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index 0e084d0484aaf..dbef370a7168a 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -561,8 +561,8 @@ public function data_strictly_equal_values() { * On Single Site, this will result in no additional queries as * the option_value database field is not nullable. * - * On Multisite, this will result in no additional query because - * of early return. + * On Multisite, this will result in one additional query as + * the meta_value database field is nullable. * * @ticket 59360 * @@ -576,8 +576,14 @@ public function test_update_option_should_handle_a_null_new_value_from_cache() { // Comparison will happen against value cached during add_option() above. $updated = update_network_option( null, 'foo', null ); - $this->assertSame( $num_queries, get_num_queries(), 'No additional queries should have run.' ); - $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); + $expected_queries = is_multisite() ? 1 : 0; + $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." ); + + if ( is_multisite() ) { + $this->assertTrue( $updated, 'update_network_option() should have returned true.' ); + } else { + $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); + } } /** @@ -587,8 +593,8 @@ public function test_update_option_should_handle_a_null_new_value_from_cache() { * On Single Site, this will result in only 1 additional query as * the option_value database field is not nullable. * - * On Multisite, this will result in only 1 additional queries as - * the meta_value database field is not nullable. + * On Multisite, this will result in two additional queries as + * the meta_value database field is nullable. * * @ticket 59360 * @@ -606,8 +612,14 @@ public function test_update_option_should_handle_a_null_new_value_from_db() { $updated = update_network_option( null, 'foo', null ); - $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.' ); + $expected_queries = is_multisite() ? 2 : 1; + $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." ); + + if ( is_multisite() ) { + $this->assertTrue( $updated, 'update_network_option() should have returned true.' ); + } else { + $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); + } } /** @@ -617,8 +629,8 @@ public function test_update_option_should_handle_a_null_new_value_from_db() { * On Single Site, this will result in no additional queries as * the option_value database field is not nullable. * - * On Multisite, this will result in no additional query as - * the meta_value database field is not nullable. + * On Multisite, this will result in one additional query as + * the meta_value database field is nullable. * * @ticket 59360 * @@ -634,8 +646,14 @@ public function test_update_option_should_handle_a_null_new_value_from_refreshed $num_queries = get_num_queries(); $updated = update_network_option( null, 'foo', null ); - $this->assertSame( $num_queries, get_num_queries(), 'No additional queries should have run.' ); - $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); + $expected_queries = is_multisite() ? 1 : 0; + $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." ); + + if ( is_multisite() ) { + $this->assertTrue( $updated, 'update_network_option() should have returned true.' ); + } else { + $this->assertFalse( $updated, 'update_network_option() should have returned false.' ); + } } /** From 2a1d706a08ade2de7eb7528b80cbf454cb74627e Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 12:55:14 +0530 Subject: [PATCH 05/22] Remove `is_scalar` check from unit tests --- tests/phpunit/tests/option/networkOption.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index dbef370a7168a..c190f14f9aed0 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -439,11 +439,6 @@ 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; - } - $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.' ); } From 2671d63d407d780458741daccfd2122e4dad29ff Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 17:51:10 +0530 Subject: [PATCH 06/22] Add check for `pre_site_option_` --- src/wp-includes/option.php | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index cc33117d901cd..4e68e01001a47 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2145,6 +2145,25 @@ function update_network_option( $network_id, $option, $value ) { $value = apply_filters( "pre_update_site_option_{$option}", $value, $old_value, $option, $network_id ); $is_multisite = is_multisite(); + + /* + * 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_site_option_{$option}" ) ) { + global $wp_filter; + + $old_filters = $wp_filter[ "pre_site_option_{$option}" ]; + unset( $wp_filter[ "pre_site_option_{$option}" ] ); + + $raw_old_value = get_network_option( $network_id, $option, false ); + $wp_filter[ "pre_site_option_{$option}" ] = $old_filters; + } else { + $raw_old_value = $old_value; + } + /* * If the new and old values are the same, no need to update. * @@ -2154,21 +2173,18 @@ function update_network_option( $network_id, $option, $value ) { * 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 || + $value === $raw_old_value || ( - false !== $old_value && - /* - * Multisite uses the `meta_value` database field, which is nullable. - * Don't check for values that are equal to `null`. - */ - ( ! $is_multisite || ( null !== $old_value && null !== $value ) ) && - _is_equal_database_value( $old_value, $value ) + false !== $raw_old_value && + // Site meta values are nullable. Don't check if the values are loosely equal to null. + ( ! $is_multisite || ( null !== $raw_old_value && null !== $value ) ) && + _is_equal_database_value( $raw_old_value, $value ) ) ) { return false; } - if ( false === $old_value ) { + if ( false === $raw_old_value ) { return add_network_option( $network_id, $option, $value ); } From cedb6543d907212e6eef565f185c49d0d35a5fdb Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 18:06:14 +0530 Subject: [PATCH 07/22] Add unit tests for pre filter check --- tests/phpunit/tests/option/networkOption.php | 54 ++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index c190f14f9aed0..7541373ddcbcf 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -688,4 +688,58 @@ 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() { + // 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_update_network_option_with_pre_filter_updates_option_with_different_value() { + // 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_update_network_option_maintains_pre_filters() { + 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' ) ); + } } From 5b42a03b0c539b4ada1cb51bbda75a373b044d33 Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 20:59:52 +0530 Subject: [PATCH 08/22] Use is_multisite() instead of variable --- src/wp-includes/option.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 4e68e01001a47..8db6c6c2bf691 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2144,8 +2144,6 @@ function update_network_option( $network_id, $option, $value ) { */ $value = apply_filters( "pre_update_site_option_{$option}", $value, $old_value, $option, $network_id ); - $is_multisite = is_multisite(); - /* * 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. @@ -2177,7 +2175,7 @@ function update_network_option( $network_id, $option, $value ) { ( false !== $raw_old_value && // Site meta values are nullable. Don't check if the values are loosely equal to null. - ( ! $is_multisite || ( null !== $raw_old_value && null !== $value ) ) && + ( ! is_multisite() || ( null !== $raw_old_value && null !== $value ) ) && _is_equal_database_value( $raw_old_value, $value ) ) ) { @@ -2196,7 +2194,7 @@ function update_network_option( $network_id, $option, $value ) { wp_cache_set( $notoptions_key, $notoptions, 'site-options' ); } - if ( ! $is_multisite ) { + if ( ! is_multisite() ) { $result = update_option( $option, $value, 'no' ); } else { $value = sanitize_option( $option, $value ); From 5c81311331443305b77cdd2f7574e6ab669872d6 Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 21:58:06 +0530 Subject: [PATCH 09/22] Check for `pre_option_` for single site --- src/wp-includes/option.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 8db6c6c2bf691..4196db331a12e 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2150,14 +2150,14 @@ function update_network_option( $network_id, $option, $value ) { * 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_site_option_{$option}" ) ) { - global $wp_filter; - - $old_filters = $wp_filter[ "pre_site_option_{$option}" ]; - unset( $wp_filter[ "pre_site_option_{$option}" ] ); - - $raw_old_value = get_network_option( $network_id, $option, false ); - $wp_filter[ "pre_site_option_{$option}" ] = $old_filters; + global $wp_filter; + + $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 ); + $wp_filter[ $pre_option_hook ] = $old_filters; } else { $raw_old_value = $old_value; } From 11ef49ffb183641e071ebf2c4496a48863e66f40 Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 22:07:31 +0530 Subject: [PATCH 10/22] Address review feedback --- src/wp-includes/option.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 4196db331a12e..9da1e82c30803 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -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. @@ -2156,7 +2156,7 @@ function update_network_option( $network_id, $option, $value ) { 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 ); + $raw_old_value = get_network_option( $network_id, $option ); $wp_filter[ $pre_option_hook ] = $old_filters; } else { $raw_old_value = $old_value; From c4ac154c01fb7e201d60b8954700fd9423b4604e Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 22:36:39 +0530 Subject: [PATCH 11/22] Apply suggestions from code review Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com> --- src/wp-includes/option.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 9da1e82c30803..784736f89290a 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2156,7 +2156,9 @@ function update_network_option( $network_id, $option, $value ) { if ( has_filter( $pre_option_hook ) ) { $old_filters = $wp_filter[ $pre_option_hook ]; unset( $wp_filter[ $pre_option_hook ] ); - $raw_old_value = get_network_option( $network_id, $option ); + /** This filter is documented in wp-includes/options.php */ + $default_value = apply_filters( 'default_site_option_' . $option, false, $option, $network_id ); + $raw_old_value = is_multisite() ? get_network_option( $network_id, $option ) : get_option( $option, $default_value ); $wp_filter[ $pre_option_hook ] = $old_filters; } else { $raw_old_value = $old_value; From 8df882e8d3521fda9e10524090c51349baa8fef3 Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Mon, 9 Oct 2023 23:41:03 +0530 Subject: [PATCH 12/22] Add changes for default value and update unit test for pre_filter --- src/wp-includes/option.php | 11 +- tests/phpunit/tests/option/networkOption.php | 135 ++++++++++++++++++- 2 files changed, 136 insertions(+), 10 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 784736f89290a..9502b6e321805 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2152,13 +2152,16 @@ function update_network_option( $network_id, $option, $value ) { */ global $wp_filter; - $pre_option_hook = is_multisite() ? "pre_site_option_{$option}" : "pre_option_{$option}"; + $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 ] ); - /** This filter is documented in wp-includes/options.php */ - $default_value = apply_filters( 'default_site_option_' . $option, false, $option, $network_id ); + $raw_old_value = is_multisite() ? get_network_option( $network_id, $option ) : get_option( $option, $default_value ); + $wp_filter[ $pre_option_hook ] = $old_filters; } else { $raw_old_value = $old_value; @@ -2184,7 +2187,7 @@ function update_network_option( $network_id, $option, $value ) { return false; } - if ( false === $raw_old_value ) { + if ( $default_value === $raw_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 7541373ddcbcf..dd69201e77ba7 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -399,7 +399,7 @@ 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(); @@ -563,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(); @@ -595,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(); @@ -631,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. @@ -697,8 +697,13 @@ public function data_stored_as_empty_string() { * @covers ::update_network_option */ public function test_update_network_option_with_pre_filter_adds_missing_option() { + + if ( is_multisite() ) { + $this->markTestSkipped( 'This test should only run on Single Site.' ); + } + // Force a return value of integer 0. - add_filter( 'pre_site_option_foo', '__return_zero' ); + add_filter( 'pre_option_foo', '__return_zero' ); /* * This should succeed, since the 'foo' option does not exist in the database. @@ -715,11 +720,16 @@ public function test_update_network_option_with_pre_filter_adds_missing_option() * @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_site_option_foo', '__return_zero' ); + add_filter( 'pre_option_foo', '__return_zero' ); /* * This should succeed, since the 'foo' option has a value of 1 in the database. @@ -736,10 +746,123 @@ public function test_update_network_option_with_pre_filter_updates_option_with_d * @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.' ); + } } From 9a2dccdae048c028d6503bf5fd8a4c7b9264e926 Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Tue, 10 Oct 2023 00:45:42 +0530 Subject: [PATCH 13/22] Address review feedback --- src/wp-includes/option.php | 26 +++-- tests/phpunit/tests/option/networkOption.php | 102 +++++++++++++++++++ 2 files changed, 121 insertions(+), 7 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 9502b6e321805..190b83e631309 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2152,17 +2152,29 @@ function update_network_option( $network_id, $option, $value ) { */ 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}"; + $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_filter( $pre_option_hook ) ) { - $old_filters = $wp_filter[ $pre_option_hook ]; - unset( $wp_filter[ $pre_option_hook ] ); + if ( $has_option_filter ) { + $old_single_site_filters = $wp_filter[ "pre_option_{$option}" ]; + unset( $wp_filter[ "pre_option_{$option}" ] ); + } $raw_old_value = is_multisite() ? get_network_option( $network_id, $option ) : get_option( $option, $default_value ); - $wp_filter[ $pre_option_hook ] = $old_filters; + 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; } diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index dd69201e77ba7..fc66b943ba6da 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -823,6 +823,108 @@ public function test_multisite_update_network_option_maintains_pre_filters() { $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 site value and a filtered default option value in Single Site. + * + * @ticket 59360 + * + * @covers ::update_network_option + */ + public function test_update_network_option_should_add_option_with_filtered_default_value() { + global $wpdb; + + if ( is_multisite() ) { + $this->markTestSkipped( 'This test should only run on Single Site.' ); + } + + $option = 'foo'; + $default_site_value = 'default-site-value'; + $default_option_value = 'default-option-value'; + + add_filter( + "default_site_option_{$option}", + static function () use ( $default_site_value ) { + return $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.' ); + + $actual = $wpdb->get_row( + $wpdb->prepare( + "SELECT option_value FROM $wpdb->options WHERE option_name = %s LIMIT 1", + $option + ) + ); + + $this->assertIsObject( $actual, 'The option was not added to the database.' ); + $this->assertObjectHasProperty( 'option_value', $actual, 'The "option_value" property was not included.' ); + $this->assertSame( '', $actual->option_value, 'The new value was not stored in the database.' ); + } + + /** + * Tests that update_network_option() applies site and option default value filters on Single Site. + * + * @ticket 59360 + * + * @covers ::update_network_option + */ + public function test_update_network_option_should_apply_site_and_option_default_value_filters_on_single_site() { + if ( is_multisite() ) { + $this->markTestSkipped( 'This test should only run on Single Site.' ); + } + + $option = 'foo'; + $site_hook = new MockAction(); + $option_hook = new MockAction(); + + add_filter( "default_site_option_{$option}", array( $site_hook, 'filter' ) ); + add_filter( "default_option_{$option}", array( $option_hook, 'filter' ) ); + + update_network_option( null, $option, 'false' ); + + $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters were not applied." ); + $this->assertSame( 1, $option_hook->get_call_count(), "'default_option_{$option}' filters were not applied." ); + } + + /** + * Tests that update_network_option() applies only site option default value filters on Multisite. + * + * @ticket 59360 + * + * @covers ::update_network_option + */ + public function test_update_network_option_should_apply_only_site_default_value_filters_on_multisite() { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'This test should only run on Multisite.' ); + } + + $option = 'foo'; + $site_hook = new MockAction(); + $option_hook = new MockAction(); + + add_filter( "default_site_option_{$option}", array( $site_hook, 'filter' ) ); + add_filter( "default_option_{$option}", array( $option_hook, 'filter' ) ); + + update_network_option( null, $option, 'false' ); + + $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters were not applied." ); + $this->assertSame( 0, $option_hook->get_call_count(), "'default_option_{$option}' filters were not applied." ); + } + /** * Tests that update_network_option() adds a non-existent option that uses a filtered default value. * From f44a845fccd942812bff1e3fd248178a1b6b9ebe Mon Sep 17 00:00:00 2001 From: Mukesh Panchal Date: Tue, 10 Oct 2023 01:01:30 +0530 Subject: [PATCH 14/22] Added additional tests for pre_ filter --- tests/phpunit/tests/option/networkOption.php | 50 ++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index fc66b943ba6da..e988bf178bc0f 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -925,6 +925,56 @@ public function test_update_network_option_should_apply_only_site_default_value_ $this->assertSame( 0, $option_hook->get_call_count(), "'default_option_{$option}' filters were not applied." ); } + /** + * Tests that update_network_option() applies 'pre_site_option_{$option}' and 'pre_option_{$option}' filters on Single Site. + * + * @ticket 59360 + * + * @covers ::update_network_option + */ + public function test_update_network_option_should_apply_pre_site_option_and_pre_option_filters_on_single_site() { + if ( is_multisite() ) { + $this->markTestSkipped( 'This test should only run on Single Site.' ); + } + + $option = 'foo'; + $site_hook = new MockAction(); + $option_hook = new MockAction(); + + add_filter( "pre_site_option_{$option}", array( $site_hook, 'filter' ) ); + add_filter( "pre_option_{$option}", array( $option_hook, 'filter' ) ); + + update_network_option( null, $option, 'false' ); + + $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters were not applied." ); + $this->assertSame( 1, $option_hook->get_call_count(), "'pre_option_{$option}' filters were not applied." ); + } + + /** + * Tests that update_network_option() applies only 'pre_site_option_{$option}' filters on Multisite. + * + * @ticket 59360 + * + * @covers ::update_network_option + */ + public function test_update_network_option_should_apply_only_pre_site_option_filters_on_multisite() { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'This test should only run on Multisite.' ); + } + + $option = 'foo'; + $site_hook = new MockAction(); + $option_hook = new MockAction(); + + add_filter( "pre_site_option_{$option}", array( $site_hook, 'filter' ) ); + add_filter( "pre_option_{$option}", array( $option_hook, 'filter' ) ); + + update_network_option( null, $option, 'false' ); + + $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters were not applied." ); + $this->assertSame( 0, $option_hook->get_call_count(), "'pre_option_{$option}' filters were not applied." ); + } + /** * Tests that update_network_option() adds a non-existent option that uses a filtered default value. * From 9b67e57e8d01a0ea673a807ff1dcd97835bb97ef Mon Sep 17 00:00:00 2001 From: costdev Date: Mon, 9 Oct 2023 21:13:37 +0100 Subject: [PATCH 15/22] Ensure `default_value` is set in Single Site. --- src/wp-includes/option.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 190b83e631309..04519294d92a3 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2152,6 +2152,7 @@ function update_network_option( $network_id, $option, $value ) { */ 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}" ); @@ -2167,7 +2168,13 @@ function update_network_option( $network_id, $option, $value ) { unset( $wp_filter[ "pre_option_{$option}" ] ); } - $raw_old_value = is_multisite() ? get_network_option( $network_id, $option ) : get_option( $option, $default_value ); + if ( is_multisite() ) { + $raw_old_value = get_network_option( $network_id, $option ); + } else { + $raw_old_value = get_option( $option, $default_value ); + /** This filter is documented in wp-includes/option.php */ + $default_value = apply_filters( "default_option_{$option}", $default_value, $option, true ); + } if ( $has_site_filter ) { $wp_filter[ "pre_site_option_{$option}" ] = $old_ms_filters; @@ -2177,6 +2184,10 @@ function update_network_option( $network_id, $option, $value ) { } } else { $raw_old_value = $old_value; + if ( ! is_multisite() ) { + /** This filter is documented in wp-includes/option.php */ + $default_value = apply_filters( "default_option_{$option}", $default_value, $option, true ); + } } /* From 20ba3a611be862606b6f34f478876f28363f56fb Mon Sep 17 00:00:00 2001 From: costdev Date: Mon, 9 Oct 2023 21:14:17 +0100 Subject: [PATCH 16/22] Update call count `default_option_{$option}`. --- tests/phpunit/tests/option/networkOption.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index e988bf178bc0f..c3364456c99e7 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -897,7 +897,7 @@ public function test_update_network_option_should_apply_site_and_option_default_ update_network_option( null, $option, 'false' ); $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters were not applied." ); - $this->assertSame( 1, $option_hook->get_call_count(), "'default_option_{$option}' filters were not applied." ); + $this->assertSame( 2, $option_hook->get_call_count(), "'default_option_{$option}' filters were not applied." ); } /** From f204d20d9fa2fc300f63a8fbf8769ab10f545b40 Mon Sep 17 00:00:00 2001 From: costdev Date: Mon, 9 Oct 2023 21:19:31 +0100 Subject: [PATCH 17/22] Tests: Improve `$message` parameters. --- tests/phpunit/tests/option/networkOption.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index c3364456c99e7..c19e625554a3c 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -896,8 +896,8 @@ public function test_update_network_option_should_apply_site_and_option_default_ update_network_option( null, $option, 'false' ); - $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters were not applied." ); - $this->assertSame( 2, $option_hook->get_call_count(), "'default_option_{$option}' filters were not applied." ); + $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters should have been applied twice." ); + $this->assertSame( 2, $option_hook->get_call_count(), "'default_option_{$option}' filters should have been applied twice." ); } /** @@ -921,8 +921,8 @@ public function test_update_network_option_should_apply_only_site_default_value_ update_network_option( null, $option, 'false' ); - $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters were not applied." ); - $this->assertSame( 0, $option_hook->get_call_count(), "'default_option_{$option}' filters were not applied." ); + $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters should have been applied twice." ); + $this->assertSame( 0, $option_hook->get_call_count(), "'default_option_{$option}' filters should not have been applied." ); } /** @@ -946,8 +946,8 @@ public function test_update_network_option_should_apply_pre_site_option_and_pre_ update_network_option( null, $option, 'false' ); - $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters were not applied." ); - $this->assertSame( 1, $option_hook->get_call_count(), "'pre_option_{$option}' filters were not applied." ); + $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters should have been applied once." ); + $this->assertSame( 1, $option_hook->get_call_count(), "'pre_option_{$option}' filters should have been applied once." ); } /** @@ -971,8 +971,8 @@ public function test_update_network_option_should_apply_only_pre_site_option_fil update_network_option( null, $option, 'false' ); - $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters were not applied." ); - $this->assertSame( 0, $option_hook->get_call_count(), "'pre_option_{$option}' filters were not applied." ); + $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters should have been applied once." ); + $this->assertSame( 0, $option_hook->get_call_count(), "'pre_option_{$option}' filters should not have been applied." ); } /** From 4ad219bb0d40bbb021a661e8d969289367a3c593 Mon Sep 17 00:00:00 2001 From: costdev Date: Mon, 9 Oct 2023 22:04:38 +0100 Subject: [PATCH 18/22] Separate `$default_value` from `pre_` filters. --- src/wp-includes/option.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 04519294d92a3..251e01087231a 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2172,8 +2172,6 @@ function update_network_option( $network_id, $option, $value ) { $raw_old_value = get_network_option( $network_id, $option ); } else { $raw_old_value = get_option( $option, $default_value ); - /** This filter is documented in wp-includes/option.php */ - $default_value = apply_filters( "default_option_{$option}", $default_value, $option, true ); } if ( $has_site_filter ) { @@ -2184,10 +2182,11 @@ function update_network_option( $network_id, $option, $value ) { } } else { $raw_old_value = $old_value; - if ( ! is_multisite() ) { - /** This filter is documented in wp-includes/option.php */ - $default_value = apply_filters( "default_option_{$option}", $default_value, $option, true ); - } + } + + if ( ! is_multisite() ) { + /** This filter is documented in wp-includes/option.php */ + $default_value = apply_filters( "default_option_{$option}", $default_value, $option, true ); } /* From f7730c8cb8d5b7c1a5185c10a3cdba1d5ea7b6c8 Mon Sep 17 00:00:00 2001 From: costdev Date: Mon, 9 Oct 2023 22:38:12 +0100 Subject: [PATCH 19/22] Improve inline documentation for `null`. --- src/wp-includes/option.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 251e01087231a..eeea2c81b10b5 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2201,8 +2201,15 @@ function update_network_option( $network_id, $option, $value ) { $value === $raw_old_value || ( false !== $raw_old_value && - // Site meta values are nullable. Don't check if the values are loosely equal to null. - ( ! is_multisite() || ( null !== $raw_old_value && null !== $value ) ) && + /* + * Single site stores values in the `option_value` field, which cannot be set to NULL. + * This means a PHP `null` value will be cast to an empty string, which can be considered + * equal to values such as an empty string, or false when cast to string. + * + * However, Multisite stores values in the `meta_value` field, which can be set to NULL. + * As NULL is unique in the database, skip checking an old or new value of NULL + * against any other value. + */ _is_equal_database_value( $raw_old_value, $value ) ) ) { From b97bbced79f94afb7bc6d3ca3ef43d283b5131ad Mon Sep 17 00:00:00 2001 From: costdev Date: Mon, 9 Oct 2023 22:38:51 +0100 Subject: [PATCH 20/22] =?UTF-8?q?Blame=20@joemcgill=20=F0=9F=98=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/wp-includes/option.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index eeea2c81b10b5..c8136950307a3 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -2210,6 +2210,7 @@ function update_network_option( $network_id, $option, $value ) { * As NULL is unique in the database, skip checking an old or new value of NULL * against any other value. */ + ( ! is_multisite() || ! ( null === $raw_old_value || null === $value ) ) && _is_equal_database_value( $raw_old_value, $value ) ) ) { From 219f839f63ebd365946988b2bea4571eaa4dcc55 Mon Sep 17 00:00:00 2001 From: costdev Date: Mon, 9 Oct 2023 23:02:02 +0100 Subject: [PATCH 21/22] Tests: Reduce test skipping to minimal. Only one test remains skipped as it is unique to single site. --- tests/phpunit/tests/option/networkOption.php | 261 ++++--------------- 1 file changed, 47 insertions(+), 214 deletions(-) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index c19e625554a3c..2b4a58a37e0c4 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -697,13 +697,10 @@ public function data_stored_as_empty_string() { * @covers ::update_network_option */ public function test_update_network_option_with_pre_filter_adds_missing_option() { - - if ( is_multisite() ) { - $this->markTestSkipped( 'This test should only run on Single Site.' ); - } + $hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo'; // Force a return value of integer 0. - add_filter( 'pre_option_foo', '__return_zero' ); + add_filter( $hook_name, '__return_zero' ); /* * This should succeed, since the 'foo' option does not exist in the database. @@ -720,16 +717,13 @@ public function test_update_network_option_with_pre_filter_adds_missing_option() * @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.' ); - } + $hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo'; // 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' ); + add_filter( $hook_name, '__return_zero' ); /* * This should succeed, since the 'foo' option has a value of 1 in the database. @@ -746,86 +740,61 @@ public function test_update_network_option_with_pre_filter_updates_option_with_d * @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.' ); - } + $hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo'; - add_filter( 'pre_option_foo', '__return_zero' ); + add_filter( $hook_name, '__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 ) ); + $this->assertSame( 10, has_filter( $hook_name, '__return_zero' ) ); } /** - * Tests that an existing option is updated even when its pre filter returns the same value. + * Tests that update_network_option() conditionally applies + * 'pre_site_option_{$option}' and 'pre_option_{$option}' filters. * * @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.' ); - } + public function test_update_network_option_should_conditionally_apply_pre_site_option_and_pre_option_filters() { + $option = 'foo'; + $site_hook = new MockAction(); + $option_hook = new MockAction(); - // Add the option with a value of 1 to the database. - update_network_option( null, 'foo', 1 ); + add_filter( "pre_site_option_{$option}", array( $site_hook, 'filter' ) ); + add_filter( "pre_option_{$option}", array( $option_hook, 'filter' ) ); - // Force a return value of integer 0. - add_filter( 'pre_site_option_foo', '__return_zero' ); + update_network_option( null, $option, 'false' ); - /* - * 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->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters should have been applied once." ); + $this->assertSame( is_multisite() ? 0 : 1, $option_hook->get_call_count(), "'pre_option_{$option}' filters should have been applied once." ); } /** - * Tests that calling update_network_option() does not permanently remove pre filters. + * Tests that update_network_option() conditionally applies + * 'default_site_{$option}' and 'default_option_{$option}' 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.' ); - } + public function test_update_network_option_should_conditionally_apply_site_and_option_default_value_filters() { + $option = 'foo'; + $site_hook = new MockAction(); + $option_hook = new MockAction(); - add_filter( 'pre_site_option_foo', '__return_zero' ); - update_network_option( null, 'foo', 0 ); + add_filter( "default_site_option_{$option}", array( $site_hook, 'filter' ) ); + add_filter( "default_option_{$option}", array( $option_hook, 'filter' ) ); - // Assert that the filter is still present. - $this->assertSame( 10, has_filter( 'pre_site_option_foo', '__return_zero' ) ); + update_network_option( null, $option, 'false' ); + + $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters should have been applied twice." ); + $this->assertSame( is_multisite() ? 0 : 2, $option_hook->get_call_count(), "'default_option_{$option}' filters should have been applied twice." ); } /** - * Tests that update_network_option() adds a non-existent option that uses - * a filtered default site value and a filtered default option value in Single Site. + * Tests that update_network_option() adds a non-existent option that uses a filtered default value. * * @ticket 59360 * @@ -834,10 +803,6 @@ public function test_multisite_update_network_option_maintains_pre_filters() { public function test_update_network_option_should_add_option_with_filtered_default_value() { global $wpdb; - if ( is_multisite() ) { - $this->markTestSkipped( 'This test should only run on Single Site.' ); - } - $option = 'foo'; $default_site_value = 'default-site-value'; $default_option_value = 'default-option-value'; @@ -863,158 +828,26 @@ static function () use ( $default_option_value ) { */ $this->assertTrue( update_network_option( null, $option, false ), 'update_network_option() should have returned true.' ); - $actual = $wpdb->get_row( - $wpdb->prepare( - "SELECT option_value FROM $wpdb->options WHERE option_name = %s LIMIT 1", - $option - ) - ); - - $this->assertIsObject( $actual, 'The option was not added to the database.' ); - $this->assertObjectHasProperty( 'option_value', $actual, 'The "option_value" property was not included.' ); - $this->assertSame( '', $actual->option_value, 'The new value was not stored in the database.' ); - } - - /** - * Tests that update_network_option() applies site and option default value filters on Single Site. - * - * @ticket 59360 - * - * @covers ::update_network_option - */ - public function test_update_network_option_should_apply_site_and_option_default_value_filters_on_single_site() { - if ( is_multisite() ) { - $this->markTestSkipped( 'This test should only run on Single Site.' ); - } - - $option = 'foo'; - $site_hook = new MockAction(); - $option_hook = new MockAction(); - - add_filter( "default_site_option_{$option}", array( $site_hook, 'filter' ) ); - add_filter( "default_option_{$option}", array( $option_hook, 'filter' ) ); - - update_network_option( null, $option, 'false' ); - - $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters should have been applied twice." ); - $this->assertSame( 2, $option_hook->get_call_count(), "'default_option_{$option}' filters should have been applied twice." ); - } - - /** - * Tests that update_network_option() applies only site option default value filters on Multisite. - * - * @ticket 59360 - * - * @covers ::update_network_option - */ - public function test_update_network_option_should_apply_only_site_default_value_filters_on_multisite() { - if ( ! is_multisite() ) { - $this->markTestSkipped( 'This test should only run on Multisite.' ); - } - - $option = 'foo'; - $site_hook = new MockAction(); - $option_hook = new MockAction(); - - add_filter( "default_site_option_{$option}", array( $site_hook, 'filter' ) ); - add_filter( "default_option_{$option}", array( $option_hook, 'filter' ) ); - - update_network_option( null, $option, 'false' ); - - $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters should have been applied twice." ); - $this->assertSame( 0, $option_hook->get_call_count(), "'default_option_{$option}' filters should not have been applied." ); - } - - /** - * Tests that update_network_option() applies 'pre_site_option_{$option}' and 'pre_option_{$option}' filters on Single Site. - * - * @ticket 59360 - * - * @covers ::update_network_option - */ - public function test_update_network_option_should_apply_pre_site_option_and_pre_option_filters_on_single_site() { if ( is_multisite() ) { - $this->markTestSkipped( 'This test should only run on Single Site.' ); - } - - $option = 'foo'; - $site_hook = new MockAction(); - $option_hook = new MockAction(); - - add_filter( "pre_site_option_{$option}", array( $site_hook, 'filter' ) ); - add_filter( "pre_option_{$option}", array( $option_hook, 'filter' ) ); - - update_network_option( null, $option, 'false' ); - - $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters should have been applied once." ); - $this->assertSame( 1, $option_hook->get_call_count(), "'pre_option_{$option}' filters should have been applied once." ); - } - - /** - * Tests that update_network_option() applies only 'pre_site_option_{$option}' filters on Multisite. - * - * @ticket 59360 - * - * @covers ::update_network_option - */ - public function test_update_network_option_should_apply_only_pre_site_option_filters_on_multisite() { - if ( ! is_multisite() ) { - $this->markTestSkipped( 'This test should only run on Multisite.' ); - } - - $option = 'foo'; - $site_hook = new MockAction(); - $option_hook = new MockAction(); - - add_filter( "pre_site_option_{$option}", array( $site_hook, 'filter' ) ); - add_filter( "pre_option_{$option}", array( $option_hook, 'filter' ) ); - - update_network_option( null, $option, 'false' ); - - $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters should have been applied once." ); - $this->assertSame( 0, $option_hook->get_call_count(), "'pre_option_{$option}' filters should not have been applied." ); - } - - /** - * 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.' ); + $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 + ) + ); } - $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 - ) - ); + $value_field = is_multisite() ? 'meta_value' : 'option_value'; $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.' ); + $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.' ); } } From c61dd4bbf2dbe993efee97d4d16866f73bf51106 Mon Sep 17 00:00:00 2001 From: costdev Date: Mon, 9 Oct 2023 23:08:31 +0100 Subject: [PATCH 22/22] Tests: Improve `$message` parameters - again. `$message` parameters, SEASON 2 FINALE! SEASON 3 COMING SOON! --- tests/phpunit/tests/option/networkOption.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/phpunit/tests/option/networkOption.php b/tests/phpunit/tests/option/networkOption.php index 2b4a58a37e0c4..2b0bed6504b5b 100644 --- a/tests/phpunit/tests/option/networkOption.php +++ b/tests/phpunit/tests/option/networkOption.php @@ -767,8 +767,8 @@ public function test_update_network_option_should_conditionally_apply_pre_site_o update_network_option( null, $option, 'false' ); - $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters should have been applied once." ); - $this->assertSame( is_multisite() ? 0 : 1, $option_hook->get_call_count(), "'pre_option_{$option}' filters should have been applied once." ); + $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters occurred an unexpected number of times." ); + $this->assertSame( is_multisite() ? 0 : 1, $option_hook->get_call_count(), "'pre_option_{$option}' filters occurred an unexpected number of times." ); } /** @@ -789,8 +789,8 @@ public function test_update_network_option_should_conditionally_apply_site_and_o update_network_option( null, $option, 'false' ); - $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters should have been applied twice." ); - $this->assertSame( is_multisite() ? 0 : 2, $option_hook->get_call_count(), "'default_option_{$option}' filters should have been applied twice." ); + $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters occurred an unexpected number of times." ); + $this->assertSame( is_multisite() ? 0 : 2, $option_hook->get_call_count(), "'default_option_{$option}' filters occurred an unexpected number of times." ); } /**