delete_network_options should only set network notoptions on multisites
#7074
Conversation
…isites On a non-multisite, `get_network_option()`, `add_network_option()`, and `update_network_option()` don't bother updating the network `notoptions` cache, instead reling on `get_option()`, `add_option()`, and `update_option()` to use the non-network `notoptions`. `delete_options()` and `delete_network_options()` were recently updated to populate the `notoptions` caches. But, contrary to existing practice, `delete_network_options()` was made to update the network `notoptions` cache even on non-multisites. This moves the updating of the network `notoptions` cache in `delete_network_options()` to the multisite code path, matching the behavior of the other functions, and adds a test to verify this. Trac ticket: https://core.trac.wordpress.org/ticket/61730
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
An alternative fix would be to update the other functions to all populate the network |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
This looks good to me, thank you!
I created #7079 to:
- ensure the new test added here failed without the fix
- add some additional tests
- push the code change to ensure the tests started passing
It all worked as expected so I have taken the liberty of pushing 3ec1a4d to this branch with the additional tests. This will allow us to work from a single PR.
@pbearne are you able to take a look at the updated code?
| ) | ||
| ); | ||
|
|
||
| if ( $result ) { |
There was a problem hiding this comment.
Sorry, on second thoughts I am wondering if this should be
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
delete_option()then has the opposite problem, it setsnotoptionseven 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?
There was a problem hiding this comment.
I think I'd prefer you fix these issues in a separate PR.
On a non-multisite,
get_network_option(),add_network_option(), andupdate_network_option()don't bother updating the networknotoptionscache, instead reling onget_option(),add_option(), andupdate_option()to use the non-networknotoptions.delete_options()anddelete_network_options()were recently updated to populate thenotoptionscaches. But, contrary to existing practice,delete_network_options()was made to update the networknotoptionscache even on non-multisites.This moves the updating of the network
notoptionscache indelete_network_options()to the multisite code path, matching the behavior of the other functions, and adds a test to verify this.Trac ticket: https://core.trac.wordpress.org/ticket/61730
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.