Fix bug when comparing options that have a pre filter. #5313
Conversation
|
Manually tested with repro steps from WordPress/gutenberg#54806 (comment), and couldn't reproduce the bug. The code looks good to me, but getting a second opinion on it would be nice. |
There was a problem hiding this comment.
Thank you, @felixarntz. I am no longer able to reproduce the same error after implementing these changes.
| * The raw value is only used to determine whether a value is present in the database. It is not used anywhere | ||
| * else, and is not passed to any of the hooks either. | ||
| */ | ||
| if ( has_filter( "pre_option_{$option}" ) ) { |
There was a problem hiding this comment.
What about the pre_option filter?
There was a problem hiding this comment.
@spacedmonkey I agree that in principle there could be the same bug due to usage of pre_option too, but I intentionally did not consider the general pre_option filter, because it may have wider reaching implications, so I think unhooking it temporarily is too risky.
There's a good chance that a site that uses that filter has some special mechanisms in place for retrieving and storing options that shouldn't be unhooked.
That is different with the pre_option_{$option} filter, which is clearly about specific options.
| global $wp_filter; | ||
|
|
||
| $old_filters = $wp_filter[ "pre_option_{$option}" ]; | ||
| unset( $wp_filter[ "pre_option_{$option}" ] ); |
There was a problem hiding this comment.
| unset( $wp_filter[ "pre_option_{$option}" ] ); | |
| remove_all_filters( "pre_option_{$option}" ); |
Could we use this?
There was a problem hiding this comment.
@spacedmonkey I tried it, but the problem is that it also calls the remove_all_filters() method on the WP_Hook object in $wp_filter[ "pre_option_{$option}" ]. So we then cannot reinstate the filters that were previously there.
There was a problem hiding this comment.
Maybe add a comment saying why we didnt use remove_all_filters.
|
Committed in https://core.trac.wordpress.org/changeset/56717 |
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.
Tests are included and proves this fixes the issue: Without the changes in
option.php, the new teststest_update_option_with_pre_filter_adds_missing_option()andtest_update_option_with_pre_filter_updates_option_with_different_value()fail.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.