Make WordPress Core

Changeset 56814


Ignore:
Timestamp:
10/10/2023 09:35:09 AM (7 months ago)
Author:
spacedmonkey
Message:

Options, Meta APIs: Prevent unnecessary database updates caused by strict comparisons in update_network_option.

Previously, in the update_network_option function, strict comparisons between old and new values could erroneously trigger updates in cases where there was no actual change in values. Building upon the groundwork laid in #22192, this commit introduces the use of the _is_equal_database_value function to accurately compare values before initiating any database updates. This change ensures consistency between the behaviors of update_option and update_network_option.

Furthermore, we have incorporated similar workarounds that were previously implemented in [56717]. These changes ensure that the raw version of network option values is considered in the comparisons, enhancing the overall reliability of the update process.

Props mukesh27, flixos90, joemcgill, costdev, spacedmonkey.
Fixes #59360.

Location:
trunk
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/wp-includes/option.php

    r56796 r56814  
    21262126    wp_protect_special_option( $option );
    21272127
    2128     $old_value = get_network_option( $network_id, $option, false );
     2128    $old_value = get_network_option( $network_id, $option );
    21292129
    21302130    /**
     
    21462146
    21472147    /*
     2148     * To get the actual raw old value from the database, any existing pre filters need to be temporarily disabled.
     2149     * Immediately after getting the raw value, they are reinstated.
     2150     * The raw value is only used to determine whether a value is present in the database. It is not used anywhere
     2151     * else, and is not passed to any of the hooks either.
     2152     */
     2153    global $wp_filter;
     2154
     2155    /** This filter is documented in wp-includes/option.php */
     2156    $default_value = apply_filters( "default_site_option_{$option}", false, $option, $network_id );
     2157
     2158    $has_site_filter   = has_filter( "pre_site_option_{$option}" );
     2159    $has_option_filter = has_filter( "pre_option_{$option}" );
     2160    if ( $has_site_filter || $has_option_filter ) {
     2161        if ( $has_site_filter ) {
     2162            $old_ms_filters = $wp_filter[ "pre_site_option_{$option}" ];
     2163            unset( $wp_filter[ "pre_site_option_{$option}" ] );
     2164        }
     2165
     2166        if ( $has_option_filter ) {
     2167            $old_single_site_filters = $wp_filter[ "pre_option_{$option}" ];
     2168            unset( $wp_filter[ "pre_option_{$option}" ] );
     2169        }
     2170
     2171        if ( is_multisite() ) {
     2172            $raw_old_value = get_network_option( $network_id, $option );
     2173        } else {
     2174            $raw_old_value = get_option( $option, $default_value );
     2175        }
     2176
     2177        if ( $has_site_filter ) {
     2178            $wp_filter[ "pre_site_option_{$option}" ] = $old_ms_filters;
     2179        }
     2180        if ( $has_option_filter ) {
     2181            $wp_filter[ "pre_option_{$option}" ] = $old_single_site_filters;
     2182        }
     2183    } else {
     2184        $raw_old_value = $old_value;
     2185    }
     2186
     2187    if ( ! is_multisite() ) {
     2188        /** This filter is documented in wp-includes/option.php */
     2189        $default_value = apply_filters( "default_option_{$option}", $default_value, $option, true );
     2190    }
     2191
     2192    /*
    21482193     * If the new and old values are the same, no need to update.
    21492194     *
    2150      * Unserialized values will be adequate in most cases. If the unserialized
    2151      * data differs, the (maybe) serialized data is checked to avoid
    2152      * unnecessary database calls for otherwise identical object instances.
    2153      *
    2154      * See https://core.trac.wordpress.org/ticket/44956
    2155      */
    2156     if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
     2195     * An exception applies when no value is set in the database, i.e. the old value is the default.
     2196     * In that case, the new value should always be added as it may be intentional to store it rather than relying on the default.
     2197     *
     2198     * See https://core.trac.wordpress.org/ticket/44956 and https://core.trac.wordpress.org/ticket/22192 and https://core.trac.wordpress.org/ticket/59360
     2199     */
     2200    if (
     2201        $value === $raw_old_value ||
     2202        (
     2203            false !== $raw_old_value &&
     2204            /*
     2205             * Single site stores values in the `option_value` field, which cannot be set to NULL.
     2206             * This means a PHP `null` value will be cast to an empty string, which can be considered
     2207             * equal to values such as an empty string, or false when cast to string.
     2208             *
     2209             * However, Multisite stores values in the `meta_value` field, which can be set to NULL.
     2210             * As NULL is unique in the database, skip checking an old or new value of NULL
     2211             * against any other value.
     2212             */
     2213            ( ! is_multisite() || ! ( null === $raw_old_value || null === $value ) ) &&
     2214            _is_equal_database_value( $raw_old_value, $value )
     2215        )
     2216    ) {
    21572217        return false;
    21582218    }
    21592219
    2160     if ( false === $old_value ) {
     2220    if ( $default_value === $raw_old_value ) {
    21612221        return add_network_option( $network_id, $option, $value );
    21622222    }
  • trunk/tests/phpunit/tests/option/networkOption.php

    r56797 r56814  
    400400     * @param mixed $new_value The new value to try to set.
    401401     */
    402     public function test_update_option_should_not_update_some_values_from_cache( $old_value, $new_value ) {
     402    public function test_update_network_option_should_not_update_some_values_from_cache( $old_value, $new_value ) {
    403403        add_network_option( null, 'foo', $old_value );
    404404
     
    408408        $updated = update_network_option( null, 'foo', $new_value );
    409409
    410         $expected_queries = $old_value === $new_value || ! is_multisite() ? 0 : 1;
    411         $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." );
    412 
     410        $this->assertSame( $num_queries, get_num_queries(), 'No additional queries should have run.' );
    413411        $this->assertFalse( $updated, 'update_network_option() should have returned false.' );
    414412    }
     
    442440        $updated = update_network_option( null, 'foo', $new_value );
    443441
    444         // Mimic the behavior of the database by casting the old value to string.
    445         if ( is_scalar( $old_value ) ) {
    446             $old_value = (string) $old_value;
    447         }
    448 
    449         $expected_queries = $old_value === $new_value || ! is_multisite() ? 1 : 2;
    450         $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." );
    451 
     442        $this->assertSame( 1, get_num_queries() - $num_queries, 'One additional query should have run to update the value.' );
    452443        $this->assertFalse( $updated, 'update_network_option() should have returned false.' );
    453444    }
     
    481472         * with no additional queries.
    482473         */
    483         $expected_queries = $old_value === $new_value || ! is_multisite() ? 0 : 1;
    484         $this->assertSame( $expected_queries, get_num_queries() - $num_queries, "The number of queries should have increased by $expected_queries." );
    485 
     474        $this->assertSame( $num_queries, get_num_queries(), 'No additional queries should have run.' );
    486475        $this->assertFalse( $updated, 'update_network_option() should have returned false.' );
    487476    }
     
    575564     * @covers ::update_network_option
    576565     */
    577     public function test_update_option_should_handle_a_null_new_value_from_cache() {
     566    public function test_update_network_option_should_handle_a_null_new_value_from_cache() {
    578567        add_network_option( null, 'foo', '' );
    579568
     
    607596     * @covers ::update_network_option
    608597     */
    609     public function test_update_option_should_handle_a_null_new_value_from_db() {
     598    public function test_update_network_option_should_handle_a_null_new_value_from_db() {
    610599        add_network_option( null, 'foo', '' );
    611600
     
    643632     * @covers ::update_network_option
    644633     */
    645     public function test_update_option_should_handle_a_null_new_value_from_refreshed_cache() {
     634    public function test_update_network_option_should_handle_a_null_new_value_from_refreshed_cache() {
    646635        add_network_option( null, 'foo', '' );
    647636
     
    700689        );
    701690    }
     691
     692    /**
     693     * Tests that a non-existent option is added even when its pre filter returns a value.
     694     *
     695     * @ticket 59360
     696     *
     697     * @covers ::update_network_option
     698     */
     699    public function test_update_network_option_with_pre_filter_adds_missing_option() {
     700        $hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo';
     701
     702        // Force a return value of integer 0.
     703        add_filter( $hook_name, '__return_zero' );
     704
     705        /*
     706         * This should succeed, since the 'foo' option does not exist in the database.
     707         * The default value is false, so it differs from 0.
     708         */
     709        $this->assertTrue( update_network_option( null, 'foo', 0 ) );
     710    }
     711
     712    /**
     713     * Tests that an existing option is updated even when its pre filter returns the same value.
     714     *
     715     * @ticket 59360
     716     *
     717     * @covers ::update_network_option
     718     */
     719    public function test_update_network_option_with_pre_filter_updates_option_with_different_value() {
     720        $hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo';
     721
     722        // Add the option with a value of 1 to the database.
     723        update_network_option( null, 'foo', 1 );
     724
     725        // Force a return value of integer 0.
     726        add_filter( $hook_name, '__return_zero' );
     727
     728        /*
     729         * This should succeed, since the 'foo' option has a value of 1 in the database.
     730         * Therefore it differs from 0 and should be updated.
     731         */
     732        $this->assertTrue( update_network_option( null, 'foo', 0 ) );
     733    }
     734
     735    /**
     736     * Tests that calling update_network_option() does not permanently remove pre filters.
     737     *
     738     * @ticket 59360
     739     *
     740     * @covers ::update_network_option
     741     */
     742    public function test_update_network_option_maintains_pre_filters() {
     743        $hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo';
     744
     745        add_filter( $hook_name, '__return_zero' );
     746        update_network_option( null, 'foo', 0 );
     747
     748        // Assert that the filter is still present.
     749        $this->assertSame( 10, has_filter( $hook_name, '__return_zero' ) );
     750    }
     751
     752    /**
     753     * Tests that update_network_option() conditionally applies
     754     * 'pre_site_option_{$option}' and 'pre_option_{$option}' filters.
     755     *
     756     * @ticket 59360
     757     *
     758     * @covers ::update_network_option
     759     */
     760    public function test_update_network_option_should_conditionally_apply_pre_site_option_and_pre_option_filters() {
     761        $option      = 'foo';
     762        $site_hook   = new MockAction();
     763        $option_hook = new MockAction();
     764
     765        add_filter( "pre_site_option_{$option}", array( $site_hook, 'filter' ) );
     766        add_filter( "pre_option_{$option}", array( $option_hook, 'filter' ) );
     767
     768        update_network_option( null, $option, 'false' );
     769
     770        $this->assertSame( 1, $site_hook->get_call_count(), "'pre_site_option_{$option}' filters occurred an unexpected number of times." );
     771        $this->assertSame( is_multisite() ? 0 : 1, $option_hook->get_call_count(), "'pre_option_{$option}' filters occurred an unexpected number of times." );
     772    }
     773
     774    /**
     775     * Tests that update_network_option() conditionally applies
     776     * 'default_site_{$option}' and 'default_option_{$option}' filters.
     777     *
     778     * @ticket 59360
     779     *
     780     * @covers ::update_network_option
     781     */
     782    public function test_update_network_option_should_conditionally_apply_site_and_option_default_value_filters() {
     783        $option      = 'foo';
     784        $site_hook   = new MockAction();
     785        $option_hook = new MockAction();
     786
     787        add_filter( "default_site_option_{$option}", array( $site_hook, 'filter' ) );
     788        add_filter( "default_option_{$option}", array( $option_hook, 'filter' ) );
     789
     790        update_network_option( null, $option, 'false' );
     791
     792        $this->assertSame( 2, $site_hook->get_call_count(), "'default_site_option_{$option}' filters occurred an unexpected number of times." );
     793        $this->assertSame( is_multisite() ? 0 : 2, $option_hook->get_call_count(), "'default_option_{$option}' filters occurred an unexpected number of times." );
     794    }
     795
     796    /**
     797     * Tests that update_network_option() adds a non-existent option that uses a filtered default value.
     798     *
     799     * @ticket 59360
     800     *
     801     * @covers ::update_network_option
     802     */
     803    public function test_update_network_option_should_add_option_with_filtered_default_value() {
     804        global $wpdb;
     805
     806        $option               = 'foo';
     807        $default_site_value   = 'default-site-value';
     808        $default_option_value = 'default-option-value';
     809
     810        add_filter(
     811            "default_site_option_{$option}",
     812            static function () use ( $default_site_value ) {
     813                return $default_site_value;
     814            }
     815        );
     816
     817        add_filter(
     818            "default_option_{$option}",
     819            static function () use ( $default_option_value ) {
     820                return $default_option_value;
     821            }
     822        );
     823
     824        /*
     825         * For a non existing option with the unfiltered default of false, passing false here wouldn't work.
     826         * Because the default is different than false here though, passing false is expected to result in
     827         * a database update.
     828         */
     829        $this->assertTrue( update_network_option( null, $option, false ), 'update_network_option() should have returned true.' );
     830
     831        if ( is_multisite() ) {
     832            $actual = $wpdb->get_row(
     833                $wpdb->prepare(
     834                    "SELECT meta_value FROM $wpdb->sitemeta WHERE meta_key = %s LIMIT 1",
     835                    $option
     836                )
     837            );
     838        } else {
     839            $actual = $wpdb->get_row(
     840                $wpdb->prepare(
     841                    "SELECT option_value FROM $wpdb->options WHERE option_name = %s LIMIT 1",
     842                    $option
     843                )
     844            );
     845        }
     846
     847        $value_field = is_multisite() ? 'meta_value' : 'option_value';
     848
     849        $this->assertIsObject( $actual, 'The option was not added to the database.' );
     850        $this->assertObjectHasProperty( $value_field, $actual, "The '$value_field' property was not included." );
     851        $this->assertSame( '', $actual->$value_field, 'The new value was not stored in the database.' );
     852    }
    702853}
Note: See TracChangeset for help on using the changeset viewer.