Skip to content

Add caching to comment feeds #2194

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 16 commits into from
Closed

Add caching to comment feeds #2194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f2a72b5
Add caching to comment feeds
spacedmonkey Jan 17, 2022
c345ebb
Add unit tests
spacedmonkey Jan 20, 2022
295b89c
Fix lints.
spacedmonkey Jan 20, 2022
2f9e17d
Break off into it's own method.
spacedmonkey Feb 1, 2022
108ff23
Improve comments.
spacedmonkey Feb 4, 2022
bba3fab
Improved logic.
spacedmonkey Mar 15, 2022
f65d588
Fix lint
spacedmonkey Mar 15, 2022
e51b35f
Remove un-needed variables.
spacedmonkey Mar 15, 2022
d484ab9
Fix number queries.
spacedmonkey Mar 15, 2022
ece6c30
Merge branch 'trunk' into fix/comments-cache
spacedmonkey Mar 16, 2022
ff1fc0f
Tweak test.
spacedmonkey Mar 16, 2022
e7e08b2
Merge branch 'trunk' into fix/comments-cache
spacedmonkey Mar 29, 2022
d261900
Revert dry.
spacedmonkey Mar 29, 2022
436af7a
Feedback
spacedmonkey Mar 29, 2022
1aaec71
Use get_col
spacedmonkey Mar 29, 2022
3e9cc15
Merge branch 'trunk' into fix/comments-cache
peterwilsoncc Apr 4, 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
33 changes: 28 additions & 5 deletions src/wp-includes/class-wp-query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2720,10 +2720,22 @@ public function get_posts() {
$corderby = ( ! empty( $corderby ) ) ? 'ORDER BY ' . $corderby : '';
$climits = ( ! empty( $climits ) ) ? $climits : '';

$comments = (array) $wpdb->get_results( "SELECT $distinct {$wpdb->comments}.* FROM {$wpdb->comments} $cjoin $cwhere $cgroupby $corderby $climits" );
$comments_request = "SELECT $distinct {$wpdb->comments}.comment_ID FROM {$wpdb->comments} $cjoin $cwhere $cgroupby $corderby $climits";

$key = md5( $comments_request );
$last_changed = wp_cache_get_last_changed( 'comment' ) . ':' . wp_cache_get_last_changed( 'posts' );

$cache_key = "comment_feed:$key:$last_changed";
$comment_ids = wp_cache_get( $cache_key, 'comment' );
if ( false === $comment_ids ) {
$comment_ids = $wpdb->get_col( $comments_request );
wp_cache_add( $cache_key, $comment_ids, 'comment' );
}
_prime_comment_caches( $comment_ids, false );

// Convert to WP_Comment.
/** @var WP_Comment[] */
$this->comments = array_map( 'get_comment', $comments );
$this->comments = array_map( 'get_comment', $comment_ids );
$this->comment_count = count( $this->comments );

$post_ids = array();
Expand Down Expand Up @@ -3164,11 +3176,22 @@ public function get_posts() {
/** This filter is documented in wp-includes/query.php */
$climits = apply_filters_ref_array( 'comment_feed_limits', array( 'LIMIT ' . get_option( 'posts_per_rss' ), &$this ) );

$comments_request = "SELECT {$wpdb->comments}.* FROM {$wpdb->comments} $cjoin $cwhere $cgroupby $corderby $climits";
$comments = $wpdb->get_results( $comments_request );
$comments_request = "SELECT {$wpdb->comments}.comment_ID FROM {$wpdb->comments} $cjoin $cwhere $cgroupby $corderby $climits";

$key = md5( $comments_request );
$last_changed = wp_cache_get_last_changed( 'comment' );

$cache_key = "comment_feed:$key:$last_changed";
$comment_ids = wp_cache_get( $cache_key, 'comment' );
if ( false === $comment_ids ) {
$comment_ids = $wpdb->get_col( $comments_request );
wp_cache_add( $cache_key, $comment_ids, 'comment' );
}
_prime_comment_caches( $comment_ids, false );

// Convert to WP_Comment.
/** @var WP_Comment[] */
$this->comments = array_map( 'get_comment', $comments );
$this->comments = array_map( 'get_comment', $comment_ids );
$this->comment_count = count( $this->comments );
}

Expand Down
112 changes: 112 additions & 0 deletions tests/phpunit/tests/query/commentFeed.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
<?php

/**
* @group query
* @group comments
* @group feeds
*/
class Tests_Query_CommentFeed extends WP_UnitTestCase {
public static $post_type = 'post';
protected static $post_ids = array();

public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) {
self::$post_ids = $factory->post->create_many(
3,
array(
'post_type' => self::$post_type,
'post_status' => 'publish',
)
);
foreach ( self::$post_ids as $post_id ) {
$factory->comment->create_post_comments( $post_id, 5 );
}

update_option( 'posts_per_rss', 100 );
}

/**
* @ticket 36904
*/
public function test_archive_comment_feed() {
global $wpdb;
add_filter( 'split_the_query', '__return_false' );
$q1 = new WP_Query();
$args = array(
'withcomments' => 1,
'feed' => 'comments-rss',
'post_type' => self::$post_type,
'update_post_meta_cache' => false,
'update_post_term_cache' => false,
'ignore_sticky_posts' => false,
'no_found_rows' => true,
);
$q1->query( $args );
$num_queries = $wpdb->num_queries;
$q2 = new WP_Query();
$q2->query( $args );
$this->assertTrue( $q2->is_comment_feed() );
$this->assertFalse( $q2->is_singular() );
Comment on lines +47 to +48
Copy link
Contributor

@peterwilsoncc peterwilsoncc Mar 25, 2022

Choose a reason for hiding this comment

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

I'd split these in to separate tests, basically:

  • test is_* methods return expected values, counts are correct -- archive
  • test is_* methods return expected values, counts are correct -- singular
  • test cache is hit
  • test cache is invalidated -- archive, new comment
  • test cache is invalidated -- singular new post without new comments (is posts cache last changed time has an affect)

Copy link
Member Author

@spacedmonkey spacedmonkey Mar 29, 2022

Choose a reason for hiding this comment

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

Not sure I understand the need to split these tests up? Can you explain why we would have that overhead?

Copy link
Contributor

@peterwilsoncc peterwilsoncc Mar 29, 2022

Choose a reason for hiding this comment

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

True, there will be a small amount of overhead but the use of shared fixtures in wpSetUpBeforeClass() will reduce most of that.

The reason I suggested splitting them up (or at least doing so differently) is that it will provide more information to developers if something is broken.

In this test, if $q2->is_comment_feed() === false then the test to ensure caching is running correctly won't run. If a code change has broken both, it would be helpful to know all at once.

Copy link
Member Author

@spacedmonkey spacedmonkey Mar 30, 2022

Choose a reason for hiding this comment

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

This is not about fixtures, it is about what we are testing. We are testing the cache, checking that is_comment_feed and is_singular are just ensuring that values I submitted to WP_Query forced those conditionals to be true. Checking all those values at once, is the value in test. Without the asserts apart, the test is not as useful.

Copy link
Contributor

@peterwilsoncc peterwilsoncc Apr 4, 2022

Choose a reason for hiding this comment

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

OK

$this->assertSame( $num_queries + 1, $wpdb->num_queries );
}

/**
* @ticket 36904
*/
public function test_archive_comment_feed_invalid_cache() {
$q1 = new WP_Query();
$args = array(
'withcomments' => 1,
'feed' => 'comments-rss',
'post_type' => self::$post_type,
'update_post_meta_cache' => false,
'update_post_term_cache' => false,
'ignore_sticky_posts' => false,
);
$q1->query( $args );
$comment_count = $q1->comment_count;
$this->assertSame( 15, $comment_count );

$post = self::factory()->post->create_and_get(
array(
'post_type' => self::$post_type,
'post_status' => 'publish',
)
);
self::factory()->comment->create_post_comments( $post->ID, 5 );
$q2 = new WP_Query();
$q2->query( $args );
$this->assertTrue( $q2->is_comment_feed() );
$this->assertFalse( $q2->is_singular() );

$comment_count = $q2->comment_count;
$this->assertSame( 20, $comment_count );
}

/**
* @ticket 36904
*/
public function test_single_comment_feed() {
global $wpdb;
$post = get_post( self::$post_ids[0] );

$q1 = new WP_Query();
$args = array(
'withcomments' => 1,
'feed' => 'comments-rss',
'post_type' => $post->post_type,
'name' => $post->post_name,
'update_post_meta_cache' => false,
'update_post_term_cache' => false,
'ignore_sticky_posts' => false,
);

$q1->query( $args );
$num_queries = $wpdb->num_queries;
$q2 = new WP_Query();
$q2->query( $args );

$this->assertTrue( $q2->is_comment_feed() );
$this->assertTrue( $q2->is_singular() );
$this->assertSame( $num_queries + 1, $wpdb->num_queries );
}
}