update_option() strict checks can cause false negatives
#5139
Conversation
mukeshpanchal27
requested review from
SergeyBiryukov,
spacedmonkey,
boonebgorges,
joemcgill,
felixarntz and
costdev
There was a problem hiding this comment.
Thanks for the PR @mukeshpanchal27! I've left some test-related tidy-up comments in this review. 🙂
There was a problem hiding this comment.
@mukeshpanchal27 Looks good for the most part, one point of feedback.
|
Thanks @costdev and @felixarntz for the initial feedback. The PR is once again ready for review. |
There was a problem hiding this comment.
@mukeshpanchal27 This looks solid now, the only thing that would be great to add is a reusable function for the duplicate code per @spacedmonkey's feedback.
Also see #5139 (comment), and then it would be great to add a single unit test for it with a data provider to check different values and their comparisons.
|
@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. |
|
@mukeshpanchal27 I just committed the side PR #5250 in https://core.trac.wordpress.org/changeset/56648. Can you please refresh this branch against |
|
@felixarntz @costdev @spacedmonkey and @joemcgill Looks like we have some failures now that the tests have been committed to trunk. |
| * @param mixed $old_value One of the values to compare. | ||
| * @param mixed $new_value The other value to compare. | ||
| */ | ||
| public function test_update_option_should_hit_cache_when_loosely_equal_to_existing_value_and_cached_values_are_faithful_to_original_type( $old_value, $new_value ) { |
There was a problem hiding this comment.
Now that a set of tests confirming current behavior are in trunk, via https://core.trac.wordpress.org/changeset/56648, is this test still needed?
|
@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. |
There was a problem hiding this comment.
Looking at the test failures in https://github.com/WordPress/wordpress-develop/actions/runs/6264701987/job/17011971702?pr=5139, I think there is a problem with the logic here because it incorrectly forces values into certain results that is not the same as how they will be interpreted in the DB.
Reviewing a few test failures as examples:
- https://github.com/WordPress/wordpress-develop/actions/runs/6264701987/job/17011971702?pr=5139#step:14:328 should pass since those values evaluate differently as strings in the DB (
'0'vs''). - https://github.com/WordPress/wordpress-develop/actions/runs/6264701987/job/17011971702?pr=5139#step:14:336 should pass since those values evaluate differently as strings in the DB (
''vs'0'). - https://github.com/WordPress/wordpress-develop/actions/runs/6264701987/job/17011971702?pr=5139#step:14:344 should pass since those values evaluate differently as strings in the DB (
''vs'0'). - https://github.com/WordPress/wordpress-develop/actions/runs/6264701987/job/17011971702?pr=5139#step:14:352 should pass since those values evaluate differently as strings in the DB (
'0'vs'').
There was a problem hiding this comment.
Thanks for the updates @mukeshpanchal27! Awesome job everyone on protecting BC here!
Just some final docs feedback in this review.
There was a problem hiding this comment.
@mukeshpanchal27 Thank you for the updates, this is looking great now. Marking as approved already.
It would be great if you could incorporate my previous feedback from #5139 (comment) (add assertions for the number of queries to the tests from https://core.trac.wordpress.org/changeset/56648), to ensure in the correct number of database requests is sent. This is especially beneficial for the test cases where the option should not be updated, because only asserting the result of false may also come from another part of the logic within the function.
Co-authored-by: Colin Stewart <[email protected]>
|
@felixarntz I try to check query on trunk but there is some inconsistency because of false-y test. Please check side PR #5272 |
|
Thanks @mukeshpanchal27, I have left feedback on #5272 (review). |
|
@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). |
|
Committed in https://core.trac.wordpress.org/changeset/56681 |
In this PR i have updated 9 year old patch 22192.5.diff.
Trac ticket: https://core.trac.wordpress.org/ticket/22192
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.