update_option() strict checks can cause false negatives

[nacin]

Given this:

add_option( $option_name, 1 );
update_option( $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.

[SergeyBiryukov]

Related: #20712

[atimmer]

​22192.1.diff adds a test to test for the amount of queries executed after adding a meta values and right after updating it with the same value.

[duck_]

Replying to atimmer:

​22192.1.diff adds a test to test for the amount of queries executed after adding a meta values and right after updating it with the same value.

Thanks for the tests :) A few notes though:

• These tests shouldn't be in Tests_Meta_Query they belong more to Tests_Meta in tests/meta.php
• Can we use the num_queries property of $wpdb? If not then the reference to $wpdb should disappear.
• Debug cruft left-over in query_counter(). (Though the entire function might go per bullet above)

[atimmer]

​22192.2.diff updates the test with the notes of duck_.

[SergeyBiryukov]

Related: #31047

[boonebgorges]

See https://core.trac.wordpress.org/ticket/31047#comment:7 for a description of what I think the problem is and how it should probably be solved.

[boonebgorges]

22192.3.diff​ has some unit tests that demonstrate what's happening in the case of update_option(), with a partial suggested solution. (update_metadata() will be very similar.) The short story is that the strict equality check between old value and new value is getting tripped up by inconsistent type handling in our cache and in our database handlers.

There are really two different sets of cases. The first is the "normal" case, where the cache is populated from the database. In this case, scalar values will become strings, which means that we only need to cast the *new* value to a string to do the comparison. The second case is where you call add_option() and then immediately call update_option(). In this case, add_option() updates the cache in a way that respects the type of the original data, so the old/new value comparison needs to cast *both sides* to get a properly loose comparison.

The is_scalar()/(string) dance in 22192.3.diff fixes most of these issues. However, (string) false (or strval( false )) resolves to an empty string, *not* '0', so the tests involving the boolean false are failing. If we want to embrace the spirit of nacin's original bug report, we could introduce special handling for false. My personal thinking is that casting between string '1' and int 1 is pretty obvious from an API point of view, but casting bools is not, and it may not be a great developer experience to juggle booleans in the background. Maybe others feel differently?

This is a potentially breaking change in certain edge cases, so it'd be nice to get a second opinion on it.

[boonebgorges]

22192.5.diff​ is a more comprehensive solution (again, for the case of options). I've added unit tests that demonstrate the difference between the way types are juggled when set directly from add_option() and when pulled from the database.

I've also added the logic that would be necessary to get all the tests to pass - that is, to get 1 == '1' == true and 0 == '0' == false. It's pretty ugly. The fact that (string) false evaluates to an empty string means, in part, that add_option( 'foo', false ) results in a db row where the 'value' field is an empty string. So, in order for update_option( 'foo', $value ) to result in a cache hit when $value is falsey, we need to do a special check against empty strings. This will result in false cache hits when an option has been explicitly set to an empty string that is not meant to be falsey, which suggests that we should be fixing this on the way into the database (ie false should be converted to 0). But this latter suggestion would likely cause even more breakage, which is why I'm recommending limiting the empty-string/false equivalence only when doing the cache check. (The remaining option is to exclude booleans from the type juggling, which is what I suggested earlier. Anyone saving booleans through these functions is asking for trouble.)

[SergeyBiryukov]

#57894 was marked as a duplicate.

[domainsupport]

Is there any chance this patch can be signed off? We're getting many false negatives per day on one of our sites because of this bug which is causing un-necessary calls to $wpdb->insert() in update_option().

Is there a reason that this is still open 10 years later?

Thanks,

Oliver

[azaozz]

Related: #55942. Seems the patch needed for here would be different after the changes there (storing of PHP types in the DB) are implemented.

[flixos90]

@SergeyBiryukov Given there haven't been any updates here in 3 months and there's only the 8 year old patch, we should probably punt? Asking you since you originally milestoned the ticket.

[oglekler]

Due to lack of activity and potential regression it may have, I am moving this ticket into 6.4 for further consideration.

[oglekler]

This ticket was discussed during a bug scrub.

Because change of comparison can have huge side effects, we need dev feedback about this matter.

Add props to @mukesh27

In this PR i have updated 9 year old patch 22192.5.diff.

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

[mukesh27]

Hello there! I have just refreshed the 22192.5.diff​ patch, raised the PR, and updated the failed tests. I would appreciate some feedback on the approach.

Thanks @costdev and @felixarntz for the initial feedback. The PR is once again ready for review.

@costdev raised some backward compatibility (BC) issues that we coordinated and resolved. If there are any remaining issues, please share them so that we can address them promptly. It is essential to land this as soon as possible to allow sufficient time for testing.

[spacedmonkey]

@mukesh27 This ticket is designed to fix options and metadata, but your PR doesn't seem to update the metadata api. What is happening here?

Also what is happening about network options?

[spacedmonkey]

@mukesh27

@spacedmonkey

What about update_network_option? I believe this same fix should be applied here

​https://github.com/WordPress/wordpress-develop/blob/44e71c8e703bb473924e0d2bd35999d3aa17e734/src/wp-includes/option.php#L2108-L2119

I think increasing the scope of this PR to cover network options will unnecessarily block the rest of the work from going in. I know the Trac ticket also mentions metadata, but I think we should work on all those things in three iterations:

1. the foundation, plus options (this PR)
2. using the new function to check for metadata equality
3. using the new function to check for network options

[spacedmonkey]

As flagged on the ​PR.

@mukesh27 I recommend doing the following.

1. Change and title and description of this ticket to reflect that changes will only effect options.
2. Create two new tickets, for metadata and network options.
3. Remove changes from PR that affect network options.
4. Create new PR for network options and port over network options based on existing PR.

I agree with @flixos90 let's not grow the scope of this ticket. On the other hand, I think the network options change is simple tweak once the options are committed and should be worked on / commit in the same release.

[flixos90]

Thanks @spacedmonkey, using separate tickets for the 3 groups of changes makes sense to me. Let's trim this one down to only (site) options.

[spacedmonkey]

Related issues #59360 #59361

This adds only unit tests for ​https://github.com/WordPress/wordpress-develop/pull/5139 without changing any current behavior in order to demonstrate the current behavior against which any new fixes can be compared.

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

[joemcgill]

To help confirm the current behavior in trunk against the changes being worked on in ​PR #5139, I've opened ​a separate PR that includes unit tests that demonstrate which loosey values currently result in a DB update and which do not. It may be useful to commit a set of currently passing tests so we can more easily see what behavior will change as a result of a fix to this ticket.

@felixarntz @costdev @spacedmonkey and @joemcgill PR is ready for another round of review.

[flixos90]

In 56648:

Options, Meta APIs: Add further test coverage for comparison between old and new option value.

This ensures potential future changes to the logic are covered by existing tests that should pass before and after.

Props joemcgill.
See #22192.

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

@mukeshpanchal27 I just committed the side PR #5250 in https://core.trac.wordpress.org/changeset/56648.

Can you please refresh this branch against trunk to fix the merge conflict? Then we can reassess what the current behavior and tests look like.

@felixarntz @costdev @spacedmonkey and @joemcgill Looks like we have some failures now that the tests have been committed to trunk.

@mukeshpanchal27 I think test failures are to be expected since the code in this PR will change the current behavior. Can you review the failing tests and confirm whether they are failing because of an intended change in behavior (meaning we need to update the tests) or because the current code is introducing a regression (meaning we need to adjust the functionality)?

@mukeshpanchal27 I see you already updated the PR before I posted my feedback 😆

That's great, just one additional consideration would be to maybe add the assertions for the number of DB queries to the existing tests? Those were not covered in the existing tests, so if we want to ensure that remains the same, it's a good idea to add it. We shouldn't change anything else in those existing tests.

@felixarntz I try to check query on trunk but there is some inconsistency because of false-y test. Please check side PR ​https://github.com/WordPress/wordpress-develop/pull/5272

Thanks @mukeshpanchal27, I have left feedback on ​https://github.com/WordPress/wordpress-develop/pull/5272#pullrequestreview-1640368659.

@costdev In the interest of getting this committed before Beta 1 and not blocking based on #5272, would you be okay approving this as is, and keeping the ticket open for the follow up test coverage for the number of queries added?

I think other than not blocking this commit, it would also strike a great balance related to our comment exchange on #5272, where as part of the production logic change here, we wouldn't modify the tests much, ensuring things still pass as they were before.

@felixarntz If you're confident that the query-related test failures don't include a BC break, I'm okay with this being committed, then query-related test coverage being added afterwards (which should include investigation to confirm no BC breaks).

[flixos90]

In 56681:

Options, Meta APIs: Improve logic to avoid unnecessary database writes in update_option().

Prior to this change, a strict comparison between the old and new database value could lead to a false negative, since database values are generally stored as strings. For example, passing an integer to update_option() would almost always result in an update given any existing database value for that option would be that number cast to a string.

This changeset adjusts the logic to perform an intentional "loose-y" comparison by casting the values to strings. Extensive coverage previously added in [56648] provides additional confidence that this does not introduce any backward compatibility issues.

Props mukesh27, costdev, spacedmonkey, joemcgill, flixos90, nacin, atimmer, duck_, boonebgorges.
Fixes #22192.

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

[flixos90]

Reopening this to follow up with additional test coverage for the number of database queries made, see ​https://github.com/WordPress/wordpress-develop/pull/5272.

@mukesh27 @costdev That PR can probably be refreshed with the latest trunk and then we can follow up on the conversation, hopefully committing within the next 2 weeks.

[Mamaduka]

Posting here for visibility. Gutenberg e2e test caught regression after r56681.

Details: ​https://github.com/WordPress/gutenberg/pull/54806#issuecomment-1734840171.

[mukesh27]

Thank you, @Mamaduka, for bringing this to attention.

I have also encountered the same error on the trunk version. I will investigate it further and provide an update. Thanks!

[flixos90]

@Mamaduka @mukesh27 As far as I can tell, I can think of two potential solutions to the problem:

• Either we temporarily remove any pre_option_{$option} filters before getting the option from the database within update_option(), in order to get the actual option. The value won't be otherwise used, so I think that could be fine.
• Or we make an actual database query to see whether the value exists in the database. That sounds like a more reliable solution to the underlying problem (checking a default isn't ever going to be fully reliable), however it could introduce other unintended side effects.

I'm personally leaning towards the first so far since it would be a simpler change.

This implements my first suggestion from https://core.trac.wordpress.org/ticket/22192#comment:55, addressing the follow up bug for options with pre filters.

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

[Mamaduka]

@flixos90

Comparing against raw DB values would be more suitable for the new logic, but it's hard to anticipate side effects.

Both are valid options (no pun intended), and I would defer to your choice here.

Manually tested with repro steps from ​https://github.com/WordPress/gutenberg/pull/54806#issuecomment-1734840171, and couldn't reproduce the bug.

The code looks good to me, but getting a second opinion on it would be nice.

[flixos90]

In 56717:

Options, Meta APIs: Fix follow up bug when comparing values for options using the pre_option_{$option} filter.

This fix is relevant for options such as gmt_offset that use a filter to force a specific value regardless of what is stored in the database.

Props mamaduka, flixos90, mukesh27, spacedmonkey.
See #22192.

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

This ticket will add query check for the update_option() function.

@mukeshpanchal27 and @costdev, thanks for digging into this and for providing such a thorough explanation of the issues you've discovered while investigating the unexpected behavior with the initial set of query tests that were added in ​fe71ba1.

There's a lot to unpack in this thread, but to summarize—for each use case we now need to test for three distinct things:

1. Does update_option result in true or false? This is what our initial set of test cases were put in place to check.
2. Is an additional DB request being made when update_option is false? Our hypothesis, which has been proven false, is that no additional DB requests would be made if the option was not being updated.
3. Should any additional DB request be expected, or are they a bug (i.e., should any of these queries be avoided)? More on this below...

The cases where we're confirming that update_option() returns false, but resulting in an additional DB query anyway, seems like the place where we _might_ want to add logic to avoid those queries. However, reviewing all of the changes that have been made in this PR, you've updated the test cases AND the logic, which makes it really difficult to confirm what is being changed and why. So I am agreement with what @felixarntz mentioned in his ​previous comment.

Looking back at the original commit that added query count checks, I noticed that there were some flaws in the tests themselves that were contributing to the high number of the failures. I've created ​this PR which goes back to the original tests that @mukeshpanchal27 added, but then fixes the way query counts were being applied, and am only seeing 9 unexpected failures—all of which have to do with the unique behavior you observed for when the initial option is set to either false or null.

I'd be much more confident at this point in adjusting the test_update_loosey_options() test method to account for these additional queries, rather than refactoring all of the original test cases we added. I think it leads to much more confidence in the changes we're making and avoiding unintentional regressions.

Thanks @joemcgill!

Looking back at the original commit that added query count checks, I noticed that there were some flaws in the tests themselves that were contributing to the high number of the failures.

Can you elaborate on this?

I think part of the issue is that the tests currently in trunk don't check for the number of expected queries, but the logic changes to update_option() are also now in trunk.

It may be prudent to add the query count checks to the tests in trunk first, so that any changes this PR makes are only to _adjust_ the query count checks, rather than to introduce them.

However, the query count checks need to be based on trunk _before_ the logic changes to update_option() were committed (i.e. 6.3's update_option()). Otherwise, we're protecting the new logic, not BC.

I'm finding it difficult to write the query count checks for trunk's tests as so much is covered in one test method, while there's actually more going on under the hood in update_option() - hence splitting up the tests. Given this, @joemcgill would you be willing to write the checks for query counts for the tests in trunk, based on 6.3's update_option()?

@joemcgill Thanks for your review. I have open ​PR 5362 with the query check that you did in your ​PR 5356 it result in ​78 failures in query checks.

@costdev:

Can you elaborate on this?

Sure! The original query count checks that were added in fe71ba1 didn't account for the fact that when the options cache is deleted, that update_option() will always run a query to get the previous value before performing the update. I adjusted the logic in those tests in ​https://github.com/WordPress/wordpress-develop/pull/5356/commits/5aa21d5f36b1c1c6f7e1354efbeb9321a5808e2a in my PR to account for this expected behavior.

I think part of the issue is that the tests currently in trunk don't check for the number of expected queries, but the logic changes to update_option() are also now in trunk.

That's true. To see what the previous behavior was prior to the changes committed in r56681 and r56717, I branched from when the initial tests were added (6b50b1b) and then applied the unit query count tests (cherry picking 4521a53 and 03e0241) and am seeing nearly the same result that Mukesh demonstrated in ​https://github.com/WordPress/wordpress-develop/pull/5362. I've pushed that branch to my fork so you can see ​the comparison, but you'd have to run the tests locally to see the output, since the GH actions have to run against a specific branch of the repo and not against an arbitrary commit.

Prior to the changes we've committed, I'm seeing 74 test failures due to queries happening when we expect them not to, but now that those changes are in trunk, we're only seeing 9 failures. What I think that demonstrates, is that the changes we've committed have fixed a majority of the cases where extra queries were being made when they didn't need to be. I.e., this demonstrates that the bug is fixed in many cases, rather than showing a back-compat break, unless I'm missing something.

I think the next steps would be to adjust the PR that I started in ​https://github.com/WordPress/wordpress-develop/pull/5356 so that the 9 cases that are still failing are accounted for. This should happen by either:

1. Updating the logic in those tests so they expect the extra queries in these 9 cases
2. Fixing the update_option logic so those extra queries are no longer being made.

I've not looked deep enough into why those cases are failing to know whether those queries should be expected or if they can be avoided without additional side effects.

Hopefully this helps? Let me know if anything is unclear.

Thanks @joemcgill! That helps!

---

Just noting here as clarification on the BC break that was revealed while working on the tests: It's actually a regression rather than a BC break. My bad on that.

When running update_option( false, false ), for example:

• 6.3: Since both the old and new value are strictly equal, update_option() returned false without running an additional query.
• trunk: An additional query runs to try to add the option.
  • As the old and new value are considered the same by the database, this results in 0 affected rows, which add_option() catches and returns false. In turn, update_option() returns false too.
  • The original tests passed because they only checked assertFalse( update_option( false, false ) ), so they didn't reveal the additional query that previously didn't happen. Since adding test coverage for this additional query would be protecting the regression, we instead resolved it with ​this commit.

---

I think the next steps would be to adjust the PR that I started in ​https://github.com/WordPress/wordpress-develop/pull/5356 so that the 9 cases that are still failing are accounted for. This should happen by either:

• Updating the logic in those tests so they expect the extra queries in these 9 cases
• Fixing the update_option logic so those extra queries are no longer being made.

I've not looked deep enough into why those cases are failing to know whether those queries should be expected or if they can be avoided without additional side effects.

Resolving those 9 cases using that single test method may be tricky/awkward/messy.

Though to start off, I'd suggest adding ​this commit to PR #5356 which resolves the regression. You'll see that the failure count increases from 9 to 10, but with some different tests failing. test_update_option_should_add_option_with_filtered_default_value, for example, was protecting the regression by expecting the option to be added despite strictly equal old and new values. This should be adjusted to pass the "default" default value of false to update_option().

Overall though, I do feel that we've become too reliant on the incomplete tests we have in trunk, which asserted based on an incorrect association regarding update_option()'s return value and the query count. By continuing with that one test method, I feel we'd be trying to do too much in one test method just to recover from that mistake. While I'll go with consensus here, I'd much prefer if you could split up the tests from #5356 to update_option_should_update(), update_option_should_not_update(), update_option_should_add_option(), etc. and split the data provider up, ensuring that all of #5356's test cases are covered. We're testing multiple behaviours with one test method, which feels wrong and problematic for clarity and maintenance. It was different when we were in a rush towards Beta 1 and thought the tests were sound, but now that we know they're not, there's time to get the tests in better shape.

Thanks @costdev. I think I'm on the same page as you now.

I can see from @mukeshpanchal27's PR that an extra query was being made in 6.3 when updating from false to false when updating without a primed cache (​example) and also when the cache has been refreshed from the DB (​example). Now in trunk, those extra queries are not being made, but an extra query _is_ being made if the option is already in cache from the first time it's set (​example). Addressing this regression via ​this commit seems sensible to me. I don't see any other new failures as a result of the changes in trunk when cross referencing ​this PR with the ones @mukeshpanchal27 added in ​https://github.com/WordPress/wordpress-develop/pull/5362

As far as splitting out the unit tests are concerned, I agree that we're making each test do too much and would be happy with a refactor as long as we're only changing tests and not tests AND functionality in the same PR, because then it's unclear how the logic change relates to the behavior that was previously being confirmed by the tests—which was the main concern I had in ​this comment.

Personally, I'd rather separate the unit tests so that we're not mixing the update_option return value and the query counts logic in the same assertion, rather than separating tests and data providers based on whether or not they produce an update, but I don't feel strongly about which strategy is used.

Seems like the most important things to address are removing the extra query you identified and fixing the new tests that were added for filtered default values that were based on this regression. Agreed?

This PR is an alternate approach to ​https://github.com/WordPress/wordpress-develop/pull/5272, which updates the unit tests to account for query counts without refactoring all of the tests and associated data providers. For context and initial discussion, see ​this comment and the following conversation.

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

[flixos90]

In 56762:

Options, Meta APIs: Implement additional tests covering update_option().

As a follow up to [56681], this changeset adds further tests, primarily focused on ensuring no unnecessary database queries are run.

Props mukesh27, costdev, joemcgill, spacedmonkey.
See #22192.

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

@costdev @mukeshpanchal27 So this is now unblocking a follow up PR to further consider the production code change.

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

Check previous discussion here ​https://github.com/WordPress/wordpress-develop/pull/5272#issuecomment-1740347838 for more detials.

@costdev Fair point. In this case I'd say though it's not crucial enough to make the changes to keep the ticket open for yet another PR. Let's just close it after committing this fix.

Thanks @felixarntz and @costdev for the feedbacks. Do i need to open followup PR for 6.4 or we can do it later?

[flixos90]

In 56788:

Options, Meta APIs: Fix minor compatibility issue with update_option() change.

When calling update_option() with value false on a non-existent option, prior to [56681] the function would have returned false and not stored the value in the database, since the given value was the same as the default.

The aforementioned changeset broke that promise with good intention, however this particular change was a backward compatibility break and therefore is resolved here.

Props mukesh27, costdev.
Fixes #22192.

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

[snicco]

Replying to flixos90:

@Mamaduka @mukesh27 As far as I can tell, I can think of two potential solutions to the problem:

• Either we temporarily remove any pre_option_{$option} filters before getting the option from the database within update_option(), in order to get the actual option. The value won't be otherwise used, so I think that could be fine.
• Or we make an actual database query to see whether the value exists in the database. That sounds like a more reliable solution to the underlying problem (checking a default isn't ever going to be fully reliable), however it could introduce other unintended side effects.

I'm personally leaning towards the first so far since it would be a simpler change.

Removing the pre_option_{$option} filter is a BC break for any functionality that relies on consistently short-circuiting the value returned by get_option.

This breaks everything we do in [Vaults&Pillars](​https://snicco.io/blog/introducing-fortress-vaults-and-pillars)

[flixos90]

Replying to snicco:

This breaks everything we do in [Vaults&Pillars](​https://snicco.io/blog/introducing-fortress-vaults-and-pillars)

Can you provide a bit more detail? What do you expect get_option() to do? And how does it not do that anymore since this change? If you can also provide an example, that would be really helpful.

In update_option(), pre_option_{$option} filters are currently removed before getting the "raw" old value, then restored afterwards.

This change performs the same removal/restoration with pre_option filters as well.

[costdev]

Hey @mukesh27 @flixos90 @joemcgill I was taking an early look at some possible alternative approaches for the pre-filters "toggle" in update_option()/update_network_option() and on my travels, a thought occurred about our current approach.

We're currently "toggling" pre_option_{$option} filters so we can get the raw value. What if someone filters the newer global pre_option instead? This is straightforward to add a "toggle" for, and we can use a data provider of pre-filter hook names for the existing tests ​PR 5448 implements this.

With the changes to update_network_option() now committed ([56814] / #59360), we'll need to follow up to do the same in update_network_option(), with any additional considerations that may be necessary. I'm AFK for a lot of today, so I won't get a chance to prep a PR for update_network_option() this before 6.4 Beta 3, but can follow up this evening.

For now, can you take a look at ​PR 5448 and see if we can get it into 6.4 Beta 3? Thanks!

[snicco]

Replying to flixos90:

Replying to snicco:

This breaks everything we do in [Vaults&Pillars](​https://snicco.io/blog/introducing-fortress-vaults-and-pillars)

Can you provide a bit more detail? What do you expect get_option() to do? And how does it not do that anymore since this change? If you can also provide an example, that would be really helpful.

We extensively use the pre_get_option filter to insert an encryption layer between the WordPress database, where options are stored, and between the runtime values passed to PHP.

This behavior is expected to be consistent from the outside.

Different calls to get_option() must not return different values.

That's manipulating the internal "state" from get_option from the outside and is no different than manipulating private properties with reflection.

I'm only sharing ​https://snicco.io/blog/introducing-fortress-vaults-and-pillars as one obvious example, but who knows in what other ways this might break other dependents.

@felixarntz @costdev This was by design ​https://github.com/WordPress/wordpress-develop/pull/5313#discussion_r1337338721

@costdev Per what @spacedmonkey said, I don't think we should remove the _generic_ pre_option (or pre_site_option) filters. The idea is that those could be applied for general changes, such as _never_ hitting the DB for options or something like that. Unhooking those could be severe for special setups like that. Unhooking a specific pre filter though should be okay, given that those would be used only on specific options, thus it would be reasonable to expect the DB isn't bypassed entirely.

[flixos90]

Replying to snicco:

We extensively use the pre_get_option filter to insert an encryption layer between the WordPress database, where options are stored, and between the runtime values passed to PHP.

For such behaviors, where general changes to fetching any options are made, the pre_option filter should be used, which is intentionally not modified here. pre_option_{$option} is for specific options, so it has different implications.

Different calls to get_option() must not return different values.

While I wish this was true, realistically it isn't the case, since with WordPress's filter system any call may return something different.

@costdev I think for the above reason we shouldn't do what you proposed in ​https://github.com/WordPress/wordpress-develop/pull/5448.

@felixarntz @spacedmonkey Yep no worries. I didn't realise that this was already discussed.

[snicco]

Replying to flixos90:

Sorry, made I a typo.

We use pre_option_{$option}, not the catch-all filter to avoid running the hook callback for any "non-intersting" option.

While I wish this was true, realistically it isn't the case, since with WordPress's filter system any call may return something different.

Well I guess, but there is an official API for removing filters. And it doesn't allow you to add them back either.

The code in question is a hackaround, modifying the internals of the global hooks.

[spacedmonkey]

Instead of removing filters, could we do something like this.

        $alloptions = wp_load_alloptions();
        if ( isset( $alloptions[ $option_name ] ) ) {
            $raw_option = $alloptions[ $option_name ];
        } else{
           ....current code to unhook option filters...
        }

[flixos90]

Sharing here the suggestion by @joemcgill from ​https://wordpress.slack.com/archives/C02KGN5K076/p1697034922980339, which I think is a great alternative solution: If any pre filters are present, we should play it safe and stick to the previous logic.

Effectively, this means for options with pre filters, the original purpose of this ticket won't get fixed. But I think that is a better trade off than the breakage we're seeing from the workaround of temporarily unhooking filters, and it's overall an edge-case anyway. So I am in favor of going that route.

[joemcgill]

I'm working on some approaches to avoid juggling the pre_option_${option} filters, so reassigning to myself for now.

[azaozz]

Replying to flixos90:

this means for options with pre filters, the original purpose of this ticket won't get fixed. But I think that is a better trade off than the breakage we're seeing from the workaround of temporarily unhooking filters, and it's overall an edge-case anyway.

+1. Ran into this when working on the patch to add data type support for options. The "proper" solution seems to be to trust that the plugins that use the pre_ filters will do the right thing :)

For demonstration only – not intended for merge
This set of unit tests are created from the 6.3 branch in order to confirm the previous behavior of update_option() when the old option value has been pre-filtered. These same behaviors should be protected as part of the updates in 6.4.

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

This removes the logic that disables pre-filteres when deciding whether to updating options and network options

See: 22192, 59360.

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

@mukeshpanchal27 I'm not sure we would want to revert _all_ of these changes if we can't get to a solution for https://core.trac.wordpress.org/ticket/59360 in time. As far as I'm aware, the outstanding problem is only occurring on the network option ticket, so maybe we would only need to revert that?

As far as I'm aware, the outstanding problem is only occurring on the network option ticket, so maybe we would only need to revert that?

Are you sure that we could keep the update_option() changes? update_network_option() relies on update_option() if the function is called on a single site install. Does the bug only affect multisite?

@joemcgill I'm referring to https://core.trac.wordpress.org/ticket/59360#comment:20, which was only flagged on that ticket. So I'm not sure whether that would also be a problem on 22192.

I attempted to add the unit tests from ​https://github.com/WordPress/wordpress-develop/pull/5250, which verified the original behavior, but this is causing a strange error with the dataProvider, which I'm having trouble understanding. I've reverted those changes so this is just a straight revert—not a revert and add tests. I'm thinking I can commit the revert and we can adjust and add tests in a follow-up commit.

Preferably, the tests we add could more closely resemble the ones being removed in this revert, but will need to have the dataProviders updated.

@joemcgill Looking at ​https://github.com/WordPress/wordpress-develop/actions/runs/6538297235/job/17753926355, seems like a weird bug indeed. I can't see anywhere in data_update_option_type_juggling() that there would be only one value in the data array.

That said, rather than adding different tests (as in ​https://github.com/WordPress/wordpress-develop/pull/5497/commits/77ec8657e95736bcc966f3835c247c63ed7aad94), I think it would be great to maintain as much of the existing core tests as possible. We'll probably mostly have to update some data provider values to reflect the expected behavior _before_ our code changes, but the approach of those tests still looks good to me.

@felixarntz

We'll probably mostly have to update some data provider values to reflect the expected behavior before our code changes, but the approach of those tests still looks good to me.

That's exactly what I was getting at with my comment:

Preferably, the tests we add could more closely resemble the ones being removed in this revert, but will need to have the dataProviders updated.

I'd prefer to add back those unit tests as a separate task, in order to unblock this revert. Sound good?

@joemcgill SGTM! Also see my updated reply above.

Exactly. Rather than trying to rewrite all of the tests now, I was just trying to get us back to the state we were in before any code changes were made, since our original tests got refactored during the implementation. However, those odd test errors (which I can reproduce locally, but don't understand) made me rethink trying to do both the revert and test update in the same commit.

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

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

[joemcgill]

In 56947:

Options, Meta APIs: Delete leftover unit tests.

This is a followup to [56946] to commit the deletion of a leftover PHP Unit file.

See #22192.

[joemcgill]

#59360 was marked as a duplicate.

Closing this now that we've chosen to revert the changes from the 6.4 release cycle.

[joemcgill]

After spending time testing ​PR #5498, which reintroduced the gmt_offset issues that were originally reported by @Mamaduka here—and after confirming with others who worked on this ticket, I decided to revert all of these changes.

The behavior we initially confirmed in [56648] is that many loosely equal values were already not affected by this bug. However, some loosely equal falsey values were. I'm beginning to think that the previous behavior needs to be maintained.

What is happening with the gmt_offset issue is that there are many places in Core that expects the value of get_option( 'gmt_offset' ) to be an (int) when calculating timestamps. When you set the timezone to UTC+0, the timezone_string option is deleted and update_option( 'gmt_offset', 0 ) is passed. Previously, this worked as expected, but if loosely equal values are not updated then this update would get skipped, causing the option to remain empty in the DB. Then, code like current_time( 'timestamp' ) will end up trying to multiply a string '' with HOUR_IN_SECONDS, which throws a warning. In other places in Core, this causes a fatal (​see example).

We could work around these edge cases by adding an additional filter that casts this specific option to an (int), but I suspect there are other places where a similar problem could cause errors if we're not allowing an empty value to be updated in the DB—particularly when there is a pre-filter in play that returns a loosely equivalent value, as is the case with gmt_offset.

Given the timeline, I'm punting this to a future release. If this is attempted again, we would need to either check for updates against the raw DB value (not just what get_option() returns) or we should likely close this as wontfix.

Regardless, adding additional unit tests confirming the current behavior should be committed, and could be backported to the 6.4 branch during the RC phase if ready.