Skip to content

delete_network_options should only set network notoptions on multisites #7074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

delete_network_options should only set network notoptions on multisites #7074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
575083f
`delete_network_options` should only set network `notoptions` on mult…
anomiex Jul 23, 2024
3ec1a4d
Additional tests.
peterwilsoncc Jul 24, 2024
5797e0d
Merge branch 'trunk' into fix/delete_network_option_notoptions_multisite
peterwilsoncc Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/wp-includes/option.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2264,6 +2264,17 @@ function delete_network_option( $network_id, $option ) {
'site_id' => $network_id,
)
);

if ( $result ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, on second thoughts I am wondering if this should be

Suggested change
if ( $result ) {
if ( false !== $result ) {

If $wpdb::delete() returns 0, it means that the query ran successfully but there was no option to delete. The result is the same, it's a known notoption.

If $wpdb::delete() returns false, then the query failed so it's not known whether or not the option doesn't exist.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a problem with the code here, there's the same problem already in trunk: the notoptions-setting code on lines 2296–2303 is guarded by an if ( $result ) on line 2269.

delete_option() then has the opposite problem, it sets notoptions even if $result === false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete_option() then has the opposite problem, it sets notoptions even if $result === false.

Good catch. I'll open the original ticket to note that it needs a little work to account for a database failure.

Are you happy to add some protections here or would you prefer me to do it on a separate PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer you fix these issues in a separate PR.

$notoptions_key = "$network_id:notoptions";
$notoptions = wp_cache_get( $notoptions_key, 'site-options' );

if ( ! is_array( $notoptions ) ) {
$notoptions = array();
}
$notoptions[ $option ] = true;
wp_cache_set( $notoptions_key, $notoptions, 'site-options' );
}
}

if ( $result ) {
Expand Down Expand Up @@ -2293,15 +2304,6 @@ function delete_network_option( $network_id, $option ) {
*/
do_action( 'delete_site_option', $option, $network_id );

$notoptions_key = "$network_id:notoptions";
$notoptions = wp_cache_get( $notoptions_key, 'site-options' );

if ( ! is_array( $notoptions ) ) {
$notoptions = array();
}
$notoptions[ $option ] = true;
wp_cache_set( $notoptions_key, $notoptions, 'site-options' );

return true;
}

Expand Down
110 changes: 110 additions & 0 deletions tests/phpunit/tests/option/networkOption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public function test_delete_network_option_on_only_one_network() {
* Tests that calling delete_network_option() updates nooptions when option deleted.
*
* @ticket 61484
* @ticket 61730
*
* @covers ::delete_network_option
*/
Expand All @@ -73,6 +74,11 @@ public function test_check_delete_network_option_updates_notoptions() {
$this->assertIsArray( $notoptions, 'The notoptions cache is expected to be an array.' );
$this->assertTrue( $notoptions['foo'], 'The deleted options is expected to be in notoptions.' );

if ( ! is_multisite() ) {
$network_notoptions = wp_cache_get( '1:notoptions', 'site-options' );
$this->assertTrue( empty( $network_notoptions['foo'] ), 'The deleted option is not expected to be in network notoptions on a non-multisite.' );
}

$before = get_num_queries();
get_network_option( 1, 'foo' );
$queries = get_num_queries() - $before;
Expand Down Expand Up @@ -302,4 +308,108 @@ public function test_add_network_option_clears_the_notoptions_cache() {
$updated_notoptions = wp_cache_get( $cache_key, $cache_group );
$this->assertArrayNotHasKey( $option_name, $updated_notoptions, 'The "foobar" option should not be in the notoptions cache after updating it.' );
}

/**
* Test adding a previously known notoption returns the correct value.
*
* @ticket 61730
*
* @covers ::add_network_option
* @covers ::delete_network_option
*/
public function test_adding_previous_notoption_returns_correct_value() {
$option_name = 'ticket_61730_option_to_be_created';

add_network_option( 1, $option_name, 'baz' );
delete_network_option( 1, $option_name );

$this->assertFalse( get_network_option( 1, $option_name ), 'The option should not be found.' );

add_network_option( 1, $option_name, 'foo' );
$this->assertSame( 'foo', get_network_option( 1, $option_name ), 'The option should return the newly set value.' );
}

/**
* Test `get_network_option()` does not use network notoptions cache for single sites.
*
* @ticket 61730
*
* @group ms-excluded
*
* @covers ::get_network_option
*/
public function test_get_network_option_does_not_use_network_notoptions_cache_for_single_sites() {
get_network_option( 1, 'ticket_61730_notoption' );

$network_notoptions_cache = wp_cache_get( '1:notoptions', 'site-options' );
$single_site_notoptions_cache = wp_cache_get( 'notoptions', 'options' );

$this->assertEmpty( $network_notoptions_cache, 'Network notoptions cache should not be set for single site installs.' );
$this->assertIsArray( $single_site_notoptions_cache, 'Single site notoptions cache should be set.' );
$this->assertArrayHasKey( 'ticket_61730_notoption', $single_site_notoptions_cache, 'The option should be in the notoptions cache.' );
}

/**
* Test `delete_network_option()` does not use network notoptions cache for single sites.
*
* @ticket 61730
* @ticket 61484
*
* @group ms-excluded
*
* @covers ::delete_network_option
*/
public function test_delete_network_option_does_not_use_network_notoptions_cache_for_single_sites() {
add_network_option( 1, 'ticket_61730_notoption', 'value' );
delete_network_option( 1, 'ticket_61730_notoption' );

$network_notoptions_cache = wp_cache_get( '1:notoptions', 'site-options' );
$single_site_notoptions_cache = wp_cache_get( 'notoptions', 'options' );

$this->assertEmpty( $network_notoptions_cache, 'Network notoptions cache should not be set for single site installs.' );
$this->assertIsArray( $single_site_notoptions_cache, 'Single site notoptions cache should be set.' );
$this->assertArrayHasKey( 'ticket_61730_notoption', $single_site_notoptions_cache, 'The option should be in the notoptions cache.' );
}

/**
* Test `get_network_option()` does not use single site notoptions cache for networks.
*
* @ticket 61730
*
* @group ms-required
*
* @covers ::get_network_option
*/
public function test_get_network_option_does_not_use_single_site_notoptions_cache_for_networks() {
get_network_option( 1, 'ticket_61730_notoption' );

$network_notoptions_cache = wp_cache_get( '1:notoptions', 'site-options' );
$single_site_notoptions_cache = wp_cache_get( 'notoptions', 'options' );

$this->assertEmpty( $single_site_notoptions_cache, 'Single site notoptions cache should not be set for multisite installs.' );
$this->assertIsArray( $network_notoptions_cache, 'Multisite notoptions cache should be set.' );
$this->assertArrayHasKey( 'ticket_61730_notoption', $network_notoptions_cache, 'The option should be in the notoptions cache.' );
}

/**
* Test `delete_network_option()` does not use single site notoptions cache for networks.
*
* @ticket 61730
* @ticket 61484
*
* @group ms-required
*
* @covers ::delete_network_option
*/
public function test_delete_network_option_does_not_use_single_site_notoptions_cache_for_networks() {
add_network_option( 1, 'ticket_61730_notoption', 'value' );
delete_network_option( 1, 'ticket_61730_notoption' );

$network_notoptions_cache = wp_cache_get( '1:notoptions', 'site-options' );
$single_site_notoptions_cache = wp_cache_get( 'notoptions', 'options' );

$this->assertEmpty( $single_site_notoptions_cache, 'Single site notoptions cache should not be set for multisite installs.' );
$this->assertIsArray( $network_notoptions_cache, 'Multisite notoptions cache should be set.' );
$this->assertArrayHasKey( 'ticket_61730_notoption', $network_notoptions_cache, 'The option should be in the notoptions cache.' );
}
}