Hierarchical comments do not display on second call of comments_template

[cookiesowns]

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.

[boonebgorges]

@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]

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

[boonebgorges]

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.