WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 5 months ago

#18603 assigned defect (bug)

Comments on pages which exceed paginate settings create erroneous permalinks

Reported by: msagman Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.2.1
Component: Comments Keywords: needs-patch needs-testing good-first-bug
Focuses: Cc:

Description

On my website, clicking on any of the Recent Comments that possess large numbers of comments exceeding paginate settings (50 comments) present erroneous permalinks. Only the posts containing enough comments on page 3 or higher present these improperly constructed links that take users to the wrong page (a page that does not contain the comment in question). The problem does not occur on pages containing a limited number of comments. So far, we have removed our Thesis theme and changed to the standard WP default theme. And we've deactivated all plugins. Yet neither solution appears to resolve the issue. Thanks for your consideration. You may view this issue live at http://www.dogfoodadvisor.com

Attachments (2)

wp_comments_trac_18603.sql (8.8 KB) - added by ambrosiawt 5 months ago.
Dump of comments table that can be used to demonstrate the problem.
18603-partial.diff (444 bytes) - added by ambrosiawt 5 months ago.
Only a partial patch for discussion. Not for live use.

Download all attachments as: .zip

Change History (24)

#1 @SergeyBiryukov
5 years ago

So get_page_of_comment() returns wrong number?

Could not reproduce this on a clean install with 5 pages of comments. Looks more like a support request to me.

#2 @solarissmoke
5 years ago

  • Keywords reporter-feedback close added

From your description it sounds like the issue is with the "Recent Comments" sidebar widget, in which case you need to talk to the author of that plugin. If the issue can be reproduced in a clean installation of Wordpress, without any plugins installed, please give the steps to reproduce.

#3 @solarissmoke
5 years ago

Oops, just realised Recent Comments is part of core, not a plugin. In any case, I can't reproduce either.

#4 @msagman
5 years ago

Thanks for looking. I've left a thread on the support forum. May I ask, is it possible some other file may have been modified by a plugin and have been corrupted (for example, htaccess) that could be causing this? Have absolutely no idea where to look. Thanks.

#5 @SergeyBiryukov
5 years ago

If it was .htaccess, the links would at least look correct when moving cursor over them.

One of the problem links contains comment-page-14/#comment-37218, whereas the proper link for that comment is comment-page-17/#comment-37218. So I suspect get_page_of_comment() returns a wrong number for some reason.

#6 @billerickson
5 years ago

So could this be caused by #8973?

I've been trying to help Mike with this issue. To give you a little more info on it:

  • I cannot reproduce it on a clean WordPress installation
  • When we disable all plugins and switch to default theme, the issue still persists

#7 @msagman
5 years ago

I think we've found the problem. And it does appear to be located in the core function of WP in the get_page_of_comment() lines. You should be able to reproduce the problem. As I understand it, it has to do with the logic of the way WP computes the page number. It appears to have something to do with whether the comments are approved or not. So, it only affects the posts containing the largest number of comments and the longest history.

Here's a copy of what my software technician reported to me. And after testing his changes, the problem disappeared and was fixed:

In /wp-includes/comment.php on line 813:
$oldercoms = $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(comment_ID) FROM $wpdb->comments WHERE comment_post_ID = %d AND comment_parent = 0 AND comment_approved = '1' AND comment_date_gmt < '%s'" . $comtypewhere, $comment->comment_post_ID, $comment->comment_date_gmt ) );

In order to determine the correct page number for the most recent comment link you must not include approved comments and parent comments in the query… After changing the query above to the following I was able to determine the correct number of $oldercoms variable, thus affecting the math on line 820, by executing:

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

Finally, I changed the math logic to:
return round( ( $oldercoms + 1 ) / $argsper_page? );

Thanks for your help.

#8 @msagman
5 years ago

  • Keywords reporter-feedback close removed

#9 @msagman
5 years ago

Please forgive my lack of understanding of how the review process works, but are you awaiting a response from me? Do I need to do something else? Have not seen any further responses or entries since my last comment. Is this bug still under review? I removed the two tags. Should I have not done that? Please be sure to see my last entry which shows you how to reproduce this same issue in a clean install. You must have sufficient comments on a number of comment pages. WP uses the wrong logic by including both approved and parent comments in the query. Please notice the get_page_of_comment() function is not set up properly. Thanks.

#10 @SergeyBiryukov
5 years ago

Thank you for the report, now it makes sense.

I'll look closer into this and #8973.

#11 @msagman
5 years ago

The problem I describe in this bug report is NOT fixed with the above code I previously submitted. I have figured out what you need to do to reproduce the issue. The problem of defective pagination only occurs when there has been more threaded responses than the number of comments per page set in the paginate feature. Even if WP is set for no threaded responses.

Today in my Wordpress research my technician concluded that the comment page calculation was thrown off when the comment's parent column was set. We tested this together and found that Wordpress sets the comment parent when replying to user's comments as an admin regardless of the threading setting under "Discussions" settings.

To fix this we ran the following SQL query on the comments table:

UPDATE wp_comments SET comment_parent = 0 WHERE comment_parent > 0

#12 @billerickson
5 years ago

  • Keywords dev-feedback added

I think I'm having the same issue on my more popular posts as well. On this post ( http://www.billerickson.net/wordpress-metaboxes ) I have 128 comments, 50 of which are top level comments. In Settings > Discussion I have it set to 50 top level comments per page.

But when I view the comments from the backend and look at their URL (or if I were using the Recent Comments widget), they link to the second page where the comment isn't.

The second to last comment (logesh) has this URL: http://www.billerickson.net/wordpress-metaboxes/comment-page-2/#comment-5608
The last comment (billerickson) has this URL: http://www.billerickson.net/wordpress-metaboxes/comment-page-2/#comment-5623

#13 @ericlewis
4 years ago

This sounds a duplicate of #8973, which @billerickson suggested.

The issue can be replicated by making multiple comments on a post until the comments pagination # is hit, after which the new comment URL will still be returned as comment-page-1. Unless there's a different bug reproduction process which hasn't been realized and detailed yet.

#14 follow-up: @SergeyBiryukov
4 years ago

  • Keywords needs-testing added

Not sure if this is an exact duplicate of #8973. That ticket fixes the issue with including current user's unmoderated comments, while this one has to do with comment threading (I didn't have any comments on the reporter's site and still noticed a problem mentioned in comment:5).

#15 in reply to: ↑ 14 @ericlewis
4 years ago

  • Keywords needs-steps-to-reproduce added

Replying to SergeyBiryukov:

Not sure if this is an exact duplicate of #8973. That ticket fixes the issue with including current user's unmoderated comments, while this one has to do with comment threading (I didn't have any comments on the reporter's site and still noticed a problem mentioned in comment:5).

Yep, you're right.

Could reproduce this after tinkering with some comments. Steps-to-reproduce would help.

Version 0, edited 4 years ago by ericlewis (next)

#16 @jaredatch
4 years ago

  • Cc jared@… added

#17 @jaredatch
3 years ago

Any update on this?

What do we need for a fix? I have a client with this issue so I can do testing/provide feedback if needed.

Basically if someone has pagination and thousands of comments, the Recent Comments widget is worthless as it is now.

#18 @wonderboymusic
23 months ago

  • Keywords needs-patch added; dev-feedback needs-testing needs-steps-to-reproduce removed
  • Milestone changed from Awaiting Review to Future Release
  • Owner Mike Sagman http://www.dogfoodadvisor.com deleted
  • Status changed from new to assigned

#19 @chriscct7
5 months ago

  • Keywords needs-testing good-first-bug added

Need steps to replicate

@ambrosiawt
5 months ago

Dump of comments table that can be used to demonstrate the problem.

#20 @ambrosiawt
5 months ago

Steps to replicate. During Contributor Day in Philadelphia today, I spent some time on this ticket. I was able to recreate what I think the situation is, although I'm recreating as a situation where nested comments were on, and then later turned off. I think there is still an aspect of this that involves a situation where nested comments were off to begin with. I will continue looking at that. I started looking at a possible solution, and I think I have part of the issue identified, but not all of it.

To recreate issue:

Clean WordPress install
Set "Enable threaded (nested) comments" to ON
Created 11 new top-level comments (12 total on Hello World)
Created 9 2nd level comments on the first top-level comment I created
Created 1 2nd level comment on a different top-level comment I created
Created 1 2nd level comment on the last top-level comment I created
Changed "Break comments into pages with" to 6 entries
Set "Enable threaded (nested) comment"s to OFF


Result:

The link to the very last 2nd level comment is wrong... says page 2. If you click on it, you don't actually see the comment on the page.
If you click on the post, the comment shows on the page just fine.
The link on the comment itself is correct (page-2/comments-23), but the link in Recent Comments says page 3 (which is incorrect at this point.
Since Nested is off, seems like there should really be 4 total pages of comments, not 2, in this case.

I attached a dump (https://core.trac.wordpress.org/attachment/ticket/18603/wp_comments_trac_18603.sql) of the comments table which you can, on a fresh install, drop the existing table and then import this one and then change "Break comments into pages with..." to 6, and turn nested comments off.

@ambrosiawt
5 months ago

Only a partial patch for discussion. Not for live use.

#21 @ambrosiawt
5 months ago

I uploaded a patch to demonstrate what I thought would solve at least part of the problem, but I've changed my mind about this... I was thinking that it looked pretty simple and the purpose of what I was removing was not necessary due to code before it. Now I think that it's not so simple. :)

I do want to explain where my head is at, however, since I was after a specific purpose here. The situation that we're having here is certainly that in some cases we can have values in the parent column, for example if you have threaded comments on, and after a while decide to turn them off. When the setting is changed, none of the rows with a value in the parent column are changed, just the behavior of how the comments are treated (all threaded items become top level).

I don't think it makes sense to change those rows. For example, if someone turns threading off, and then realized they didn't mean to make that change, turning it back on should retain the original threading data, which means the parent column in those rows needs to be retained. If others think differently, please comment.

That said, it seems like for this piece of code we could just drop the parent=0 from the query, but it doesn't solve the whole problem and seems to be problematic still. I may try another patch shortly.

#22 @dossy
5 months ago

I'm hoping to fix this bug as part of the work I'm doing on #8973, as until that issue is addressed and fixed, this issue will always be broken under specific conditions.

Once I get #8973 fully fixed, then I'll look more closely at the STR for this issue - thanks for confirming that the issue still exists.

Note: See TracTickets for help on using tickets.