update_network_option() strict checks can cause false negatives

[mukesh27]

Follow up: #22192

add_network_option( $network_id, $option_name, 1 );
update_network_option( $network_id, $option_name, 1 );

The update should not work, because they are the same. However, the meta cache will have "1" as a string, and then it will strict compare it to 1 as an integer. Thus, an unnecessary update will run.

It is quite common to use integers and booleans directly into these functions. They should be smart enough to recognize that "1" == 1 == true and "0" == 0 == false, and that any numeric string is also equal to a properly cast integer.

The new changes need unit tests.

Ticket from which this was spun: #22189, saving navigation menus is slow.

cc. @flixos90 @spacedmonkey @joemcgill

This PR check the behaviour of update_network_option()

Hello, @felixarntz, @joemcgill, and @spacedmonkey, This PR demonstrates the behavior of the current update_network_option() function. @costdev provided valuable suggestions and assistance in identifying backward compatibility (BC) and edge cases, which we have addressed and included in the unit tests.

When you have some free time, could you please review it? Your feedback would be greatly appreciated.

As we are approaching the RCs, we may defer this, but committing these older unit tests will facilitate our progress in implementing code changes and adding new unit tests. Thank you!

[flixos90]

In 56797:

Options, Meta APIs: Add test coverage for update_network_option() comparison of new and existing value.

Having those tests in trunk already will help ensure potential future fixes to this logic maintain backward compatibility.

Props mukesh27, spacedmonkey.
See #59360.

Committed in https://core.trac.wordpress.org/changeset/56797

[flixos90]

@mukesh27 Per [56797] committed, let's open a follow up PR with the actual production code changes, to address this ticket.

It shouldn't require any test changes, and in fact maintaining the tests and having them all pass should help ensure there are no BC breaks.

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.

@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.

@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.

@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 🙃).

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. 🦸‍♂️ 🚀

[mukesh27]

Great news! With sufficient approvals on the PR, it's time to prepare it for committing. Let's move forward and get this done.👍 🚀

[spacedmonkey]

Assigning to myself to commit.

[spacedmonkey]

In 56814:

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.

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

[costdev]

Reopening to handle pre_option filters too (see this comment on the related ticket).

I'm AFK for a lot of today and won't get time to get a PR together before 6.4 Beta 3, so I'll follow up with a PR later this evening.

[jrf]

Heads-up: I'm seeing automated test runs for plugins against WP multisite currently failing and have traced these failure back to this [56814] commit.

I suggest further investigation and potentially reverting the commit in the mean time.

[costdev]

@jrf Thanks for the report! Can you direct us towards the failures, or a test that will reproduce the failures so we can investigate?

[jrf]

Replying to costdev:

@jrf Thanks for the report! Can you direct us towards the failures, or a test that will reproduce the failures so we can investigate?

You can see a failing build here: ​https://github.com/Yoast/wordpress-seo/actions/runs/6470608918/job/17567196156

The underlying code which the failures are related to is the ​https://github.com/Yoast/wordpress-seo/blob/trunk/inc/options/class-wpseo-option.php class which ensures that for select options, the values are stable, i.e. the values are arrays and the class makes sure that if any of the expected array keys are missing from the option, they get added with a default value to make sure that all code in the plugin which relies on the option can always count on all keys being available and set.

[costdev]

Thanks @jrf!

It looks that changing ​this line to the following makes the tests pass again.

if ( false === $raw_old_value ) {

The change to use $default_value rather than false on that line in trunk was not originally going to be part of the patch, but we (mistakenly) thought that this wouldn't cause a problem and would be a benefit (mistakenly*2).

@jrf I don't suppose you could verify that making the above change resolves the issue in the Yoast tests?

[spacedmonkey]

Reassigning to @flixos90 to own, as I am unavailable.

[azaozz]

This ticket seems pretty much a duplicate of #22192. Even the description is copied from there.
Shouldn't the commit here be reverted and then both places be fixed from #22192, as the method to fix these and the code is the same?

This reverts a change that used $default_value when determining whether to add a new network option, and restores the use of false to make this determination.

[costdev]

This ticket seems pretty much a duplicate of #22192. Even the description is copied from there.
Shouldn't the commit here be reverted and then both places be fixed from #22192, as the method to fix these and the code is the same?

There's some different considerations between the functions, such as different filters/numbers of filters, nullable vs non-nullable database fields and such. IMO, it's been beneficial having the work for each function separated to allow us to focus on a given function in its own context.

[costdev]

@mukesh27 @flixos90 @joemcgill @hellofromTonya With logic revisions under discussion, a BC break pending investigation, confirmation, and test updates, I'm thinking we should revert for 6.4 and reconsider our strategy here.

This component is so crucial for plugins, and despite having a possible fix for the latest issue, we've now identified a few breaks during this work and it's getting late in the cycle. I'm not yet confident that we have a stable enough patch for release.

Given this, I'd propose that we:

• Revert the commits for this ticket and the update_option() ticket (#22192).
• Do more testing with various plugins to determine possible breakages.
• Revisit and add to our existing test coverage.
• Commit the tests during the 6.5 cycle.
• With improved knowledge and tests, look again at exactly what we want to/can/should achieve.

What are your thoughts?

[joemcgill]

Replying to costdev:

...I'm thinking we should revert for 6.4 and reconsider our strategy here.

This component is so crucial for plugins, and despite having a possible fix for the latest issue, we've now identified a few breaks during this work and it's getting late in the cycle. I'm not yet confident that we have a stable enough patch for release.

Thanks for raising the question, @costdev. I'd like to see if we can remove the pre_filter juggling in both functions and add tests that validate that the previous behavior is protected when pre_filters are in place. However, if we can't get a good solution before RC1, I do think your proposal makes sense.

[flixos90]

I agree with @joemcgill, let's try to fix it first, we have an idea for it already. @joemcgill I believe you're working on this together with the same fix for #22192?

[joemcgill]

Replying to flixos90:

@joemcgill I believe you're working on this together with the same fix for #22192?

Correct. Will be working on this again today. Will reassign myself as owner, but don't let me stop anyone else from contributing.

[hellofromTonya]

Hello all,

This component is so crucial for plugins, and despite having a possible fix for the latest issue, we've now identified a few breaks during this work and it's getting late in the cycle. I'm not yet confident that we have a stable enough patch for release.

@costdev concerns give me pause due to "identified a few breaks" and his "not yet confident".

Noting @jrf:

The underlying code which the failures are related to is the ​https://github.com/Yoast/wordpress-seo/blob/trunk/inc/options/class-wpseo-option.php class which ensures that for select options, the values are stable, i.e. the values are arrays and the class makes sure that if any of the expected array keys are missing from the option, they get added with a default value to make sure that all code in the plugin which relies on the option can always count on all keys being available and set.

Quoting @joemcgill

I'd like to see if we can remove the pre_filter juggling in both functions and add tests that validate that the previous behavior is protected when pre_filters are in place.

If removing the pre_filter juggling has very high confidence in resolving the issue and there's very high confidence that "identified a few breaks" will not have impacts on the release, then the fix could go in. Otherwise, I'd advise leaning into caution by reverting and continuing the work during the early phases of 6.5 Alpha.

While yes, these changes can be reverted during the RC cycle, IMO that should not be the target. Rather, (again IMO) the release should go into the RC cycle with high confidence the changes are ready to ship without impact.

Timing: A revert or fix decision can happen before RC1, though I'd advice making that decision and doing the revert / commit the day before (at latest) in case another unscheduled beta is needed.

[costdev]

Update/summary on the BC break discovered during Yoast's test runs:

• In 6.3, update_network_option() adds an option if ( false === $old_value ). ​Reference
• For parity with update_option(), we changed this to if ( $default_value === $raw_old_value ) ​Reference.
  • $raw_old_value is fine, $default_value is not.
• This change caused failures in the Yoast test suite on multisite. ​Reference
• ​PR 5465 reverts this change, and updates our default value test (which was protecting this BC break) to ensure that update_network_option() does not add an option with a filtered default value. That is, the default value should have no bearing on whether the option is added.
  • This test was also run against the 6.3 branch on multisite and it passes as expected, showing that this is the expected behaviour to protect BC.

[joemcgill]

In 56946:

Options, Meta APIs: Revert update_option changes.

This reverts changes from [56648], [56681], [56717], [56762], [56788], [56797], and [56814] to restore the behavior for update_option() and update_network_option() to their prior state as of 6.3.X due to the discovery of various backwards compatibility issues found late in the 6.4 release cycle.

Props mukesh27, costdev, flixos90, spacedmonkey, snicco, jrf, joemcgill.
See #22192, #59360.

[joemcgill]

Now that the changes we attempted here have been reverted, I'm closing this as a duplicate of #22192. While it was beneficial to separate them out initially, the issue described here should be resolved together with the related update_option() issue.

[jrf]

Thanks @joemcgill ! I hope a better/more stable solution can be found for the WP 6.5 cycle.