Make WordPress Core

Opened 12 years ago

Last modified 13 months ago

#22192 assigned defect (bug)

update_option() strict checks can cause false negatives

Reported by: nacin's profile nacin Owned by: joemcgill's profile joemcgill
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: needs-patch needs-unit-tests
Focuses: performance Cc:

Description (last modified by mukesh27)

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.

Attachments (6)

22192.1.diff (1.7 KB) - added by atimmer 11 years ago.
22192.2.diff (1.7 KB) - added by atimmer 11 years ago.
22192.diff (2.0 KB) - added by boonebgorges 10 years ago.
22192.3.diff (2.7 KB) - added by boonebgorges 10 years ago.
22192.4.diff (3.6 KB) - added by boonebgorges 10 years ago.
22192.5.diff (4.2 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (117)

#1 @knutsp
12 years ago

  • Cc knut@… added

#3 follow-up: @atimmer
11 years ago

  • Cc atimmermans@… added

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.

@atimmer
11 years ago

#4 in reply to: ↑ 3 @duck_
11 years ago

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
11 years ago

#5 @atimmer
11 years ago

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

#6 @nacin
11 years ago

  • Component changed from Performance to Options and Meta
  • Focuses performance added

#8 @boonebgorges
10 years ago

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
10 years ago

#9 @boonebgorges
10 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch needs-unit-tests removed

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.

#10 @boonebgorges
10 years ago

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

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by boone. View the logs.


8 years ago

#13 @SergeyBiryukov
20 months ago

#57894 was marked as a duplicate.

#14 @domainsupport
20 months ago

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

#15 @SergeyBiryukov
20 months ago

  • Milestone set to 6.3

#16 @azaozz
20 months ago

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

#17 @flixos90
17 months ago

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

#18 @oglekler
16 months ago

  • Milestone changed from 6.3 to 6.4

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

This ticket was mentioned in Slack in #core by oglekler. View the logs.


14 months ago

#20 @oglekler
14 months ago

  • Keywords dev-feedback added

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

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


14 months ago

This ticket was mentioned in PR #5139 on WordPress/wordpress-develop by @mukesh27.


14 months ago
#22

  • Keywords has-unit-tests added

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

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

#23 @mukesh27
14 months ago

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.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


14 months ago

@mukesh27 commented on PR #5139:


14 months ago
#25

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

@mukesh27 commented on PR #5139:


14 months ago
#26

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

#27 @spacedmonkey
14 months ago

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

#28 @spacedmonkey
14 months ago

  • Owner set to mukesh27
  • Status changed from new to assigned

@mukesh27

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


14 months ago

@flixos90 commented on PR #5139:


14 months ago
#30

@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

#31 @spacedmonkey
14 months ago

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.

#32 @flixos90
14 months ago

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.

This ticket was mentioned in Slack in #core-performance by spacedmonkey. View the logs.


14 months ago

#34 @mukesh27
14 months ago

  • Description modified (diff)
  • Summary changed from update_metadata() and update_option() strict checks can cause false negatives to update_option() strict checks can cause false negatives

This ticket was mentioned in PR #5250 on WordPress/wordpress-develop by joemcgill.


14 months ago
#36

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

#37 @joemcgill
14 months ago

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.

Last edited 14 months ago by joemcgill (previous) (diff)

@mukesh27 commented on PR #5139:


14 months ago
#38

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

#39 @flixos90
14 months ago

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.

@flixos90 commented on PR #5139:


14 months ago
#41

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

@mukesh27 commented on PR #5139:


14 months ago
#42

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

joemcgill commented on PR #5139:


14 months ago
#43

@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)?

@flixos90 commented on PR #5139:


14 months ago
#44

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

@mukesh27 commented on PR #5139:


14 months ago
#45

@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

@flixos90 commented on PR #5139:


14 months ago
#47

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

@costdev commented on PR #5139:


14 months ago
#48

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

#49 @flixos90
14 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#51 @flixos90
14 months ago

  • Keywords 2nd-opinion dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#52 @Mamaduka
14 months ago

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

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

#53 @mukesh27
14 months ago

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!

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


14 months ago

#55 follow-up: @flixos90
14 months ago

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

Last edited 14 months ago by flixos90 (previous) (diff)

This ticket was mentioned in PR #5313 on WordPress/wordpress-develop by @flixos90.


14 months ago
#56

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

#57 @Mamaduka
14 months ago

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

@Mamaduka commented on PR #5313:


14 months ago
#58

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.

#59 @flixos90
14 months ago

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.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


13 months ago

This ticket was mentioned in PR #5272 on WordPress/wordpress-develop by @mukesh27.


13 months ago
#62

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

joemcgill commented on PR #5272:


13 months ago
#63

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

@costdev commented on PR #5272:


13 months ago
#64

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?

@costdev commented on PR #5272:


13 months ago
#65

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()?

@mukesh27 commented on PR #5272:


13 months ago
#66

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

joemcgill commented on PR #5272:


13 months ago
#67

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

@costdev commented on PR #5272:


13 months ago
#68

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.

joemcgill commented on PR #5272:


13 months ago
#69

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 ticket was mentioned in PR #5356 on WordPress/wordpress-develop by joemcgill.


13 months ago
#70

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

#71 @flixos90
13 months ago

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.

@flixos90 commented on PR #5272:


13 months ago
#72

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.

@flixos90 commented on PR #5394:


13 months ago
#74

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

@mukesh27 commented on PR #5394:


13 months ago
#75

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

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


13 months ago

#77 @flixos90
13 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#79 in reply to: ↑ 55 ; follow-up: @snicco
13 months ago

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)


Last edited 13 months ago by snicco (previous) (diff)

#80 in reply to: ↑ 79 ; follow-up: @flixos90
13 months ago

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.

This ticket was mentioned in PR #5448 on WordPress/wordpress-develop by @costdev.


13 months ago
#81

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.

#82 @costdev
13 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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!

#83 in reply to: ↑ 80 ; follow-up: @snicco
13 months ago

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.

@flixos90 commented on PR #5448:


13 months ago
#85

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

#86 in reply to: ↑ 83 ; follow-up: @flixos90
13 months ago

  • Keywords close added

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.

@costdev commented on PR #5448:


13 months ago
#87

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

#88 in reply to: ↑ 86 @snicco
13 months ago

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.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


13 months ago

#90 @spacedmonkey
13 months ago

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

#91 follow-up: @flixos90
13 months ago

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.

#92 @joemcgill
13 months ago

  • Owner changed from mukesh27 to joemcgill
  • Status changed from reopened to assigned

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

#93 in reply to: ↑ 91 @azaozz
13 months ago

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 :)

Last edited 13 months ago by azaozz (previous) (diff)

This ticket was mentioned in PR #5487 on WordPress/wordpress-develop by joemcgill.


13 months ago
#94

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 ticket was mentioned in PR #5497 on WordPress/wordpress-develop by @mukesh27.


13 months ago
#95

This ticket was mentioned in PR #5498 on WordPress/wordpress-develop by joemcgill.


13 months ago
#96

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

@flixos90 commented on PR #5497:


13 months ago
#97

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

joemcgill commented on PR #5497:


13 months ago
#98

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?

@flixos90 commented on PR #5497:


13 months ago
#99

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

This ticket was mentioned in Slack in #core by nicolefurlan. View the logs.


13 months ago

joemcgill commented on PR #5497:


13 months ago
#101

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.

@flixos90 commented on PR #5497:


13 months ago
#102

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

joemcgill commented on PR #5497:


13 months ago
#103

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

@flixos90 commented on PR #5497:


13 months ago
#104

@joemcgill SGTM! Also see my updated reply above.

joemcgill commented on PR #5497:


13 months ago
#105

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.

#106 @joemcgill
13 months ago

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.

#108 @joemcgill
13 months ago

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.

#109 @joemcgill
13 months ago

#59360 was marked as a duplicate.

joemcgill commented on PR #5498:


13 months ago
#110

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

#111 @joemcgill
13 months ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests close removed
  • Milestone changed from 6.4 to Future Release

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.

Note: See TracTickets for help on using tickets.