Skip to content
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

Try casting options, etc to strings #7133

Draft
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Draft

Try casting options, etc to strings #7133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
34b0ac5
Tests for get_option().
peterwilsoncc Aug 26, 2024
40c7bde
Tests for get_network_option()
peterwilsoncc Aug 26, 2024
ec26ed8
Cast option DB values to strings.
peterwilsoncc Aug 4, 2024
f6f5cf4
Avoid updates.
peterwilsoncc Aug 4, 2024
d2f5231
Cast various option values as required.
peterwilsoncc Aug 20, 2024
77822e7
Additional type casting here and there.
peterwilsoncc Aug 20, 2024
667df54
Cast space allowed to an integer later in the function.
peterwilsoncc Aug 20, 2024
3750a6d
Comment on why the tests are as they are.
peterwilsoncc Aug 26, 2024
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
2 changes: 1 addition & 1 deletion src/wp-content/themes/twentytwenty/classes/class-twentytwenty-walker-page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public function start_el( &$output, $data_object, $depth = 0, $args = array(), $
} elseif ( $_current_page && $page->ID === $_current_page->post_parent ) {
$css_class[] = 'current_page_parent';
}
} elseif ( get_option( 'page_for_posts' ) === $page->ID ) {
} elseif ( (int) get_option( 'page_for_posts' ) === $page->ID ) {
$css_class[] = 'current_page_parent';
}

Expand Down
4 changes: 2 additions & 2 deletions src/wp-includes/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2547,8 +2547,8 @@ function build_comment_query_vars_from_block( $block ) {
$comment_args['hierarchical'] = false;
}

if ( get_option( 'page_comments' ) === '1' || get_option( 'page_comments' ) === true ) {
$per_page = get_option( 'comments_per_page' );
if ( (bool) get_option( 'page_comments' ) === true ) {
$per_page = (int) get_option( 'comments_per_page' );
$default_page = get_option( 'default_comments_page' );
if ( $per_page > 0 ) {
$comment_args['number'] = $per_page;
Expand Down
2 changes: 1 addition & 1 deletion src/wp-includes/class-wp-site.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ private function get_details() {
}
$details->blogname = get_option( 'blogname' );
$details->siteurl = get_option( 'siteurl' );
$details->post_count = get_option( 'post_count' );
$details->post_count = (int) get_option( 'post_count' );
$details->home = get_option( 'home' );
restore_current_blog();

Expand Down
3 changes: 3 additions & 0 deletions src/wp-includes/ms-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2565,6 +2565,9 @@ function get_space_allowed() {
$space_allowed = 100;
}

// Cast the value to an integer.
$space_allowed = (int) $space_allowed;

/**
* Filters the upload quota for the current site.
*
Expand Down
48 changes: 43 additions & 5 deletions src/wp-includes/option.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,8 @@ function update_option( $option, $value, $autoload = null ) {
*/
$value = apply_filters( 'pre_update_option', $value, $option, $old_value );

$serialized_value = maybe_serialize( $value );

/*
* If the new and old values are the same, no need to update.
*
Expand All @@ -900,7 +902,7 @@ function update_option( $option, $value, $autoload = null ) {
*
* See https://core.trac.wordpress.org/ticket/38903
*/
if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
if ( sprintf( '%s', $serialized_value ) === $old_value || maybe_serialize( $old_value ) === $serialized_value ) {
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned about this, for the same reason that https://core.trac.wordpress.org/ticket/22192 didn't work out the way we were hoping. There is definitely room for BC breakage here.

$old_value is not necessarily a string or non-scalar value. The return value of get_option() can be filtered, and it's not mandated anywhere to return a string. And even without custom filters, this can commonly be another type, particularly when the default is returned that is specified via register_setting() and injected via core's own filter usage.

Copy link
Member

Choose a reason for hiding this comment

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

Also, why are we using $serialized_value in the first clause? $old_value may be non-scalar in which case this is never true.

Could this maybe work instead?

Suggested change
if ( sprintf( '%s', $serialized_value ) === $old_value || maybe_serialize( $old_value ) === $serialized_value ) {
if (
$value === $old_value ||
( is_scalar( $value ) && is_scalar( $old_value ) && sprintf( '%s', $value ) === sprintf( '%s', $old_value ) ) ||
maybe_serialize( $old_value ) === $serialized_value ) {

This seems a bit safer to me as it maintains both of the original clauses and only considers the problem case as a new clause for scalar values.

return false;
}

Expand All @@ -909,8 +911,6 @@ function update_option( $option, $value, $autoload = null ) {
return add_option( $option, $value, '', $autoload );
}

$serialized_value = maybe_serialize( $value );

/**
* Fires immediately before an option value is updated.
*
Expand All @@ -922,6 +922,15 @@ function update_option( $option, $value, $autoload = null ) {
*/
do_action( 'update_option', $option, $old_value, $value );

/*
* Ensure the serialized value is a string.
*
* This ensure that the option is stored in the cache in the same format as the
* option is stored in the database. Rather than type casting, sprintf is used to
* match the process used by wpdb::prepare().
*/
$serialized_value = sprintf( '%s', $serialized_value );
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, the $serialized_value variable isn't used anywhere between lines 905 and 923. Could we move this right below the clause in 905 to better colocated the code that adjusts the option value to store?


$update_args = array(
'option_value' => $serialized_value,
);
Expand Down Expand Up @@ -1117,6 +1126,15 @@ function add_option( $option, $value = '', $deprecated = '', $autoload = null )
*/
do_action( 'add_option', $option, $value );

/*
* Ensure the serialized value is a string.
*
* This ensure that the option is stored in the cache in the same format as the
* option is stored in the database. Rather than type casting, sprintf is used to
* match the process used by wpdb::prepare().
*/
$serialized_value = sprintf( '%s', $serialized_value );

$result = $wpdb->query( $wpdb->prepare( "INSERT INTO `$wpdb->options` (`option_name`, `option_value`, `autoload`) VALUES (%s, %s, %s) ON DUPLICATE KEY UPDATE `option_name` = VALUES(`option_name`), `option_value` = VALUES(`option_value`), `autoload` = VALUES(`autoload`)", $option, $serialized_value, $autoload ) );
if ( ! $result ) {
return false;
Expand Down Expand Up @@ -2147,6 +2165,16 @@ function add_network_option( $network_id, $option, $value ) {
$value = sanitize_option( $option, $value );

$serialized_value = maybe_serialize( $value );

/*
* Ensure the serialized value is a string.
*
* This ensure that the option is stored in the cache in the same format as the
* option is stored in the database. Rather than type casting, sprintf is used to
* match the process used by wpdb::prepare().
*/
$serialized_value = sprintf( '%s', $serialized_value );

$result = $wpdb->insert(
$wpdb->sitemeta,
array(
Expand Down Expand Up @@ -2359,6 +2387,8 @@ function update_network_option( $network_id, $option, $value ) {
*/
$value = apply_filters( "pre_update_site_option_{$option}", $value, $old_value, $option, $network_id );

$serialized_value = maybe_serialize( $value );

/*
* If the new and old values are the same, no need to update.
*
Expand All @@ -2368,7 +2398,7 @@ function update_network_option( $network_id, $option, $value ) {
*
* See https://core.trac.wordpress.org/ticket/44956
*/
if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
if ( sprintf( '%s', $serialized_value ) === $old_value || maybe_serialize( $old_value ) === $serialized_value ) {
Copy link
Member

Choose a reason for hiding this comment

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

See my comment above, this would apply here as well.

return false;
}

Expand All @@ -2389,7 +2419,15 @@ function update_network_option( $network_id, $option, $value ) {
} else {
$value = sanitize_option( $option, $value );

$serialized_value = maybe_serialize( $value );
/*
* Ensure the serialized value is a string.
*
* This ensure that the option is stored in the cache in the same format as the
* option is stored in the database. Rather than type casting, sprintf is used to
* match the process used by wpdb::prepare().
*/
$serialized_value = sprintf( '%s', $serialized_value );

$result = $wpdb->update(
$wpdb->sitemeta,
array( 'meta_value' => $serialized_value ),
Expand Down
8 changes: 4 additions & 4 deletions tests/phpunit/tests/ajax/wpAjaxWpCompressionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public function test_set_yes() {
}

// Check the site option is not changed due to lack of nonce.
$this->assertSame( 0, get_site_option( 'can_compress_scripts' ) );
$this->assertSame( '0', get_site_option( 'can_compress_scripts' ) );

// Add a nonce.
$_GET['_ajax_nonce'] = wp_create_nonce( 'update_can_compress_scripts' );
Expand All @@ -161,7 +161,7 @@ public function test_set_yes() {
}

// Check the site option is changed.
$this->assertSame( 1, get_site_option( 'can_compress_scripts' ) );
$this->assertSame( '1', get_site_option( 'can_compress_scripts' ) );
}

/**
Expand All @@ -186,7 +186,7 @@ public function test_set_no() {
}

// Check the site option is not changed due to lack of nonce.
$this->assertSame( 1, get_site_option( 'can_compress_scripts' ) );
$this->assertSame( '1', get_site_option( 'can_compress_scripts' ) );

// Add a nonce.
$_GET['_ajax_nonce'] = wp_create_nonce( 'update_can_compress_scripts' );
Expand All @@ -199,7 +199,7 @@ public function test_set_no() {
}

// Check the site option is changed.
$this->assertSame( 0, get_site_option( 'can_compress_scripts' ) );
$this->assertSame( '0', get_site_option( 'can_compress_scripts' ) );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/tests/https-migration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public function test_wp_update_https_migration_required() {
// Changing HTTP to HTTPS on a site with content should result in flag being set, requiring migration.
update_option( 'fresh_site', '0' );
wp_update_https_migration_required( 'http://example.org', 'https://example.org' );
$this->assertTrue( get_option( 'https_migration_required' ) );
$this->assertTrue( (bool) get_option( 'https_migration_required' ) );

// Changing another part than the scheme should delete/reset the flag because changing those parts (e.g. the
// domain) can have further implications.
Expand Down
65 changes: 65 additions & 0 deletions tests/phpunit/tests/option/getOptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,69 @@ public function test_get_options_with_nonexistent_options() {

$this->assertFalse( $options['nonexistent_option'], 'nonexistent_option is present in option.' );
}

/**
* Test get_option() returns the same type when cached and uncached.
*
* @ticket 32848
*
* @dataProvider data_get_option_return_type_cached_and_uncached
*
* @param mixed $option_vale The value to test.
*/
public function test_get_option_return_type_cached_and_uncached( $option_vale ) {
$option_name = 'option_for_type_testing';

// Set the option value.
update_option( $option_name, $option_vale, false );

// Get the option while cached.
$option_cached = get_option( $option_name );

// Clear the cache.
wp_cache_delete( $option_name, 'options' );

// Get the option while uncached.
$option_uncached = get_option( $option_name );

// Check that the return type is the same.
$this->assertSame( gettype( $option_cached ), gettype( $option_uncached ), 'The return type is not the same.' );
/*
* Check canonicalized value.
*
* This is done separately from the above check to avoid false negatives
* for objects as assertSame checks for the same instance.
*/
$this->assertEqualsCanonicalizing( $option_cached, $option_uncached, 'The option values are not the same.' );
}

/**
* Data provider for test_get_option_return_type_cached_and_uncached().
*
* @return array[]
*/
public function data_get_option_return_type_cached_and_uncached() {
return array(
'an empty string' => array( '' ),
'a string with spaces' => array( ' ' ),
'a string with tabs' => array( "\t" ),
'a string with new lines' => array( "\n" ),
'a string with carriage returns' => array( "\r" ),
'int -1' => array( -1 ),
'int 0' => array( 0 ),
'int 1' => array( 1 ),
'float -1.0' => array( -1.0 ),
'float 0.0' => array( 0.0 ),
'float 1.0' => array( 1.0 ),
'false' => array( false ),
'true' => array( true ),
'null' => array( null ),
'an empty array' => array( array() ),
'a non-empty array' => array( array( 'value' ) ),
'an empty object' => array( new stdClass() ),
'a non-empty object' => array( (object) array( 'value' ) ),
'INF' => array( INF ),
'NAN' => array( NAN ),
);
}
}
74 changes: 74 additions & 0 deletions tests/phpunit/tests/option/networkOption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,4 +412,78 @@ public function test_delete_network_option_does_not_use_single_site_notoptions_c
$this->assertIsArray( $network_notoptions_cache, 'Multisite notoptions cache should be set.' );
$this->assertArrayHasKey( 'ticket_61730_notoption', $network_notoptions_cache, 'The option should be in the notoptions cache.' );
}

/**
* Test get_network_option() returns the same type when cached and uncached.
*
* @ticket 32848
*
* @covers ::get_network_option

* @dataProvider data_get_network_option_return_type_cached_and_uncached
*
* @param mixed $option_vale The value to test.
*/
public function test_get_network_option_return_type_cached_and_uncached( $option_vale ) {
$option_name = 'option_for_type_testing';
if ( is_multisite() ) {
$cache_key = "1:$option_name";
$cache_group = 'site-options';
} else {
$cache_key = $option_name;
$cache_group = 'options';
}

// Set the option value.
update_network_option( 1, $option_name, $option_vale, false );

// Get the option while cached.
$option_cached = get_network_option( 1, $option_name );

// Clear the cache.
wp_cache_delete( $cache_key, $cache_group );

// Get the option while uncached.
$option_uncached = get_network_option( 1, $option_name );

// Check that the return type is the same.
$this->assertSame( gettype( $option_cached ), gettype( $option_uncached ), 'The return type is not the same.' );
/*
* Check canonicalized value.
*
* This is done separately from the above check to avoid false negatives
* for objects as assertSame checks for the same instance.
*/
$this->assertEqualsCanonicalizing( $option_cached, $option_uncached, 'The option values are not the same.' );
}

/**
* Data provider for test_get_network_option_return_type_cached_and_uncached().
*
* @return array[]
*/
public function data_get_network_option_return_type_cached_and_uncached() {
return array(
'an empty string' => array( '' ),
'a string with spaces' => array( ' ' ),
'a string with tabs' => array( "\t" ),
'a string with new lines' => array( "\n" ),
'a string with carriage returns' => array( "\r" ),
'int -1' => array( -1 ),
'int 0' => array( 0 ),
'int 1' => array( 1 ),
'float -1.0' => array( -1.0 ),
'float 0.0' => array( 0.0 ),
'float 1.0' => array( 1.0 ),
'false' => array( false ),
'true' => array( true ),
'null' => array( null ),
'an empty array' => array( array() ),
'a non-empty array' => array( array( 'value' ) ),
'an empty object' => array( new stdClass() ),
'a non-empty object' => array( (object) array( 'value' ) ),
'INF' => array( INF ),
'NAN' => array( NAN ),
);
}
}
4 changes: 2 additions & 2 deletions tests/phpunit/tests/taxonomy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,7 +1056,7 @@ public function test_default_term_for_custom_taxonomy() {

// Test default term.
$term = wp_get_post_terms( $post_id, $tax );
$this->assertSame( get_option( 'default_term_' . $tax ), $term[0]->term_id );
$this->assertSame( get_option( 'default_term_' . $tax ), (string) $term[0]->term_id );

// Test default term deletion.
$this->assertSame( wp_delete_term( $term[0]->term_id, $tax ), 0 );
Expand All @@ -1077,7 +1077,7 @@ public function test_default_term_for_custom_taxonomy() {

// Test default term.
$term = wp_get_post_terms( $post_id, $tax );
$this->assertSame( get_option( 'default_term_' . $tax ), $term[0]->term_id );
$this->assertSame( get_option( 'default_term_' . $tax ), (string) $term[0]->term_id );

// wp_set_object_terms() should not assign default term.
wp_set_object_terms( $post_id, array(), $tax );
Expand Down
4 changes: 2 additions & 2 deletions tests/phpunit/tests/term/splitSharedTerm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,12 @@ public function test_should_update_default_category_on_term_split() {
);
clean_term_cache( $t1['term_id'], 'category' );

$this->assertSame( $t1['term_id'], get_option( 'default_category', -1 ) );
$this->assertSame( $t1['term_id'], (int) get_option( 'default_category', -1 ) );

$new_term_id = _split_shared_term( $t1['term_id'], $t1['term_taxonomy_id'] );

$this->assertNotEquals( $new_term_id, $t1['term_id'] );
$this->assertSame( $new_term_id, get_option( 'default_category', -1 ) );
$this->assertSame( $new_term_id, (int) get_option( 'default_category', -1 ) );
}

/**
Expand Down
Loading