From 8bf5fce78f88748e87800e2135a6cfcb41e42679 Mon Sep 17 00:00:00 2001 From: Boone Gorges Date: Mon, 16 Feb 2015 14:09:40 +0000 Subject: [PATCH] Improve 'orderby' syntax for `WP_Comment_Query`. Since [29027], `WP_Query` has supported an array of values for the `$orderby` parameter, with field names as array keys and ASC/DESC as the array values. This changeset introduces the same syntax to `WP_Comment_Query`. We leverage the new support for multiple ORDER BY clauses to fix a bug that causes comments to be queried in an indeterminate order when sorting by the default `comment_date_gmt` and comments share the same value for `comment_date_gmt`. By always including a `comment_ID` subclause at the end of the ORDER BY statement, we ensure that comments always have a unique fallback for sorting. This changeset also includes improvements paralleling those introduced to `WP_Query` in [31312] and [31340], which allow `$orderby` to accept array keys from specific `$meta_query` clauses. This change lets devs sort by multiple clauses of an associated meta query. See #31045. Fixes #30478. See #31265. git-svn-id: https://develop.svn.wordpress.org/trunk@31467 602fd350-edb4-49c9-b593-d223f7449a82 --- src/wp-includes/comment.php | 212 +++++++++++++++---- tests/phpunit/tests/comment/query.php | 287 ++++++++++++++++++++++++-- 2 files changed, 441 insertions(+), 58 deletions(-) diff --git a/src/wp-includes/comment.php b/src/wp-includes/comment.php index 2e0fdb7f24..f0c71af2bb 100644 --- a/src/wp-includes/comment.php +++ b/src/wp-includes/comment.php @@ -326,14 +326,18 @@ class WP_Comment_Query { * @type int $number Maximum number of comments to retrieve. Default null (no limit). * @type int $offset Number of comments to offset the query. Used to build LIMIT clause. * Default 0. - * @type string|array $orderby Comment status or array of statuses. Accepts 'comment_agent', - * 'comment_approved', 'comment_author', 'comment_author_email', - * 'comment_author_IP', 'comment_author_url', 'comment_content', - * 'comment_date', 'comment_date_gmt', 'comment_ID', 'comment_karma', + * @type string|array $orderby Comment status or array of statuses. To use 'meta_value' or + * 'meta_value_num', `$meta_key` must also be defined. To sort by + * a specific `$meta_query` clause, use that clause's array key. + * Accepts 'comment_agent', 'comment_approved', 'comment_author', + * 'comment_author_email', 'comment_author_IP', + * 'comment_author_url', 'comment_content', 'comment_date', + * 'comment_date_gmt', 'comment_ID', 'comment_karma', * 'comment_parent', 'comment_post_ID', 'comment_type', 'user_id', - * 'meta_value', 'meta_value_num', or value of $meta_key. - * Also accepts false, empty array, or 'none' to disable `ORDER BY` - * clause. Default: 'comment_date_gmt'. + * 'meta_value', 'meta_value_num', the value of $meta_key, and the + * array keys of `$meta_query`. Also accepts false, an empty array, + * or 'none' to disable `ORDER BY` clause. + * Default: 'comment_date_gmt'. * @type string $order How to order retrieved comments. Accepts 'ASC', 'DESC'. * Default: 'DESC'. * @type int $parent Parent ID of comment to retrieve children of. Default empty. @@ -411,6 +415,10 @@ class WP_Comment_Query { $this->meta_query = new WP_Meta_Query(); $this->meta_query->parse_query_vars( $this->query_vars ); + if ( ! empty( $this->meta_query->queries ) ) { + $meta_query_clauses = $this->meta_query->get_sql( 'comment', $wpdb->comments, 'comment_ID', $this ); + } + /** * Fires before comments are retrieved. * @@ -514,39 +522,75 @@ class WP_Comment_Query { $this->query_vars['orderby'] : preg_split( '/[,\s]/', $this->query_vars['orderby'] ); - $allowed_keys = array( - 'comment_agent', - 'comment_approved', - 'comment_author', - 'comment_author_email', - 'comment_author_IP', - 'comment_author_url', - 'comment_content', - 'comment_date', - 'comment_date_gmt', - 'comment_ID', - 'comment_karma', - 'comment_parent', - 'comment_post_ID', - 'comment_type', - 'user_id', - ); - if ( ! empty( $this->query_vars['meta_key'] ) ) { - $allowed_keys[] = $this->query_vars['meta_key']; - $allowed_keys[] = 'meta_value'; - $allowed_keys[] = 'meta_value_num'; - } - $ordersby = array_intersect( $ordersby, $allowed_keys ); - foreach ( $ordersby as $key => $value ) { - if ( $value == $this->query_vars['meta_key'] || $value == 'meta_value' ) { - $ordersby[ $key ] = "$wpdb->commentmeta.meta_value"; - } elseif ( $value == 'meta_value_num' ) { - $ordersby[ $key ] = "$wpdb->commentmeta.meta_value+0"; + $orderby_array = array(); + $found_orderby_comment_ID = false; + foreach ( $ordersby as $_key => $_value ) { + if ( ! $_value ) { + continue; } + + if ( is_int( $_key ) ) { + $_orderby = $_value; + $_order = $order; + } else { + $_orderby = $_key; + $_order = $_value; + } + + if ( ! $found_orderby_comment_ID && 'comment_ID' === $_orderby ) { + $found_orderby_comment_ID = true; + } + + $parsed = $this->parse_orderby( $_orderby ); + + if ( ! $parsed ) { + continue; + } + + $orderby_array[] = $parsed . ' ' . $this->parse_order( $_order ); } - $orderby = empty( $ordersby ) ? 'comment_date_gmt' : implode(', ', $ordersby); + + // If no valid clauses were found, order by comment_date_gmt. + if ( empty( $orderby_array ) ) { + $orderby_array[] = "$wpdb->comments.comment_date_gmt $order"; + } + + // To ensure determinate sorting, always include a comment_ID clause. + if ( ! $found_orderby_comment_ID ) { + $comment_ID_order = ''; + + // Inherit order from comment_date or comment_date_gmt, if available. + foreach ( $orderby_array as $orderby_clause ) { + if ( preg_match( '/comment_date(?:_gmt)*\ (ASC|DESC)/', $orderby_clause, $match ) ) { + $comment_ID_order = $match[1]; + break; + } + } + + // If no date-related order is available, use the date from the first available clause. + if ( ! $comment_ID_order ) { + foreach ( $orderby_array as $orderby_clause ) { + if ( false !== strpos( 'ASC', $orderby_clause ) ) { + $comment_ID_order = 'ASC'; + } else { + $comment_ID_order = 'DESC'; + } + + break; + } + } + + // Default to DESC. + if ( ! $comment_ID_order ) { + $comment_ID_order = 'DESC'; + } + + $orderby_array[] = "$wpdb->comments.comment_ID $comment_ID_order"; + } + + $orderby = implode( ', ', $orderby_array ); } else { - $orderby = 'comment_date_gmt'; + $orderby = "$wpdb->comments.comment_date_gmt $order"; } $number = absint( $this->query_vars['number'] ); @@ -709,12 +753,11 @@ class WP_Comment_Query { $join = "JOIN $wpdb->posts ON $wpdb->posts.ID = $wpdb->comments.comment_post_ID"; } - if ( ! empty( $this->meta_query->queries ) ) { - $clauses = $this->meta_query->get_sql( 'comment', $wpdb->comments, 'comment_ID', $this ); - $join .= $clauses['join']; + if ( ! empty( $meta_query_clauses ) ) { + $join .= $meta_query_clauses['join']; // Strip leading 'AND'. - $where[] = preg_replace( '/^\s*AND\s*/', '', $clauses['where'] ); + $where[] = preg_replace( '/^\s*AND\s*/', '', $meta_query_clauses['where'] ); if ( ! $this->query_vars['count'] ) { $groupby = "{$wpdb->comments}.comment_ID"; @@ -729,7 +772,7 @@ class WP_Comment_Query { $where = implode( ' AND ', $where ); - $pieces = array( 'fields', 'join', 'where', 'orderby', 'order', 'limits', 'groupby' ); + $pieces = array( 'fields', 'join', 'where', 'orderby', 'limits', 'groupby' ); /** * Filter the comment query clauses. * @@ -744,7 +787,6 @@ class WP_Comment_Query { $join = isset( $clauses[ 'join' ] ) ? $clauses[ 'join' ] : ''; $where = isset( $clauses[ 'where' ] ) ? $clauses[ 'where' ] : ''; $orderby = isset( $clauses[ 'orderby' ] ) ? $clauses[ 'orderby' ] : ''; - $order = isset( $clauses[ 'order' ] ) ? $clauses[ 'order' ] : ''; $limits = isset( $clauses[ 'limits' ] ) ? $clauses[ 'limits' ] : ''; $groupby = isset( $clauses[ 'groupby' ] ) ? $clauses[ 'groupby' ] : ''; @@ -757,7 +799,7 @@ class WP_Comment_Query { } if ( $orderby ) { - $orderby = "ORDER BY $orderby $order"; + $orderby = "ORDER BY $orderby"; } $this->request = "SELECT $fields FROM $wpdb->comments $join $where $groupby $orderby $limits"; @@ -809,6 +851,88 @@ class WP_Comment_Query { return ' AND (' . implode(' OR ', $searches) . ')'; } + + /** + * Parse and sanitize 'orderby' keys passed to the comment query. + * + * @since 4.2.0 + * @access protected + * + * @global wpdb $wpdb WordPress database abstraction object. + * + * @param string $orderby Alias for the field to order by. + * @return string|bool Value to used in the ORDER clause. False otherwise. + */ + protected function parse_orderby( $orderby ) { + global $wpdb; + + $allowed_keys = array( + 'comment_agent', + 'comment_approved', + 'comment_author', + 'comment_author_email', + 'comment_author_IP', + 'comment_author_url', + 'comment_content', + 'comment_date', + 'comment_date_gmt', + 'comment_ID', + 'comment_karma', + 'comment_parent', + 'comment_post_ID', + 'comment_type', + 'user_id', + ); + + if ( ! empty( $this->query_vars['meta_key'] ) ) { + $allowed_keys[] = $this->query_vars['meta_key']; + $allowed_keys[] = 'meta_value'; + $allowed_keys[] = 'meta_value_num'; + } + + $meta_query_clauses = $this->meta_query->get_clauses(); + if ( $meta_query_clauses ) { + $allowed_keys = array_merge( $allowed_keys, array_keys( $meta_query_clauses ) ); + } + + $parsed = false; + if ( $orderby == $this->query_vars['meta_key'] || $orderby == 'meta_value' ) { + $parsed = "$wpdb->commentmeta.meta_value"; + } else if ( $orderby == 'meta_value_num' ) { + $parsed = "$wpdb->commentmeta.meta_value+0"; + } else if ( in_array( $orderby, $allowed_keys ) ) { + + if ( isset( $meta_query_clauses[ $orderby ] ) ) { + $meta_clause = $meta_query_clauses[ $orderby ]; + $parsed = sprintf( "CAST(%s.meta_value AS %s)", esc_sql( $meta_clause['alias'] ), esc_sql( $meta_clause['cast'] ) ); + } else { + $parsed = "$wpdb->comments.$orderby"; + } + } + + return $parsed; + } + + /** + * Parse an 'order' query variable and cast it to ASC or DESC as necessary. + * + * @since 4.2.0 + * @access protected + * + * @param string $order The 'order' query variable. + * @return string The sanitized 'order' query variable. + */ + protected function parse_order( $order ) { + if ( ! is_string( $order ) || empty( $order ) ) { + return 'DESC'; + } + + if ( 'ASC' === strtoupper( $order ) ) { + return 'ASC'; + } else { + return 'DESC'; + } + } } /** diff --git a/tests/phpunit/tests/comment/query.php b/tests/phpunit/tests/comment/query.php index e4785fcd81..7003f2ab0b 100644 --- a/tests/phpunit/tests/comment/query.php +++ b/tests/phpunit/tests/comment/query.php @@ -612,14 +612,10 @@ class Tests_Comment_Query extends WP_UnitTestCase { $this->assertEquals( $comment_id2, $comments[1]->comment_ID ); $comments = get_comments( array( 'meta_value' => 'value3', 'orderby' => array( 'key' ) ) ); - $this->assertEquals( 2, count( $comments ) ); - $this->assertEquals( $comment_id, $comments[0]->comment_ID ); - $this->assertEquals( $comment_id3, $comments[1]->comment_ID ); + $this->assertEquals( array( $comment_id3, $comment_id ), wp_list_pluck( $comments, 'comment_ID' ) ); $comments = get_comments( array( 'meta_value' => 'value3', 'orderby' => array( 'meta_value' ) ) ); - $this->assertEquals( 2, count( $comments ) ); - $this->assertEquals( $comment_id, $comments[0]->comment_ID ); - $this->assertEquals( $comment_id3, $comments[1]->comment_ID ); + $this->assertEquals( array( $comment_id3, $comment_id ), wp_list_pluck( $comments, 'comment_ID' ) ); // value1 is present on two different keys for $comment_id yet we should get only one instance // of that comment in the results @@ -630,6 +626,103 @@ class Tests_Comment_Query extends WP_UnitTestCase { $this->assertEquals( 1, count( $comments ) ); } + /** + * @ticket 30478 + */ + public function test_orderby_clause_key() { + $comments = $this->factory->comment->create_many( 3 ); + add_comment_meta( $comments[0], 'foo', 'aaa' ); + add_comment_meta( $comments[1], 'foo', 'zzz' ); + add_comment_meta( $comments[2], 'foo', 'jjj' ); + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'meta_query' => array( + 'foo_key' => array( + 'key' => 'foo', + 'compare' => 'EXISTS', + ), + ), + 'orderby' => 'foo_key', + 'order' => 'DESC', + ) ); + + $this->assertEquals( array( $comments[1], $comments[2], $comments[0] ), $found ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_clause_key_as_secondary_sort() { + $c1 = $this->factory->comment->create( array( + 'comment_date' => '2015-01-28 03:00:00', + ) ); + $c2 = $this->factory->comment->create( array( + 'comment_date' => '2015-01-28 05:00:00', + ) ); + $c3 = $this->factory->comment->create( array( + 'comment_date' => '2015-01-28 03:00:00', + ) ); + + add_comment_meta( $c1, 'foo', 'jjj' ); + add_comment_meta( $c2, 'foo', 'zzz' ); + add_comment_meta( $c3, 'foo', 'aaa' ); + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'meta_query' => array( + 'foo_key' => array( + 'key' => 'foo', + 'compare' => 'EXISTS', + ), + ), + 'orderby' => array( + 'comment_date' => 'asc', + 'foo_key' => 'asc', + ), + ) ); + + $this->assertEquals( array( $c3, $c1, $c2 ), $found ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_more_than_one_clause_key() { + $comments = $this->factory->comment->create_many( 3 ); + + add_comment_meta( $comments[0], 'foo', 'jjj' ); + add_comment_meta( $comments[1], 'foo', 'zzz' ); + add_comment_meta( $comments[2], 'foo', 'jjj' ); + add_comment_meta( $comments[0], 'bar', 'aaa' ); + add_comment_meta( $comments[1], 'bar', 'ccc' ); + add_comment_meta( $comments[2], 'bar', 'bbb' ); + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'meta_query' => array( + 'foo_key' => array( + 'key' => 'foo', + 'compare' => 'EXISTS', + ), + 'bar_key' => array( + 'key' => 'bar', + 'compare' => 'EXISTS', + ), + ), + 'orderby' => array( + 'foo_key' => 'asc', + 'bar_key' => 'desc', + ), + ) ); + + $this->assertEquals( array( $comments[2], $comments[0], $comments[1] ), $found ); + } + + /** * @ticket 27064 */ @@ -985,64 +1078,78 @@ class Tests_Comment_Query extends WP_UnitTestCase { } public function test_orderby_default() { + global $wpdb; + $q = new WP_Comment_Query(); $q->query( array() ); - $this->assertContains( 'ORDER BY comment_date_gmt', $q->request ); + $this->assertContains( "ORDER BY $wpdb->comments.comment_date_gmt", $q->request ); } public function test_orderby_single() { + global $wpdb; + $q = new WP_Comment_Query(); $q->query( array( 'orderby' => 'comment_agent', ) ); - $this->assertContains( 'ORDER BY comment_agent', $q->request ); + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent", $q->request ); } public function test_orderby_single_invalid() { + global $wpdb; + $q = new WP_Comment_Query(); $q->query( array( 'orderby' => 'foo', ) ); - $this->assertContains( 'ORDER BY comment_date_gmt', $q->request ); + $this->assertContains( "ORDER BY $wpdb->comments.comment_date_gmt", $q->request ); } public function test_orderby_comma_separated() { + global $wpdb; + $q = new WP_Comment_Query(); $q->query( array( 'orderby' => 'comment_agent, comment_approved', ) ); - $this->assertContains( 'ORDER BY comment_agent, comment_approved', $q->request ); + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent DESC, $wpdb->comments.comment_approved DESC", $q->request ); } - public function test_orderby_array() { + public function test_orderby_flat_array() { + global $wpdb; + $q = new WP_Comment_Query(); $q->query( array( 'orderby' => array( 'comment_agent', 'comment_approved' ), ) ); - $this->assertContains( 'ORDER BY comment_agent, comment_approved', $q->request ); + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent DESC, $wpdb->comments.comment_approved DESC", $q->request ); } public function test_orderby_array_contains_invalid_item() { + global $wpdb; + $q = new WP_Comment_Query(); $q->query( array( 'orderby' => array( 'comment_agent', 'foo', 'comment_approved' ), ) ); - $this->assertContains( 'ORDER BY comment_agent, comment_approved', $q->request ); + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent DESC, $wpdb->comments.comment_approved DESC", $q->request ); } public function test_orderby_array_contains_all_invalid_items() { + global $wpdb; + $q = new WP_Comment_Query(); $q->query( array( 'orderby' => array( 'foo', 'bar', 'baz' ), ) ); - $this->assertContains( 'ORDER BY comment_date_gmt', $q->request ); + $this->assertContains( "ORDER BY $wpdb->comments.comment_date_gmt", $q->request ); } /** @@ -1081,6 +1188,158 @@ class Tests_Comment_Query extends WP_UnitTestCase { $this->assertNotContains( 'ORDER BY', $q->request ); } + /** + * @ticket 30478 + */ + public function test_orderby_array() { + global $wpdb; + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'orderby' => array( + 'comment_agent' => 'DESC', + 'comment_date_gmt' => 'ASC', + 'comment_ID' => 'DESC', + ), + ) ); + + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent DESC, $wpdb->comments.comment_date_gmt ASC, $wpdb->comments.comment_ID DESC", $q->request ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_array_should_discard_invalid_columns() { + global $wpdb; + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'orderby' => array( + 'comment_agent' => 'DESC', + 'foo' => 'ASC', + 'comment_ID' => 'DESC', + ), + ) ); + + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent DESC, $wpdb->comments.comment_ID DESC", $q->request ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_array_should_convert_invalid_order_to_DESC() { + global $wpdb; + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'orderby' => array( + 'comment_agent' => 'DESC', + 'comment_date_gmt' => 'foo', + 'comment_ID' => 'DESC', + ), + ) ); + + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent DESC, $wpdb->comments.comment_date_gmt DESC, $wpdb->comments.comment_ID DESC", $q->request ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_array_should_sort_by_comment_ID_as_fallback_and_should_inherit_order_from_comment_date_gmt() { + global $wpdb; + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'orderby' => array( + 'comment_agent' => 'DESC', + 'comment_date_gmt' => 'ASC', + ), + ) ); + + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent DESC, $wpdb->comments.comment_date_gmt ASC, $wpdb->comments.comment_ID ASC", $q->request ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_array_should_sort_by_comment_ID_as_fallback_and_should_inherit_order_from_comment_date() { + global $wpdb; + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'orderby' => array( + 'comment_agent' => 'DESC', + 'comment_date' => 'ASC', + ), + ) ); + + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent DESC, $wpdb->comments.comment_date ASC, $wpdb->comments.comment_ID ASC", $q->request ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_array_should_sort_by_comment_ID_DESC_as_fallback_when_not_sorted_by_date() { + global $wpdb; + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'fields' => 'ids', + 'orderby' => array( + 'comment_agent' => 'ASC', + ), + ) ); + + $this->assertContains( "ORDER BY $wpdb->comments.comment_agent ASC, $wpdb->comments.comment_ID DESC", $q->request ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_date_modified_gmt_should_order_by_comment_ID_in_case_of_tie_ASC() { + $now = current_time( 'mysql', 1 ); + $comments = $this->factory->comment->create_many( 5, array( + 'comment_post_ID' => $this->post_id, + 'comment_date_gmt' => $now, + ) ); + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'orderby' => 'comment_date_gmt', + 'order' => 'ASC', + ) ); + + // $comments is ASC by default. + $this->assertEquals( $comments, wp_list_pluck( $found, 'comment_ID' ) ); + } + + /** + * @ticket 30478 + */ + public function test_orderby_date_modified_gmt_should_order_by_comment_ID_in_case_of_tie_DESC() { + $now = current_time( 'mysql', 1 ); + $comments = $this->factory->comment->create_many( 5, array( + 'comment_post_ID' => $this->post_id, + 'comment_date_gmt' => $now, + ) ); + + $q = new WP_Comment_Query(); + $found = $q->query( array( + 'orderby' => 'comment_date_gmt', + 'order' => 'DESC', + ) ); + + // $comments is ASC by default. + rsort( $comments ); + + $this->assertEquals( $comments, wp_list_pluck( $found, 'comment_ID' ) ); + } + public function test_count() { $c1 = $this->factory->comment->create( array( 'comment_post_ID' => $this->post_id, 'user_id' => 7 ) ); $c2 = $this->factory->comment->create( array( 'comment_post_ID' => $this->post_id, 'user_id' => 7 ) );