Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35419 closed defect (bug) (fixed)

Incorrect comment pagination when comment threading is turned off

Reported by: jmdodd's profile jmdodd Owned by: boonebgorges's profile boonebgorges
Milestone: 4.4.2 Priority: normal
Severity: normal Version: 4.4
Component: Comments Keywords: fixed-major
Focuses: template Cc:

Description

In comments_template(), the comment count for pagination is queried with the 'parent' = 0 parameter. When threading is turned off, the comment count for pagination still excludes child comments from the query. The number of found comments can be smaller than the actual number of comments, resulting in pagination issues because the total is incorrect.

Attachments (3)

35419.diff (2.4 KB) - added by jmdodd 8 years ago.
35419-src.diff (608 bytes) - added by jmdodd 8 years ago.
35419-unit-tests.diff (1.8 KB) - added by jmdodd 8 years ago.

Download all attachments as: .zip

Change History (20)

@jmdodd
8 years ago

@jmdodd
8 years ago

#1 @jmdodd
8 years ago

  • Keywords has-patch has-unit-tests added

I should have broken the src and unit test diffs apart before uploading.

#2 @azaozz
8 years ago

  • Milestone changed from Awaiting Review to 4.4.2

#3 @boonebgorges
8 years ago

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

In 36275:

Ignore hierarchy in pagination calculation when comment threading is disabled.

In order to calculate comment pagination when newest comments are displayed
first, comments_template() must perform a separate query to determine the
total number of paginating comments available on a post. See [34729], #8071,
pagination calculation - can be defined as a top-level comment, or a comment
with parent=0. However, when comment threading is disabled, yet comments
exist in the database that have parents, all comments - even those with a
parent - are "paginating". (This typically happens when comments threading was
once enabled, but has since been turned off.) As such, the total-paginating-
comments query should only be limited to top-level comments when
'thread_comments' is disabled.

Props jmdodd.
Fixes #35419.

#4 @boonebgorges
8 years ago

  • Keywords fixed-major added; has-patch has-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.4.2.

#5 follow-up: @navycs
8 years ago

My site had 75 comments per page -- now the number is an arbitrary number north of 100. The huge problem is that cpages that were created with the number set at 75 are now blank and they are being reindexed as blank pages -- I have a lot of them -- how can I implement this fix? Not sure I can wait until the release of 4.4.2 as Google does not seem to be impressed with the no content pages version 4.4 has created.

To add: Deleting the code in red and adding the green code -- if that is how it is done -- in file 35419-src.diff did not appear to fix my issue.

#6 in reply to: ↑ 5 @boonebgorges
8 years ago

Replying to navycs:

My site had 75 comments per page -- now the number is an arbitrary number north of 100. The huge problem is that cpages that were created with the number set at 75 are now blank and they are being reindexed as blank pages -- I have a lot of them -- how can I implement this fix? Not sure I can wait until the release of 4.4.2 as Google does not seem to be impressed with the no content pages version 4.4 has created.

To add: Deleting the code in red and adding the green code -- if that is how it is done -- in file 35419-src.diff did not appear to fix my issue.

Sorry to hear that you're having problems. Manually patching your installation, as you've described (deleting the code in red and adding the green code) should work in this case. If it does not, it's possible that you have a different problem. But it's difficult to tell from the description you've given.

Can you tell me whether you've had comment threading turned on in the past? Do you have it enabled or disabled now? Do you have access to the database? If so, can you tell me whether there are rows in the wp_comments table which contain comment_parent other than 0?

#7 @navycs
8 years ago

I have had my blog in Wordpress since around 2006, I do not recall ever using the threading option for comments -- it is not currently set.

I do have a number of comment_parent replies that contain a number other than zero. Most of them appear to be replies I made to questions made back in the 2009 time frame.

Edit -- changed all to most of them appear. Some are more recent.

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

#8 @boonebgorges
8 years ago

@navycs Thanks for the additional details. Can you cross-reference these comments-with-parents against the mismatched pagination you're seeing? In other words: if you're seeing 100 comments on page, when you should be seeing 75, is it the case that the 100 comments contains 25 that are comments-with-parents? Or does it appear that there is no connection?

#9 follow-up: @navycs
8 years ago

You may be hitting the limits of my database query knowledge, but, yes, it appears close to what you are asking -- it seems that every one of my personal comments are a reply because each has a comment_parent number -- a number like 135501 for example. But when I count each of my comments on a page and subtract them from the page's total count, it does not equal 75 -- one page equaled 68 and another 81 -- now, there are a random few comments with the parent number that belong to others, but not a lot as compared to my own.

I do know for a fact that I have not had the threading option selected over the last three or more years for sure -- don't remember ever having it on, truthfully, but those comments still have the numbers associated. No idea why.

Should I implement both 35402 and 35419 together and report back?

#10 @navycs
8 years ago

I have implemented both 35402 and 419 with no change realized.

Adding my specific settings:

"Break comments into pages with 75 top level comments per page and the first page displayed by default"

"Comments should be displayed with the older comments at the top of each page"

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

#11 in reply to: ↑ 9 @boonebgorges
8 years ago

Replying to navycs:

You may be hitting the limits of my database query knowledge, but, yes, it appears close to what you are asking -- it seems that every one of my personal comments are a reply because each has a comment_parent number -- a number like 135501 for example. But when I count each of my comments on a page and subtract them from the page's total count, it does not equal 75 -- one page equaled 68 and another 81 -- now, there are a random few comments with the parent number that belong to others, but not a lot as compared to my own.

Based on this, it sounds to me like my hypothesis is probably correct. The key question is not how many comments on a page *belong to you*, it's how many *have parents*. My guess is that the number of comments on a page, minus the number that have parents (whether authored by you or someone else) equals 75.

If that's true, then [36275] should resolve your issue. I'm not sure how to explain the fact that it's not. The only way I'd be able to tell more is by doing some debugging on what query is taking place. I'm not sure how well-equipped you are to help with this sort of debugging. Specifically, I'd want to know the value of the $comment_args and $top_level_comment_args just before line 1334 here: https://core.trac.wordpress.org/browser/tags/4.4.1/src/wp-includes/comment-template.php?marks=1334#L1300 If you know how to provide that, it'd be great, but if you don't, I'm not going to tell you because I don't want to risk you crashing your site :)

Side note - [36226] may be related.

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

#12 @navycs
8 years ago

The 36275 looks exactly like 35419 which I have implemented -- should the test (second part of 36275) also be included?

To the debugging, I have no idea. That would be testing my limits for sure :)

#13 @boonebgorges
8 years ago

The 36275 looks exactly like 35419 which I have implemented -- should the test (second part of 36275) also be included?

No, the test doesn't matter (it's probably not included on your installation anyway). Another bit to take a look at is [36226] - this might be required for [36275].

#14 @navycs
8 years ago

WHOO HOO!!!! Added 36226 and BOOM!

Now at 75 per page.

Thank you!

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

#15 follow-up: @boonebgorges
8 years ago

Great! Sorry for not being clearer before on the relationship between the two changesets.

#16 in reply to: ↑ 15 @navycs
8 years ago

Replying to boonebgorges:

Great! Sorry for not being clearer before on the relationship between the two changesets.

No need to apologize, nothing but huge kudos to you and the Wordpress team!

BZ, boonebgorges!

#17 @dd32
8 years ago

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

In 36362:

Comments: Ignore hierarchy in pagination calculation when comment threading is disabled.

In order to calculate comment pagination when newest comments are displayed
first, comments_template() must perform a separate query to determine the
total number of paginating comments available on a post. See [34729], #8071,
pagination calculation - can be defined as a top-level comment, or a comment
with parent=0. However, when comment threading is disabled, yet comments
exist in the database that have parents, all comments - even those with a
parent - are "paginating". (This typically happens when comments threading was
once enabled, but has since been turned off.) As such, the total-paginating-
comments query should only be limited to top-level comments when
'thread_comments' is disabled.

Merges [36275] to the 4.4 branch.
Props jmdodd.
Fixes #35419.

Note: See TracTickets for help on using tickets.