WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#36487 closed defect (bug) (fixed)

Hierarchical comments do not display on second call of comments_template

Reported by: cookiesowns Owned by: boonebgorges
Milestone: 4.6 Priority: high
Severity: normal Version: 4.4.2
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

If you attempt to use comments_template more than once on a page, it will break rendering of Hierarchical comments on the second or more calls.

It appears to be a bug related to class-wp-comment-query->get_comment_ids()'s caching.

As get_comment_ids() also populates the clauses such as the where clauses... for the comment query which fill_descendants() require.

It also spits out SQL errors:

WordPress database error You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE  AND comment_parent IN (1,4,5)  ORDER BY comment_date_gmt ASC, comment_ID ' at line 1 for query   WHERE  AND comment_parent IN (1,4,5)  ORDER BY comment_date_gmt ASC, comment_ID ASC made by require('C:\wamp\www\wordpress-test\wp-blog-header.php'), require_once('C:\wamp\www\wordpress-test\wp-includes\template-loader.php'), include('C:\wamp\www\wordpress-test\wp-content\themes\twentysixteen\single.php'), comments_template, WP_Comment_Query->__construct, WP_Comment_Query->query, WP_Comment_Query->get_comments, WP_Comment_Query->fill_descendants
<?php
$comment_ids = wp_cache_get( $cache_key, 'comment' );
if ( false === $comment_ids ) {
        $comment_ids = $this->get_comment_ids();
        wp_cache_add( $cache_key, $comment_ids, 'comment' );
}
<?php

protected function fill_descendants( $comments ) {
.....

$_where = $this->filtered_where_clause
....


}

This is easily reproducible on a clean WP 4.4.2 install. Simply output comments_template() more than once on a page that has comments in place, and add some Hierarchical comments.

Attachments (1)

36487.diff (4.9 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (6)

#1 @boonebgorges
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.6

@cookiesowns Thanks very much for the report and the preliminary diagnosis. You are quite correct.

To spell out the problem a bit more: The fill_descendants() method builds its SQL strings out of the SQL strings built in get_comment_ids(). But if the main comment query is cached, then get_comment_ids() is never called, and those strings are never built. Thus, when you run the exact same comment query twice, the first time primes the get_comment_ids cache, and the second one hits that cache, resulting in a broken fill_descendants() query.

Coming up with a "correct" fix for this is pretty difficult. In theory, I suppose it would be cleanest if the fill_descendants() queries were built totally independently of the get_comment_ids() queries. But this would require a fairly extensive rewrite of WP_Comment_Query, which is more than I can stomach at the moment.

I propose 36487.diff as an alternative solution. It's a bit tricky, but it is also much more localized - and it has the potential for auxiliary benefit. Here's the breakdown:

  • fill_descendants() crawls down each step of the hierarchy and runs a single SQL query to grab the child comments of a given level.
  • I've added a parent-child relationship cache. So, the first time fill_descendants() is run for a given set of query_vars, each located comment gets an array of its child-comment IDs stored in the cache.
  • Then, on subsequent calls to fill_descendants(), the query at each level of the hierarchy only queries for parents whose children are not found in the cache. For queries that are exact duplicates, *all* parents in the hierarchy should already be cached. So no additional SQL queries are run.
  • Note that the cache key has to be built out of the query_vars, since the hierarchy being cached depends on the params passed to WP_Comment_Query. (For example, running a query that includes comment__not_in might result in a cached hierarchy that is incomplete for subsequent queries that don't contain comment__not_in.) In order to make this reliable, I had to make a change to the logic of get_comment_ids() so that that method never changes the query_vars array (see the bit about 'parent').

Strictly speaking, this doesn't directly address the technique used for building the SQL strings in fill_descendants(). The fix is indirect: the fill_descendants() queries should only be run when the get_comment_child_ids caches are empty, but anytime these caches are empty, it should be the case that get_comment_ids() has also been run, and thus the SQL strings available. As an added benefit, if the day ever comes when the 'comment' cache becomes persistent, 36487.diff will result in meaningful performance enhancements across the board.

Phew - it's a bit complicated. @cookiesowns, could you think about this logic, and test the patch to ensure it's behaving as you'd expect? (I could also use a review from someone else familiar with this rat's nest - maybe @rachelbaker?)

@boonebgorges
4 years ago

This ticket was mentioned in Slack in #core-comments by boone. View the logs.


4 years ago

#3 @boonebgorges
4 years ago

  • Owner set to boonebgorges
  • Priority changed from normal to high
  • Status changed from new to assigned

This problem has become more acute now that comment caches are persistent. I've worked with @ocean90 to verify the approach.

#4 @boonebgorges
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 37625:

Comments: Improve caching for hierarchical queries.

Hierarchical comment queries work by first fetching the IDs of top-level
comments, and then filling the descendant tree one level at a time based on the
top-level results. When top-level comment IDs are found in the cache,
WP_Comment_Query does not generate the SQL used to fetch these comments. In
this case, the fill_descendants() query does not have enough information
to fill children. As a result, descendant comments were failing to be filled
in cases where the top-level comments were found in the cache.

This was a minor bug previously, because comment caches were not maintained
between pageloads. Since comment caches are now persistent [37613], the problem
becomes evident anywhere that a persistent object cache is in use.

The solution is to cache parent-child relationships, so that when top-level
comments are found in the cache, descendant comments should be found there as
well.

Fixes #36487.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.