Skip to content

Options: Remove pre-filter juggling #5498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
joemcgill wants to merge 2 commits into from
Closed

Options: Remove pre-filter juggling #5498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a75c927
Options: Remove pre-filter juggling when updating options and network…
joemcgill Oct 16, 2023
8442e96
Fix covers annotations
joemcgill Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 9 additions & 56 deletions src/wp-includes/option.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,24 +776,6 @@ function update_option( $option, $value, $autoload = null ) {
*/
$value = apply_filters( 'pre_update_option', $value, $option, $old_value );

/*
* To get the actual raw old value from the database, any existing pre filters need to be temporarily disabled.
* Immediately after getting the raw value, they are reinstated.
* 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}" ) ) {
global $wp_filter;

$old_filters = $wp_filter[ "pre_option_{$option}" ];
unset( $wp_filter[ "pre_option_{$option}" ] );

$raw_old_value = get_option( $option );
$wp_filter[ "pre_option_{$option}" ] = $old_filters;
} else {
$raw_old_value = $old_value;
}

/** This filter is documented in wp-includes/option.php */
$default_value = apply_filters( "default_option_{$option}", false, $option, false );

Expand All @@ -806,16 +788,16 @@ function update_option( $option, $value, $autoload = null ) {
* See https://core.trac.wordpress.org/ticket/38903 and https://core.trac.wordpress.org/ticket/22192.
*/
if (
$value === $raw_old_value ||
$value === $old_value ||
(
$raw_old_value !== $default_value &&
_is_equal_database_value( $raw_old_value, $value )
$old_value !== $default_value &&
_is_equal_database_value( $old_value, $value )
)
) {
return false;
}

if ( $raw_old_value === $default_value ) {
if ( $old_value === $default_value ) {

// Default setting for new options is 'yes'.
if ( null === $autoload ) {
Expand Down Expand Up @@ -2155,35 +2137,6 @@ function update_network_option( $network_id, $option, $value ) {
/** This filter is documented in wp-includes/option.php */
$default_value = apply_filters( "default_site_option_{$option}", false, $option, $network_id );

$has_site_filter = has_filter( "pre_site_option_{$option}" );
$has_option_filter = has_filter( "pre_option_{$option}" );
if ( $has_site_filter || $has_option_filter ) {
if ( $has_site_filter ) {
$old_ms_filters = $wp_filter[ "pre_site_option_{$option}" ];
unset( $wp_filter[ "pre_site_option_{$option}" ] );
}

if ( $has_option_filter ) {
$old_single_site_filters = $wp_filter[ "pre_option_{$option}" ];
unset( $wp_filter[ "pre_option_{$option}" ] );
}

if ( is_multisite() ) {
$raw_old_value = get_network_option( $network_id, $option );
} else {
$raw_old_value = get_option( $option, $default_value );
}

if ( $has_site_filter ) {
$wp_filter[ "pre_site_option_{$option}" ] = $old_ms_filters;
}
if ( $has_option_filter ) {
$wp_filter[ "pre_option_{$option}" ] = $old_single_site_filters;
}
} else {
$raw_old_value = $old_value;
}

if ( ! is_multisite() ) {
/** This filter is documented in wp-includes/option.php */
$default_value = apply_filters( "default_option_{$option}", $default_value, $option, true );
Expand All @@ -2198,9 +2151,9 @@ function update_network_option( $network_id, $option, $value ) {
* See https://core.trac.wordpress.org/ticket/44956 and https://core.trac.wordpress.org/ticket/22192 and https://core.trac.wordpress.org/ticket/59360
*/
if (
$value === $raw_old_value ||
$value === $old_value ||
(
false !== $raw_old_value &&
false !== $old_value &&
/*
* Single site stores values in the `option_value` field, which cannot be set to NULL.
* This means a PHP `null` value will be cast to an empty string, which can be considered
Expand All @@ -2210,14 +2163,14 @@ function update_network_option( $network_id, $option, $value ) {
* As NULL is unique in the database, skip checking an old or new value of NULL
* against any other value.
*/
( ! is_multisite() || ! ( null === $raw_old_value || null === $value ) ) &&
_is_equal_database_value( $raw_old_value, $value )
( ! is_multisite() || ! ( null === $old_value || null === $value ) ) &&
_is_equal_database_value( $old_value, $value )
)
) {
return false;
}

if ( $default_value === $raw_old_value ) {
if ( $default_value === $old_value ) {
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't verify but this will introduce error in Yoast. Per https://core.trac.wordpress.org/ticket/59360#comment:35

return add_network_option( $network_id, $option, $value );
}

Expand Down
116 changes: 112 additions & 4 deletions tests/phpunit/tests/option/networkOption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,14 +689,122 @@ public function data_stored_as_empty_string() {
);
}

/**
* Test cases for testing whether update_network_option() will add a non-existent option.
*/
public function data_option_values() {
return array(
array( '1' ),
array( 1 ),
array( 1.0 ),
array( true ),
array( 'true' ),
array( '0' ),
array( 0 ),
array( 0.0 ),
array( false ),
array( '' ),
array( null ),
array( array() ),
);
}

/**
* Tests that a non-existent option is added only when the pre-filter matches the default 'false'.
*
* @ticket 59360
* @dataProvider data_option_values
*
* @covers ::update_network_option
*/
public function test_update_option_with_false_pre_filter_adds_missing_option( $option ) {
// Filter the old option value to `false`.
add_filter( 'pre_option_foo', '__return_false' );
add_filter( 'pre_site_option_foo', '__return_false' );
Comment on lines +722 to +723
Copy link
Member Author

@joemcgill joemcgill Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these use the pattern used in other tests here?

$hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo';

Copy link
Member

@felixarntz felixarntz Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
*/
Comment on lines +725 to +729
Copy link
Member Author

@joemcgill joemcgill Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this comment is accurate, as this was copy/pasta from the update_option tests.

if ( false === $option ) {
$this->assertFalse( update_network_option( null, 'foo', $option ) );
} else {
$this->assertTrue( update_network_option( null, 'foo', $option ) );
}
}

/**
* Tests that a non-existent option is never added when the pre-filter is not 'false'.
*
* @ticket 59360
* @dataProvider data_option_values
*
* @covers ::update_network_option
*/
public function test_update_option_with_truthy_pre_filter_does_not_add_missing_option( $option ) {
// Filter the old option value to `true`.
add_filter( 'pre_option_foo', '__return_true' );
add_filter( 'pre_site_option_foo', '__return_true' );

$this->assertFalse( update_network_option( null, 'foo', $option ) );
}

/**
* Tests that an existing option is updated even when its pre filter returns the same value.
*
* @ticket 59360
* @dataProvider data_option_values
*
* @covers ::update_network_option
*/
public function test_update_option_with_false_pre_filter_updates_option( $option ) {
// Add the option with a value that is different than any updated.
add_network_option( null, 'foo', 'bar' );

// Force a return value of false.
add_filter( 'pre_option_foo', '__return_false' );
add_filter( 'pre_site_option_foo', '__return_false' );

// This should succeed, since the pre-filtered option will be treated as the default.
$this->assertTrue( update_network_option( null, 'foo', $option ) );
}

/**
* Tests that an existing option is updated even when its pre filter returns the same value.
*
* @ticket 59360
* @dataProvider data_option_values
*
* @covers ::update_network_option
*/
public function test_update_option_with_true_pre_filter_updates_option( $option ) {
// Add the option with a value that is different than any updated.
update_network_option( null, 'foo', 'bar' );

// Force a return value of true.
add_filter( 'pre_option_foo', '__return_true' );
add_filter( 'pre_site_option_foo', '__return_true' );

/*
* 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 ) ) {
$this->assertFalse( update_network_option( null, 'foo', $option ) );
} else {
$this->assertTrue( update_network_option( null, 'foo', $option ) );
}
}

/**
* Tests that a non-existent option is added even when its pre filter returns a value.
*
* @ticket 59360
*
* @covers ::update_network_option
*/
public function test_update_network_option_with_pre_filter_adds_missing_option() {
public function test_update_network_option_with_pre_filter_does_not_missing_option() {
$hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo';

// Force a return value of integer 0.
Expand All @@ -706,7 +814,7 @@ public function test_update_network_option_with_pre_filter_adds_missing_option()
* This should succeed, since the 'foo' option does not exist in the database.
* The default value is false, so it differs from 0.
Comment on lines 814 to 815
Copy link
Member Author

@joemcgill joemcgill Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update inline comment

*/
$this->assertTrue( update_network_option( null, 'foo', 0 ) );
$this->assertFalse( update_network_option( null, 'foo', 0 ) );
}

/**
Expand All @@ -716,7 +824,7 @@ public function test_update_network_option_with_pre_filter_adds_missing_option()
*
* @covers ::update_network_option
*/
public function test_update_network_option_with_pre_filter_updates_option_with_different_value() {
public function test_update_network_option_with_pre_filter_does_not_option_with_different_value() {
$hook_name = is_multisite() ? 'pre_site_option_foo' : 'pre_option_foo';

// Add the option with a value of 1 to the database.
Expand All @@ -729,7 +837,7 @@ public function test_update_network_option_with_pre_filter_updates_option_with_d
* This should succeed, since the 'foo' option has a value of 1 in the database.
* Therefore it differs from 0 and should be updated.
Comment on lines 837 to 838
Copy link
Member Author

@joemcgill joemcgill Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update inline comment

*/
$this->assertTrue( update_network_option( null, 'foo', 0 ) );
$this->assertFalse( update_network_option( null, 'foo', 0 ) );
}

/**
Expand Down
Loading