get_page_of_comment returns wrong page number when changing threading level

[laceous]

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

[laceous]

initial patch

[laceous]

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.

[nacin]

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]

second stab at a patch

[laceous]

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]

better filter

[wonderboymusic]

we can analyze this as part of our comments autopsy

[boonebgorges]

In 34805:

Only count top-level comments when calculating threaded pagination.

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

See #13939, #11334.

[boonebgorges]

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,

2. 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.

[boonebgorges]

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.

[boonebgorges]

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.

[SergeyBiryukov]

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

[boonebgorges]

In 34815:

Rechristen the get_page_of_comment filter.

See [34808].

Props DrewAPicture, SergeyBiryukov.
Fixes #13939.