Make WordPress Core

Opened 15 years ago

Closed 4 years ago

Last modified 4 years ago

#8973 closed defect (bug) (fixed)

Function get_page_of_comment() returns incorrect page numbers

Reported by: gregmulhauser's profile GregMulhauser Owned by: whyisjake's profile whyisjake
Milestone: 5.5 Priority: normal
Severity: normal Version: 2.7
Component: Comments Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:

Description

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

Attachments (8)

8973.diff (2.2 KB) - added by solarissmoke 13 years ago.
Include current user's unmoderated comments when finding comment page
8973-1.diff (3.3 KB) - added by dossy 8 years ago.
Test and updated patch to implement this for 4.4-4.5
attachments-ticket-34391.zip (86.7 KB) - added by renggo888 7 years ago.
8973.2.diff (6.1 KB) - added by matjack1 6 years ago.
Updated patch for WP 5.0 and added functionality in templates
Screenshot from 2018-04-06 15-34-19.png (59.2 KB) - added by matjack1 6 years ago.
How to replicate the bug
8973.3.diff (6.8 KB) - added by matjack1 5 years ago.
8973.4.patch (8.1 KB) - added by imath 5 years ago.
8973.5.patch (8.1 KB) - added by imath 4 years ago.

Download all attachments as: .zip

Change History (53)

#1 @GregMulhauser
15 years ago

  • Component changed from General to Comments
  • Owner anonymous deleted

#2 @DD32
15 years ago

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

#3 @ryan
15 years ago

  • Resolution set to fixed
  • Status changed from new to closed

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

#4 @ryan
15 years ago

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

#5 @mrmist
15 years ago

  • Resolution fixed deleted
  • Severity changed from major to normal
  • Status changed from closed to reopened

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.

#6 @ryan
15 years ago

  • Milestone changed from 2.7.1 to 2.8

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.

#7 @GregMulhauser
15 years ago

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

#8 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added; comments paging removed

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

#9 @hakre
15 years ago

  • Summary changed from Function get_page_of_comment() returns incorrect page numbers -- fix included to Function get_page_of_comment() returns incorrect page numbers

#10 @Denis-de-Bernardy
15 years ago

  • Milestone changed from 2.8 to Future Release

#11 @Denis-de-Bernardy
15 years ago

  • Milestone changed from Future Release to 2.9

#12 @hakre
15 years ago

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.

#13 @azaozz
14 years ago

  • Milestone changed from 2.9 to Future Release

No patch.

#14 @solarissmoke
13 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

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
13 years ago

Include current user's unmoderated comments when finding comment page

#15 @billerickson
13 years ago

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

#16 @ericlewis
12 years ago

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.

#17 @SergeyBiryukov
12 years ago

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

#18 @chriscct7
9 years ago

  • Keywords needs-refresh added; has-patch removed

@dossy
8 years ago

Test and updated patch to implement this for 4.4-4.5

#19 @dossy
8 years ago

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.

Last edited 8 years ago by dossy (previous) (diff)

#20 @chriscct7
8 years ago

  • Owner set to chriscct7
  • Status changed from reopened to reviewing

This ticket was mentioned in Slack in #core by dossy. View the logs.


8 years ago

#22 @lukecavanagh
7 years ago

@dossy

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

#23 @jdorner
7 years ago

Tried to reproduce, but need more info (steps)

@matjack1
6 years ago

Updated patch for WP 5.0 and added functionality in templates

@matjack1
6 years ago

How to replicate the bug

#24 @matjack1
6 years ago

  • Keywords has-patch added; needs-refresh removed

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:

https://core.trac.wordpress.org/raw-attachment/ticket/8973/Screenshot%20from%202018-04-06%2015-34-19.png

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.

#25 @SergeyBiryukov
6 years ago

  • Milestone changed from Future Release to 5.0
  • Owner changed from chriscct7 to SergeyBiryukov

#26 @matjack1
6 years ago

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.

#27 @pento
5 years ago

  • Milestone changed from 5.0 to 5.1

#28 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


5 years ago

#30 @audrasjb
5 years ago

  • Keywords needs-refresh added

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
5 years ago

#31 @matjack1
5 years ago

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 :)

#32 @matjack1
5 years ago

  • Keywords needs-refresh removed

#33 @pento
5 years ago

  • Owner changed from SergeyBiryukov to pento
  • Status changed from reviewing to accepted

#34 @pento
5 years ago

  • Keywords needs-refresh added; dev-feedback removed
  • Milestone changed from 5.2 to 5.3

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!

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


5 years ago

@imath
5 years ago

#36 @imath
5 years ago

  • Keywords needs-refresh removed

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.

This ticket was mentioned in Slack in #core by pento. View the logs.


4 years ago

#38 @pento
4 years ago

  • Owner pento deleted
  • Status changed from accepted to assigned

#39 @davidbaumwald
4 years ago

  • Keywords needs-testing dev-feedback added
  • Milestone changed from 5.3 to Future Release

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.

#40 @davidbaumwald
4 years ago

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

@imath
4 years ago

#41 @imath
4 years ago

  • Milestone changed from Future Release to 5.5

Hi @davidbaumwald

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

This ticket was mentioned in Slack in #core by marybaum. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#44 @whyisjake
4 years ago

  • Owner set to whyisjake
  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#45 @SergeyBiryukov
4 years ago

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.

Note: See TracTickets for help on using tickets.