WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 months ago

#19623 new enhancement

Use Comment API in comments_template, rather than hardcoded SQL

Reported by: simonwheatley Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.3
Component: Comments Keywords: has-patch needs-testing
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 (3)

19623-1.diff (4.3 KB) - added by simonwheatley 2 years ago.
Enhance WP_Comment_Query and use the new functionality in comments_template (now without error_log call, and without repeated approve arg)
comment-template-api.diff (2.9 KB) - added by hardy101 21 months ago.
Use get_comments() API instead of SQL in comment-template.php
19623-2.diff (3.6 KB) - added by hardy101 21 months ago.
Best of simonwheatley's patch with some streamlining

Download all attachments as: .zip

Change History (12)

comment:1 simonwheatley2 years ago

  • Keywords has-patch dev-feedback added

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)

simonwheatley2 years ago

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

comment:2 scribu2 years ago

Related: #8120 #8071

comment:3 hardy10121 months 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...

hardy10121 months ago

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

comment:4 scribu21 months 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.

comment:5 hardy10121 months 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

Last edited 21 months ago by hardy101 (previous) (diff)

hardy10121 months ago

Best of simonwheatley's patch with some streamlining

comment:6 scribu21 months ago

Yes, 19623-2.diff looks better.

comment:7 hardy10121 months ago

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

comment:9 SergeyBiryukov2 months ago

#27018 was marked as a duplicate.

Note: See TracTickets for help on using tickets.