update_network_option() strict checks can cause false negatives

[mukeshpanchal27]

Trac ticket: https://core.trac.wordpress.org/ticket/59360

This PR update the production code of update_network_option() and the unit tests. The unit tests update shows that after the code change it will reduce some queries for multisite.

---

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.

[costdev]

Looks good to me! 👍

[joemcgill]

Left a few suggestions, nothing blocking. Looks good to me, pending any adjustments you want to make.

[felixarntz]

@mukeshpanchal27 While this PR looks mostly good to me, I have some concerns about it, most importantly two points one of which @joemcgill raised as well.

I think we'll need clarity on that and potentially make changes before committing. I personally am not convinced this is safe to commit at this point, so late in the beta cycle.

[felixarntz]

@mukeshpanchal27 @spacedmonkey @costdev @joemcgill I think we need to take a step back here. Based on some of the feedback above, this change is not ready for commit, as there are outstanding problems to address. Some thoughts:

• How do we deal with the pre option filters?
• Do we need to deal with them at all, or can we skip that part in favor of a cleaner solution?
• Can we look up whether the value exists in the DB directly instead? Or would that be a BC break?

At a higher level, the questions here are:

1. How do we want this to work in an ideal scenario (i.e. if BC wasn't a concern)?
2. How can we make this work, considering BC, that is as close to what we want but has as little incompatibilities as possible?

I think we should talk about those points first before continuing to make changes to the code preliminarily.

[spacedmonkey]

@felixarntz I disagree a little here. The beavhour between update_option and update_network_optoin should be the same. What ever one function has, the other should have. I don't see why that is complex.

[felixarntz]

@spacedmonkey

What ever one function has, the other should have.

Definitely, I agree. I think if we we hold off committing this because we want to rethink the approach, it is because the change here is more complex than we thought, and in that case we would need to revert the changes from 22192 as well.

See related Slack discussion: I think if we add the pre_option_{$option} handling to this PR, then it'll be okay to commit (even though that logic is super ugly 🙃).

[felixarntz]

@mukeshpanchal27 @costdev This looks almost good to go. I think one more concern to address regarding potentially incorrect defaults on non-multisite.

[felixarntz]

Thanks for all the updates @mukeshpanchal27 @costdev! LGTM 🎉

[mukeshpanchal27]

Thank you for all the valuable feedback and insights, @joemcgill and @felixarntz. A special thanks to @costdev for pushing the changes while I was asleep. It's wonderful to see that when I began working, everything was ready for commit. 🦸‍♂️ 🚀

[spacedmonkey]

Committed https://core.trac.wordpress.org/changeset/56814