Opened 12 years ago
Closed 9 years ago
#19623 closed enhancement (fixed)
Use Comment API in comments_template, rather than hardcoded SQL
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.1 | Priority: | normal |
Severity: | major | Version: | 3.3 |
Component: | Comments | Keywords: | has-patch needs-testing 2nd-opinion |
Focuses: | Cc: |
Description
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. :)
Attachments (7)
Change History (28)
@
12 years ago
Enhance WP_Comment_Query and use the new functionality in comments_template (now without error_log call, and without repeated approve arg)
#3
@
11 years ago
- Cc hardy101@… added
- Version changed from 3.3 to 3.4.1
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...
#4
@
11 years ago
- Keywords needs-testing added; dev-feedback removed
- Version changed from 3.4.1 to 3.3
@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.
#5
@
11 years ago
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
#6
@
11 years ago
Yes, 19623-2.diff looks better.
#7
@
11 years ago
Thanks @scribu - glad to hear it. Hope it helps some folks and makes it into core in some capacity.
This ticket was mentioned in IRC in #wordpress-dev by johnbillion. View the logs.
10 years ago
#11
follow-up:
↓ 12
@
10 years ago
The line numbers have changed since 19623-2.diff
, the 19623.3.diff
patch was created from 4.0-alpha.
#12
in reply to:
↑ 11
@
10 years ago
Replying to jesin:
The line numbers have changed since
19623-2.diff
, the19623.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.
#13
@
9 years ago
- Severity changed from normal to major
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.
#14
@
9 years ago
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.
#17
@
9 years ago
- Keywords 2nd-opinion added; 4.1-early removed
- Milestone changed from Future Release to 4.1
- Owner set to boonebgorges
- Status changed from new to accepted
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.
#18
@
9 years ago
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 ;)
Here's a patch as a starting point for discussion.
WP_Comment_Query
to add query args:unapproved_author
,unapproved_author_email
,unapproved_user_id
comments_template
to use the new functionality in theWP_Comment_Query
to get commentscomments_template_args
on the args passed toWP_Comment_Query
before they are passedpost__in
filter if anyone should ever want to query comments from more than one post at the same time (separate from the above requirements)