Opened 12 years ago
Last modified 13 months ago
#22192 assigned defect (bug)
update_option() strict checks can cause false negatives
Reported by: | nacin | Owned by: | 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 )
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)
Change History (117)
#3
follow-up:
↓ 4
@
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.
#4
in reply to:
↑ 3
@
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 toTests_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)
#5
@
11 years ago
22192.2.diff updates the test with the notes of duck_.
#8
@
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.
#9
@
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
@
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
#14
@
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
#16
@
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
@
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
@
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
@
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
@
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
@
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?
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
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:
- the foundation, plus options (this PR)
- using the new function to check for metadata equality
- using the new function to check for network options
#31
@
14 months ago
As flagged on the PR.
@mukesh27 I recommend doing the following.
- Change and title and description of this ticket to reflect that changes will only effect options.
- Create two new tickets, for metadata and network options.
- Remove changes from PR that affect network options.
- 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
@
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
@
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
@
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.
@mukesh27 commented on PR #5139:
14 months ago
#38
@felixarntz @costdev @spacedmonkey and @joemcgill PR is ready for another round of review.
@flixos90 commented on PR #5250:
14 months ago
#40
Committed in https://core.trac.wordpress.org/changeset/56648
@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
#46
Thanks @mukeshpanchal27, I have left feedback on https://github.com/WordPress/wordpress-develop/pull/5272#pullrequestreview-1640368659.
@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.
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).
@flixos90 commented on PR #5139:
14 months ago
#50
Committed in https://core.trac.wordpress.org/changeset/56681
#51
@
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
@
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
@
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:
↓ 79
@
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 withinupdate_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 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
@
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.
@flixos90 commented on PR #5313:
14 months ago
#60
Committed in https://core.trac.wordpress.org/changeset/56717
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:
- Does
update_option
result intrue
orfalse
? This is what our initial set of test cases were put in place to check. - Is an additional DB request being made when
update_option
isfalse
? Our hypothesis, which has been proven false, is that no additional DB requests would be made if the option was not being updated. - 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.
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?
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:
- 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.
Hopefully this helps? Let me know if anything is unclear.
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()
returnedfalse
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 returnsfalse
. In turn,update_option()
returnsfalse
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.
- As the old and new value are considered the same by the database, this results in 0 affected rows, which
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
@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.
This ticket was mentioned in PR #5394 on WordPress/wordpress-develop by @mukesh27.
13 months ago
#73
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.
@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
@flixos90 commented on PR #5394:
13 months ago
#78
Committed in https://core.trac.wordpress.org/changeset/56788
#79
in reply to:
↑ 55
;
follow-up:
↓ 80
@
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 withinupdate_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)
#80
in reply to:
↑ 79
;
follow-up:
↓ 83
@
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
@
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:
↓ 86
@
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.
@spacedmonkey commented on PR #5448:
13 months ago
#84
@felixarntz @costdev This was by design https://github.com/WordPress/wordpress-develop/pull/5313#discussion_r1337338721
@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:
↓ 88
@
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.
13 months ago
#87
@felixarntz @spacedmonkey Yep no worries. I didn't realise that this was already discussed.
#88
in reply to:
↑ 86
@
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
@
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:
↓ 93
@
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
@
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
@
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 :)
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.
joemcgill commented on PR #5497:
13 months ago
#107
Committed in https://core.trac.wordpress.org/changeset/56946
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
@
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.
Related: #20712