Make WordPress Core

Opened 5 years ago

Last modified 11 months ago

#13939 new defect (bug)

get_page_of_comment returns wrong page number when changing threading level

Reported by: laceous Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Comments Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:


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 5 years ago.
initial patch
get_page_of_comment.2.diff (4.4 KB) - added by laceous 5 years ago.
second stab at a patch
get_page_of_comment.3.diff (4.4 KB) - added by laceous 5 years ago.
better filter

Download all attachments as: .zip

Change History (8)

@laceous5 years ago

initial patch

comment:1 @laceous5 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.

comment:2 @nacin5 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.

@laceous5 years ago

second stab at a patch

comment:3 @laceous5 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.

@laceous5 years ago

better filter

comment:4 @nacin5 years ago

  • Milestone changed from Awaiting Triage to Future Release

comment:5 @chriscct711 months ago

  • Keywords dev-feedback added
Note: See TracTickets for help on using tickets.