[wp-trac] [WordPress Trac] #36487: Hierarchical comments do not display on second call of comments_template

[WordPress Trac]

#36487: Hierarchical comments do not display on second call of comments_template
--------------------------+--------------------
 Reporter:  cookiesowns   |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  4.6
Component:  Comments      |     Version:  4.4.2
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:
--------------------------+--------------------
Changes (by boonebgorges):

 * keywords:   => has-patch
 * milestone:  Awaiting Review => 4.6


Comment:

 @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 [attachment: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, [attachment: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?)

--
Ticket URL: <https://core.trac.wordpress.org/ticket/36487#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform