Function get_page_of_comment() returns incorrect page numbers

[GregMulhauser]

The function get_page_of_comment(), defined in /wp-includes/comment.php for WordPress 2.7, returns wildly incorrect results in cases where the database includes spam trackbacks or pingbacks. The reason is that the DB query on line 595 does not include "comment_approved = 1" and therefore incorrectly counts unapproved trackback and pingback spam (not to mention non-spam comments in moderation). As far as I can tell from browsing my DB, although spam comments have their comment_type changed to 'spam' in the database, trackbacks and pingbacks still retain their original comment_type.

As a newbie to the WordPress Trac, I'm afraid I don't know how to provide a diff, but the full correct query is as follows:

$oldercoms = $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(comment_ID) FROM $wpdb->comments WHERE comment_approved = 1 AND comment_post_ID = %d AND comment_parent = 0 AND comment_date_gmt < '%s'" . $comtypewhere, $comment->comment_post_ID, $comment->comment_date_gmt ) );

The upshot is that without the above fix, comment URLs in several places (e.g., RSS feeds) may point to non-existent pages.

All the best,
Greg

[DD32]

see also: #8946: Redirection after commenting leads to an empty page [not confirmed] (Which i suspect is due to this)

[ryan]

(In [10449]) Include only approved comments when determining page of comment. Props GregMulhauser. fixes #8973 for trunk

[ryan]

(In [10450]) Include only approved comments when determining page of comment. Props GregMulhauser. fixes #8973 for 2.7

[mrmist]

Unfortunately I do not think that this is fixed.

Or rather, one issue is fixed but another is created.

If someone has a comment awaiting moderation, and they add a further comment, which happens to be a page-turning comment, then the refreshed page after submitting will

a) not be on the new page and b) not link to the new page.

In other words, the new comment does not immediately appear as awaiting moderation. Once the article page is (manually) revisited, the new comment is visable (awaiting mod.)

Possibly you may find this behaviour less of an issue than whatever the other issue was.

[ryan]

We'll probably have to use queries similar to those in comment_template(). I'd rather leave that for 2.8 and go with comment_approved = 1 for 2.7.1 as it seems the lesser of the two problems.

[GregMulhauser]

I think ryan and mrmist are both right.

Related to this (not sure whether this merits a separate submission?), I think it would be helpful if those functions which call on get_page_of_comments could get an indication back as to whether a value of 1 indicates the same page of comments as that included at the post's permalink or whether it indicates a completely different page of comments (as will happen when there is more than one page, and comments are displayed in reverse order).

As the function operates at the moment, folks with reverse-ordered comments wind up with a lot of extraneous "comment-page-1" URLs for individual comments when they could just have links to the main post instead.

All the best,
Greg

[Denis-de-Bernardy]

itching to punt this to future, from lack of traction...

[hakre]

Looks like thise foremost is a documentation problem with the get_page_of_comment() function. For what I see are arguments already supported (comments, trackback, and pingsbacks).

The documentation says nothing about wether or not comments must be approved. It does not say wether or not the function should be used on the blogs front or within the backend.

[azaozz]

No patch.

[solarissmoke]

This patch uses logic similar to comments_template(). There are some assumptions here regarding what this information is going to be used for - i.e., to get the right page number for *the current user* (who can he his/her own awaiting-moderation comments). If, for example, a plugin tries to use it for something else, then weirdness can be expected.

[solarissmoke]

Include current user's unmoderated comments when finding comment page

[billerickson]

Related: #18603 - get_page_of_comment() adds approved comments and parent comments, which results in an inaccurate count of comments and page number.

[ericlewis]

8973.diff solves the issue for me, makes sense.

For reproduction of bug:

1. Lower comment pagination break from 50 to 3 in Settings > Discussion.
2. With a user account that requires moderation of their comments, add 4 comments, and don't moderate.
3. On the 4th comment, you should be sent to comment-page-1, where your comment does not exist.

[SergeyBiryukov]

8973.diff​ still applies and fixes the issue outlined in comment:5 and comment:16.

[dossy]

Test and updated patch to implement this for 4.4-4.5

[dossy]

I wrote up a test case to demonstrate the issue (a commenter's unapproved comments not being included in the count) and a patch to address it (see 8973-1.diff). However, code that calls get_page_of_comment() will need to be updated to be aware of this in order for the user experience to Do The Right Thing, but I'm hoping this at least is a start in the right direction.

[lukecavanagh]

@dossy

Patch 8973-1.diff applies cleanly to WP 4.7 beta2.

[jdorner]

Tried to reproduce, but need more info (steps)

[matjack1]

Updated patch for WP 5.0 and added functionality in templates

[matjack1]

How to replicate the bug

[matjack1]

I've managed to reproduce the error by publishing a number of unapproved comments higher than comments pagination and then an approved comment.

For example in the following screenshot I'm on the second page of comments just because of unapproved comments:

In this case if I'm the author of the unapproved comments the links in the "Recent comments" section were wrong.

The attached patch is fixing this issue and removing some duplication in the file. I don't know if you like the code like this or if I should extract the added template lines in a separate function. I'm happy to update the patch if you think that's better.

[matjack1]

Not sure if helpful, but I've run this patch on Travis: ​https://travis-ci.org/matjack1/wordpress-develop/builds/363134316

Maybe it's a bit easier for you to check.

[pento]

8973.2.diff​ looks good from a brief glance, but needs testing and review.

[audrasjb]

Addressed during today's bug scrub.
The patch doesn't applies cleanly. Could you please refresh it so this one can land in 5.2?

[matjack1]

Hey @audrasjb @pento just updated the patch, looks like the conflict was something of my code that has been implemented in the same way already. I think :)

[pento]

Thank you for the refresh, @matjack1!

I have a few notes on 8973.3.diff​:

• In comments_template(), is there are reason for removing the use of wp_get_current_commenter()? This function allows for commenters who aren't logged in, but do have a cookie saved with their commenter info. Unapproved comments should show for these commenters. See Tests_Comment_CommentsTemplate::test_query_offset_should_not_include_unapproved_comments() for testing this functionality.
• The docblocks for get_comment_link() and get_page_of_comment() need to be updated to mention the include_unapproved parameter.
• To keep things looking neat, running composer run format on the changed files will automatically format the code according to the WordPress Coding Standards.

As we're getting quite late in the 5.2 beta period, and there are still quite a lot of tickets open, I'm going to move this ticket to 5.3. There's no need to rush this patch, we can take our time reviewing and testing it to make sure it works as expected. Thank you for your work here!

[imath]

Hi,

I've managed to reproduce an issue a bit different than the one described in this comment. If I do the 3 steps listed to reproduce, the issue I get at last step is I'm redirected to a wrong comments page and it's actually an "earlier" one.
For example: if the redirected page should be comment-page-3, I'm redirected to comment-page-2. I guess it's probably linked to whether you want newest or oldest comments first.

To fix the issue, I find it a bit too bad to add new parameters to the array argument of get_comment_link() and get_page_of_comment().

Instead, in 8973.4.patch, I'm suggesting to create a new function to get the include_unapproved arguments for the comments_template() and get_page_of_comment() functions.

I also think it's important to add a new filter inside get_page_of_comment() to make it possible just like it is in comments_template() to edit the comments query arguments before actually querying.

I agree with @pento It's important to keep the code relative to wp_get_current_commenter() in comments_template(), because the issue is also concerning these users.

I added two unit tests, one for Logged in user and another one for the current commenter.
I've run composer run format on edited files.

[davidbaumwald]

This ticket needs a resolution, and with 5.3 RC1 approaching, I don't think it will make the deadline. This is being moved to Future Release, but if a committer feels strongly about this one being in 5.3 and has the time to see it through, it can be moved back up.

[davidbaumwald]

@imath Is this something you feel can make into 5.5?

[imath]

Hi @davidbaumwald

Sure I've just refreshed the patch to do so ;)

[whyisjake]

In 48133:

Comments: Ensure the proper comment count and pages for unapproved comments.

Previiously, unapproved comments can alter the comment count, returning incorrect page numbers.

Fixes #8973.

Props GregMulhauser, dd32, ryan, mrmist, hakre, solarissmoke, billerickson, ericlewis, SergeyBiryukov, chriscct7, dossy, lukecavanagh, renggo888, jdorner, matjack1, pento, audrasjb, imath, davidbaumwald, whyisjake.

[SergeyBiryukov]

In 48140:

Comments: Remove wp_get_include_unapproved_comments_argument() for now.

The function seems too specific and low-level for an abstraction, is only used in two places, and does not provide a significant benefit in terms of reducing code duplication.

Follow-up to [48133].

See #8973.