Use Comment API in comments_template, rather than hardcoded SQL

[simonwheatley]

Currently the function comments_template uses hard-coded SQL queries, rather than WP_Comment_Query or get_comments. There's a note above the queries saying ​/** @todo Use API instead of SELECTs. */.

To replace the queries using the API, the WP_Comment_Query class will need to be extended to allow querying for comments which are either approved or unapproved (hold status) but by a particular comment author email and comment author name or by a particular user_id to replace these two queries:

• SELECT * FROM $wpdb->comments WHERE comment_post_ID = %d AND (comment_approved = '1' OR ( user_id = %d AND comment_approved = '0' ) ) ORDER BY comment_date_gmt
• SELECT * FROM $wpdb->comments WHERE comment_post_ID = %d AND ( comment_approved = '1' OR ( comment_author = %s AND comment_author_email = %s AND comment_approved = '0' ) ) ORDER BY comment_date_gmt

If we change to use the API, there's also an opportunity to add a filter for the args, which would be lovely for plugin developers. :)

[simonwheatley]

Here's a patch as a starting point for discussion.

• Enhances WP_Comment_Query to add query args: unapproved_author, unapproved_author_email, unapproved_user_id
• Enhances comments_template to use the new functionality in the WP_Comment_Query to get comments
• Adds a filter comments_template_args on the args passed to WP_Comment_Query before they are passed
• Adds in a post__in filter if anyone should ever want to query comments from more than one post at the same time (separate from the above requirements)

[simonwheatley]

Enhance WP_Comment_Query and use the new functionality in comments_template (now without error_log call, and without repeated approve arg)

[scribu]

Related: #8120 #8071

[hardy101]

I came up with a patch similar to the one from simonwheatly. Fewer lines in my patch, but the logic is essentially the same. There are three conditions for which comments_template() checks when outputting comments on a post page. 2 out of 3 were using straight SQL in wp-includes/comment-template.php:

1) If the user is logged in, show them approved comments, as well as pending comments where the author (user_id) matches their user ID. [Used direct SQL query - *patched*]

2) If not logged in, and no comment author identified - via WP's author cookie system wp_get_current_commenter() - just display approved comments. [Used WP_Comment_Query API]

3) You're an author identified by your browser cookie after leaving a comment. We'll show you the pending comment if your name and email match the comment. [Used direct SQL query - *patched*]

My patch takes the 2 SQL queries and ports their query elements to get_comments() via WP_Comments_Query's query() arguments. I added:

unapproved_comment_author_user_id

and

unapproved_comment_author_data

The former takes the logged-in user's ID. The latter takes an array including the author's name and email from the cookie data retrieved by wp_get_current_commenter() in comments_template()

The patch is a line-for-line swap in comments_template, with 2 new lines in WP_Comment_Query->query()

The trac ticket scribu mentioned was more about optimizing get_comments(). This patch simply let's us use the benefits of the API that's already there.

Let's put the infamous

/** @todo Use API instead of SELECTs. */

line to rest once and for all...

[hardy101]

Use get_comments() API instead of SQL in comment-template.php

[scribu]

@hardy101: Your patch may have fewer lines, but they're a significantly longer and they duplicate more code.

19623-1.diff​ still applies cleanly.

Also, the Version field should indicate the earliest version when the problem was identified, not the latest.

[hardy101]

scribu,

Sorry I got twitchy on the version flag.

You're right - I was trying to create a line-for-line replacement of the old SQL queries. simonwheatley's original patch was much more elegant in some ways.

I took the best of his patch and streamlined a few things:

1) Took out the direct instantiation of a new WP_Comment_Query, instead using the layer of abstraction afforded by get_comments(). Cleaner.

2) Took out filter 'comments_template_args' since 'pre_get_comments' already accounts for the ability to modify the arg list passed to WP_Comment_Query

3) Cleaned up the 'where' string concatenation when building the SQL query.

4) Did a check for empty $post_in array when adding " AND comment_post_ID IN ($_post_in)" to where clause. Would cause an SQL syntax error if passed an empty array.

I think this combination effort may be a winner. See patch 19623-2.diff

[hardy101]

Best of simonwheatley's patch with some streamlining

[scribu]

Yes, 19623-2.diff​ looks better.

[hardy101]

Thanks @scribu - glad to hear it. Hope it helps some folks and makes it into core in some capacity.

[SergeyBiryukov]

Related: #8120

[SergeyBiryukov]

#27018 was marked as a duplicate.

[jesin]

19623-2.diff patch with line number changes

[jesin]

The line numbers have changed since 19623-2.diff, the 19623.3.diff patch was created from 4.0-alpha.

[helen]

Replying to jesin:

The line numbers have changed since 19623-2.diff, the 19623.3.diff patch was created from 4.0-alpha.

For future reference, offsets in patches are fine so long as they successfully apply, which 19623-2.diff​ still did.

[boonebgorges]

I've run into this issue a number of times, when working on plugins that need to manipulate comments in one way or another. The direct queries in comments_template() effectively mean that all such plugins have to have two separate filters to do anything meaningful - one on 'comments_array' and one somewhere in WP_Comment_Query. This is a bit embarassing, and kind of defeats the purpose of having an API. So I'd urge that the team consider this a high priority.

19623.03.patch is an adaptation of 19623-2.diff that does the following:

• Adds unit tests for the new WP_Comment_Query arguments
• Improves adherence to WP coding standands
• Reorganizes the logic of assembling the SQL query so that it's easier to understand

I'm also attaching a patch 19623.backpat.patch. This contains a couple of PHPUnit integration tests that demonstrate that we're not breaking backward compatibility with these changes. comments_template() is (very) non-amenable to testing, so I had to do some crummy output buffering and filter tricks. The first three tests demonstrate that the comments pulled from the query are the same as the three if/else direct queries currently in comments_template(). The last test shows that plugins filter 'comments_array' will continue to work. These are some pretty terrible looking tests, so I understand if they're not included in the core suite. But I did want to show programatically that the changes maintain full backward compatibility, by writing tests that pass both before and after the changes in 19623.03.patch.

[boonebgorges]

One last point I forgot to mention - I took out the 'post_id__in' stuff because, while it's a good improvement, it didn't have tests and is not relevant to the current ticket.

[boonebgorges]

Circling back around to this, because I'd like to get another opinion or two before moving forward with the fix suggested in 19623.03.patch​. IMHO, the params 'unapproved_user_id', 'unapproved_author', and 'unapproved_author_email' are very clunky. Their semantics overlap with each other and with 'status' in awkward ways, and it will be very important to document how they override other params (so that, eg, passing 'unapproved_author' with 'status=approve' means that not all the returned comments will actually be approved).

A much more elegant - but more complicated - solution would be to allow more fine-grained 'status' queries. Let's say (and here I'm just tossing out a possible syntax) your status argument looked like this:

'status' => array(
    'relation' => 'OR',
    array(
        'status' => 'approve',
    ),
    array(
        'status' => '0',
        'author_email' => 'foo@example.com',
    ),
),

This would be a general framework for complex status queries, and would simultaneously fix #29612.

I think that this kind of framework is probably more than is feasible for 4.1. But I want the record to show that I think it's probably the kind of thing we should be shooting for in the medium to long term. And I don't want the decisions made in the short term to affect these longer term plans. In the current case, it's fairly easy to see how 'unapproved_author_email' etc could, at some point in the future, be translated into a query of the syntax above. So, I do think that the suggested fix is probably good enough for us to fix the problem for 4.1 (and it's definitely better than doing nothing at all). But I wanted to throw this comment out there to see if anyone had ideas for a slightly more elegant approach to the issue.

[boonebgorges]

After some more thinking, 19623.patch​ is an update that I think makes more sense from an API point of view than the other options on the table.

Instead of separate params for unapproved_author, unapproved_author_email, and unapproved_user_id, a single param: include_unapproved. Accepts an array (or comma-separated string) of user IDs or email addresses. A couple things I like about this ("like" being a relative term here):

• The naming is better. 'include_unapproved' carries with it the implication that these unapproved comments will be included *regardless of* the 'status' parameter.
• No need to pass unapproved_author. Email address is enough to differentiate.
• Accepts multiple values. This makes it seem slightly less like a kludge built for the specific purposes of comments_template(), and more like a legitimate API feature.

I feel this is a significant enough improvement to the previous suggestions (and it's a huge improvement over the current situation of making direct SQL queries) that I would feel OK with it going into 4.1 like this, even if we ended up doing something more robust down the line. I'll wait a few days before going ahead with it to see if any brilliant commentary coalesces out of the ether ;)

[boonebgorges]

In 29965:

Use the comment API rather than direct SQL queries in comments_template().

comments_template() is used by most themes to display a post's comments. It
shows all comments that have been approved, and also shows all pending comments
by the current visitor (as determined by the comment cookies). However, the
comments API previously had no way of querying for "all comments that are
either approved, or are unapproved but written by foo@…". The
workaround was a direct SQL query: uncached, not subject to the same filters as
other comment queries, and just generally icky.

The new include_unapproved parameter for WP_Comment_Query accepts an array
of user IDs or email addresses. Pending comments associated with users in this
array will be included in query results, regardless of the value of the 'status'
parameter. In comments_template(), we leap from direct SQL queries to
get_comments() plus `include_unapproved', striving to put right what once
went wrong.

Props boonebgorges, simonwheatley, hardy101, jesin.
Fixes #19623.

[westi]

There is a typo in the new code which means it exposes unapproved comments incorrectly in some cases.

Fixing.

[westi]

In 30547:

Fix the passing of commenter cookie data to the comment API so that we don't try and fetch all unapproved comments for users with no-email address when we only have an author.

Fixes #19623