Options: Remove pre-filter juggling #5498
Conversation
… options See: 22192, 59360.
| add_filter( 'pre_option_foo', '__return_false' ); | ||
| add_filter( 'pre_site_option_foo', '__return_false' ); |
There was a problem hiding this comment.
Should these use the pattern used in other tests here?
$hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo';
There was a problem hiding this comment.
I don't feel strongly about this, but I think having both filters potentially helps uncover future weirdness in behavior more reliably than only having one of them set.
Alternatively, we duplicate this test and have both versions covered (one test with both filters, the other with only the one appropriate filter).
| /* | ||
| * When the network option is equal to the filtered version, update option will bail early. | ||
| * Otherwise, The pre-filter will make the old option `false`, which is equal to the | ||
| * default value. This causes an add_network_option() to be triggered. | ||
| */ |
There was a problem hiding this comment.
Not sure this comment is accurate, as this was copy/pasta from the update_option tests.
| * This should succeed, since the 'foo' option does not exist in the database. | ||
| * The default value is false, so it differs from 0. |
| * This should succeed, since the 'foo' option has a value of 1 in the database. | ||
| * Therefore it differs from 0 and should be updated. |
| * If the option will result in the same DB value, the option should not | ||
| * be updated. Otherwise, the option should be updated regardless of the prefilter. | ||
| */ | ||
| if ( _is_equal_database_value( $option, true ) ) { |
There was a problem hiding this comment.
Not sure if it's ok to use the helper function here, or if that makes the tests susceptible to side effects from changes to that business logic. Could also explicitly list the values we expect to be equal in an array and run in_array() on the $option value.
There was a problem hiding this comment.
@joemcgill This looks solid so far, though we should triple check we don't reintroduce the same bug with the gmt_offset option that the pre filter juggling originally fixed here. Or have we come to the conclusion that this bug somehow is invalid / wontfix? Let's clarify that before proceeding.
| add_filter( 'pre_option_foo', '__return_false' ); | ||
| add_filter( 'pre_site_option_foo', '__return_false' ); |
There was a problem hiding this comment.
I don't feel strongly about this, but I think having both filters potentially helps uncover future weirdness in behavior more reliably than only having one of them set.
Alternatively, we duplicate this test and have both versions covered (one test with both filters, the other with only the one appropriate filter).
| */ | ||
| $this->assertTrue( update_option( 'foo', 0 ) ); | ||
| // This will fail since it has been pre-filtered to the same value. | ||
| $this->assertFalse( update_option( 'foo', 0 ) ); | ||
| } |
There was a problem hiding this comment.
It makes sense that these tests are being updated given the change here, however we need to make sure we don't introduce the problem flagged in https://core.trac.wordpress.org/ticket/22192#comment:52 again, which originally led to introducing the pre filter juggling we are reverting here (see fix in https://core.trac.wordpress.org/changeset/56717).
How does this revert address that bug? I am not against this, but I am wary of us making a change to clean up this code while potentially reintroducing the problem. How can we verify that that's not the case? Have you tested this?
There was a problem hiding this comment.
Per @mukeshpanchal27's comment in #5498 (review), looks like that bug is indeed reintroduced. So we either need to find an alternative solution or settle on this BC break being okay.
While the latter may sound controversial, maybe it would be okay, something to cover in a dev note and encourage changing, as it is certainly an edge case.
There was a problem hiding this comment.
I think we need a test specifically for that bug, and not one that abstracts the bug to a root cause check, like this one does. I would not be surprised if removing the pre-filtering, reintroduces the bug, which my require additional consideration.
There was a problem hiding this comment.
This changes again introduce gmt_offset error: WordPress/gutenberg#54806 (comment)
| ) | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( $default_value === $raw_old_value ) { | ||
| if ( $default_value === $old_value ) { |
There was a problem hiding this comment.
Couldn't verify but this will introduce error in Yoast. Per https://core.trac.wordpress.org/ticket/59360#comment:35
|
Closing this now that we've chosen to revert the changes from the 6.4 release cycle. |
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
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.