WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 12 months ago

#13939 closed defect (bug) (fixed)

get_page_of_comment returns wrong page number when changing threading level

Reported by: laceous Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.0
Component: Comments Keywords:
Focuses: Cc:

Description

Initial discussion settings:

  • Enable threaded comments 2 levels deep
  • 5 top-level comments per page, first page by default
  • Older comments at top of each page

If I have comments on a post like:

cpage 1

  • comment-1
  • comment-2
  • comment-3
  • comment-4
  • comment-5
    • comment-6
$page = get_page_of_comment(6); // returns 1 which is the correct page

If I modify the initial discussion settings by disabling threading (e.g. changing the threading level to 1), the comments change to:

cpage 1

  • comment-1
  • comment-2
  • comment-3
  • comment-4
  • comment-5

cpage 2

  • comment-6
$page = get_page_of_comment(6); // returns 1 which is incorrect

This function doesn't follow the same algorithm as what is used to display the comments

This can potentially happen whenever you decrease the threading level from x to x-n

Attachments (3)

get_page_of_comment.diff (1.4 KB) - added by laceous 6 years ago.
initial patch
get_page_of_comment.2.diff (4.4 KB) - added by laceous 6 years ago.
second stab at a patch
get_page_of_comment.3.diff (4.4 KB) - added by laceous 6 years ago.
better filter

Download all attachments as: .zip

Change History (16)

@laceous
6 years ago

initial patch

#1 @laceous
6 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from 3.1 to 3.0.1

I think in the example above I was running into two different issues in this function. To repro the issue I was initially reporting you'd actually need to add a few more sub-replies under "comment-5."

I changed get_query_var('comments_per_page') to get_option('comments_per_page') because the first wasn't returning anything. I may have been calling this function before the query_vars are populated. If query_vars are important here, then maybe we can try that first and then fall back to get_option?

I also tweaked the SQL in the case that threading isn't enabled.

#2 @nacin
6 years ago

  • Milestone changed from 3.0.1 to 3.1

get_query_var('comments_per_page') is handled in WP_Query like so:

		if ( !isset($q['comments_per_page']) || $q['comments_per_page'] == 0 )
			$q['comments_per_page'] = get_option('comments_per_page');

Moving back to 3.1, as this isn't major or a regression.

@laceous
6 years ago

second stab at a patch

#3 @laceous
6 years ago

I found a couple more issues with the get_page_of_comment function. Here's a quick recap:

  1. When calling this function from the init action, get_query_var('comments_per_page') isn't available yet. get_option('comments_per_page') is, however. This isn't a deal-breaker because you can pass in an arg to override comments_per_page.
  2. If you have threaded comments and then disable threading, the comment page returned can be incorrect. (initial issue reported; ignore the x to x-n... I was wrong there)
  3. The comment type was not allowing "pings" which is one of the values wp_list_comments allows
  4. This function wasn't taking into account unapproved comments for the current user like the comments_template function does.
  5. There's lots of ways to customize how comments are displayed (custom walkers, callbacks, filters, etc) so it makes sense to return with a filter from this function.

The patch I just submitted attempts to correct these issues.

@laceous
6 years ago

better filter

#4 @nacin
6 years ago

  • Milestone changed from Awaiting Triage to Future Release

#5 @chriscct7
2 years ago

  • Keywords dev-feedback added

#6 @wonderboymusic
12 months ago

  • Keywords needs-patch added; has-patch needs-testing dev-feedback removed
  • Milestone changed from Future Release to 4.4

we can analyze this as part of our comments autopsy

#7 @wonderboymusic
12 months ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

#8 @boonebgorges
12 months ago

In 34805:

Only count top-level comments when calculating threaded pagination.

The change in [34535] did not properly account for threading.

See #13939, #11334.

#9 @boonebgorges
12 months ago

In 34806:

Use 'comments_per_page' option as fallback in get_page_of_comment().

The function now uses the following order of precedence when calculating
comment pagination: 1. the 'per_page' value passed in the $args array,

  1. the 'comments_per_page' query var in $wp_query, and 3. the

'comments_per_page' setting from options-discussion.php. This change allows
get_page_of_comment() to return an accurate value before the main query
has been run.

Props laceous.
See #13939.

#10 @boonebgorges
12 months ago

In 34808:

Introduce 'page_of_comment' filter.

This filter allows developers to modify the output of get_page_of_comment().

As a side effect of this new filter, comment page numbers will always be
returned as integers. Previously, they would sometimes be returned as floats -
eg float(2.0) instead of int(2).

Props laceous.
See #13939.

#11 @boonebgorges
12 months ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from assigned to closed

I've addressed a few of these issues in the commits above. As for what remains:

If you have threaded comments and then disable threading, the comment page returned can be incorrect. (initial issue reported; ignore the x to x-n... I was wrong there)

I can't reproduce this. The logic of the function was largely rewritten in [34535], and it's possible that the rewrite fixed things.

The comment type was not allowing "pings" which is one of the values wp_list_comments allows

The value of 'type' is now passed directly to WP_Comment_Query, so you can use whatever values are accepted there.

This function wasn't taking into account unapproved comments for the current user like the comments_template function does.

I'm inclined to do nothing about this. I thought about adding status and include_unapproved args to the function, and passing them to WP_Comment_Query. But at this point, it begins to confuse what the function (and indeed, what comment pagination is for). In theory, the page on which a comment appears is supposed to be immutable - it shouldn't change over time, because the comment page is used as part of the comment permalink. This is not really true in practice, because of things like unapproved-to-approved comments, changed comments_per_page, and so forth. But adding additional params that allow more bypassing of this logic doesn't seem right to me. In some ideal world, comments_template() shouldn't be adjusting pagination for unapproved comments either, but that's a whole other ball of wax.

#12 @SergeyBiryukov
12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Could we rename the filter to get_page_of_comment, for consistency with the function name?

#13 @boonebgorges
12 months ago

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

In 34815:

Rechristen the get_page_of_comment filter.

See [34808].

Props DrewAPicture, SergeyBiryukov.
Fixes #13939.

Note: See TracTickets for help on using tickets.