Skip to content

Remove placeholders. #3448

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
spacedmonkey wants to merge 4 commits into from
+92 −3
Closed

Remove placeholders. #3448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea31ac6
Remove placeholders.
spacedmonkey Oct 12, 2022
f371e0e
Add unit tests.
spacedmonkey Oct 12, 2022
5e4d8ef
Fix unit tests.
spacedmonkey Oct 13, 2022
aec3d11
Improve comments.
spacedmonkey Oct 13, 2022
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
4 changes: 3 additions & 1 deletion src/wp-includes/class-wp-query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3088,10 +3088,12 @@ public function get_posts() {
$cache_args['update_post_meta_cache'],
$cache_args['update_post_term_cache'],
$cache_args['lazy_load_term_meta'],
$cache_args['update_menu_item_cache']
$cache_args['update_menu_item_cache'],
$cache_args['search_orderby_title']
Copy link
Copy Markdown

@dlh01 dlh01 Oct 14, 2022

Choose a reason for hiding this comment

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

Suggested change
$cache_args['search_orderby_title']
$cache_args['search_orderby_title'],

);

$new_request = str_replace( $fields, "{$wpdb->posts}.*", $this->request );
$new_request = $wpdb->remove_placeholder_escape( $new_request );
Copy link
Copy Markdown

@dlh01 dlh01 Oct 12, 2022

Choose a reason for hiding this comment

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

wpdb::remove_placeholder_escape() will call wpdb::placeholder_escape(), and that function will add a filter to the 'query' filter if it isn't already there:

https://github.com/wordpress/wordpress-develop/blob/f371e0ea3007e7fbccca12d38a76b5fa05e13da5/src/wp-includes/class-wpdb.php#L2412-L2439

That might be harmless in the end, but I thought it was worth flagging just because any filter added to 'query' during the RC phase seemed potentially risky.

Copy link
Copy Markdown
Member Author

@spacedmonkey spacedmonkey Oct 12, 2022

Choose a reason for hiding this comment

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

wpdb::placeholder_escape will already have been called to add the placeholder in the first place, when wpdb::prepare is called. Wouldn't this filter already be set?

Copy link
Copy Markdown
Contributor

@peterwilsoncc peterwilsoncc Oct 12, 2022

Choose a reason for hiding this comment

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

Wouldn't this filter already be set?

Looks like it, I ran this in a private window on the front end and it the filter was set:

add_action( 'init', function() {
	global $wp_filter;
	var_dump( $wp_filter[ 'query' ] );
	exit;
} );

Copy link
Copy Markdown

@dlh01 dlh01 Oct 13, 2022

Choose a reason for hiding this comment

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

I was thinking that not all queries might generate a call to prepare(). But I guess it's probably reasonable to assume that something will have at some point, as Peter's snippet suggests.

Copy link
Copy Markdown
Member Author

@spacedmonkey spacedmonkey Oct 13, 2022

Choose a reason for hiding this comment

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

Prepare is called within the WP_Query class.

wordpress-develop/src/wp-includes/class-wp-query.php

Line 1429 in 70dedb0

$q['search_orderby_title'][] = $wpdb->prepare( "{$wpdb->posts}.post_title LIKE %s", $like );

wordpress-develop/src/wp-includes/class-wp-query.php

Line 1433 in 70dedb0

$search .= $wpdb->prepare( "{$searchand}(({$wpdb->posts}.post_title $like_op %s) $andor_op ({$wpdb->posts}.post_excerpt $like_op %s) $andor_op ({$wpdb->posts}.post_content $like_op %s))", $like, $like, $like );

wordpress-develop/src/wp-includes/class-wp-query.php

Line 1558 in 70dedb0

$search_orderby .= $wpdb->prepare( "WHEN {$wpdb->posts}.post_title LIKE %s THEN 1 ", $like );

wordpress-develop/src/wp-includes/class-wp-query.php

Lines 1574 to 1575 in 70dedb0

$search_orderby .= $wpdb->prepare( "WHEN {$wpdb->posts}.post_excerpt LIKE %s THEN 4 ", $like );
$search_orderby .= $wpdb->prepare( "WHEN {$wpdb->posts}.post_content LIKE %s THEN 5 ", $like );

wordpress-develop/src/wp-includes/class-wp-query.php

Line 2062 in 70dedb0

$where .= $wpdb->prepare( " AND {$wpdb->posts}.post_title = %s", stripslashes( $q['title'] ) );

Not just in context where LIKE is used.

$key = md5( serialize( $cache_args ) . $new_request );

$last_changed = wp_cache_get_last_changed( 'posts' );
Expand Down
9 changes: 7 additions & 2 deletions src/wp-includes/class-wpdb.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2413,12 +2413,17 @@ public function log_query( $query, $query_time, $query_callstack, $query_start,
* Generates and returns a placeholder escape string for use in queries returned by ::prepare().
*
* @since 4.8.3
* @since 6.1.0 Added `$reset` parameter.
*
* @param bool $reset Optional. Whether to reset the placeholder. Default false.
Copy link
Copy Markdown
Contributor

@costdev costdev Oct 13, 2022

Choose a reason for hiding this comment

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

Suggested change
* @param bool $reset Optional. Whether to reset the placeholder. Default false.
* @param bool $reset Optional. Whether to reset the placeholder. Default false.
*

Also needs a since annotation below the 4.8.3 entry.

*
* @return string String to escape placeholders.
*/
public function placeholder_escape() {
public function placeholder_escape( $reset = false ) {
static $placeholder;

if ( $reset ) {
$placeholder = null;
}
if ( ! $placeholder ) {
// If ext/hash is not present, compat.php's hash_hmac() does not support sha256.
$algo = function_exists( 'hash' ) ? 'sha256' : 'sha1';
Expand Down
82 changes: 82 additions & 0 deletions tests/phpunit/tests/query/cacheResults.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,18 @@ public function data_query_cache() {
),
),
),
'cache meta query search' => array(
'args' => array(
'cache_results' => true,
'meta_query' => array(
array(
'key' => 'color',
'value' => '00',
'compare' => 'LIKE',
),
),
),
),
'cache comment_count' => array(
'args' => array(
'cache_results' => true,
Expand All @@ -209,6 +221,12 @@ public function data_query_cache() {
),
),
),
'cache search query' => array(
'args' => array(
'cache_results' => true,
's' => 'e',
),
),
);
}

Expand Down Expand Up @@ -827,6 +845,70 @@ public function test_query_cache_delete_meta() {
$this->assertNotSame( $query1->found_posts, $query2->found_posts );
}


/**
* @ticket 56802
*/
public function test_query_cache_like_meta() {
global $wpdb;
$p1 = self::$posts[0];

$args = array(
'cache_results' => true,
'fields' => 'ids',
'meta_query' => array(
array(
'key' => 'color',
'value' => '00',
'compare' => 'LIKE',
),
),
);
$query1 = new WP_Query();
$posts1 = $query1->query( $args );
$num_queries = get_num_queries();

// Force placeholder to be different.
$wpdb->placeholder_escape( true );

$query2 = new WP_Query();
$posts2 = $query2->query( $args );

$this->assertSame( $posts1, $posts2, 'Results of first and second query should be the same' );
$this->assertSame( $num_queries, get_num_queries(), 'Should not result in another database query.' );
$this->assertContains( $p1, $posts2, 'Result should contain post 0.' );
$this->assertSame( $query1->found_posts, $query2->found_posts, 'Found posts should match on both queries' );
}

/**
* @ticket 56802
*/
public function test_query_cache_search() {
global $wpdb;
// Post 0 already has the category foo.
$p1 = self::$posts[1];

$args = array(
'cache_results' => true,
'fields' => 'ids',
's' => 'e',
);
$query1 = new WP_Query();
$posts1 = $query1->query( $args );
$num_queries = get_num_queries();

// Force placeholder to be different.
$wpdb->placeholder_escape( true );

$query2 = new WP_Query();
$posts2 = $query2->query( $args );

$this->assertSame( $posts1, $posts2, 'Results of first and second query should be the same' );
$this->assertSame( $num_queries, get_num_queries(), 'Should not result in another database query.' );
$this->assertContains( $p1, $posts2, 'Result should contain post 0.' );
$this->assertSame( $query1->found_posts, $query2->found_posts, 'Found posts should match on both queries' );
}

/**
* @ticket 22176
*/
Expand Down