update_network_option() strict checks can cause false negatives

src/wp-includes/option.php

@@ -2144,20 +2144,47 @@ 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.
 	 *
-	 * 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.
+			( ! $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 );
 	}
 
@@ -2169,7 +2196,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 );

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.' );
 	}
 
@@ -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.' );
 	}
 
@@ -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.' );
 	}
 
@@ -699,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' ) );
+	}
 }