Comment walker calls get_comment_link with invalid arguments

[peterwilsoncc]

The comment walker calls get_comment_link such the the returned link is always includes the first page of comment pagination links, ie comment-page-1 (literally one, not any positive integer).

The pagination portion of the $args passed by the walker always sets the page to 1.

array(15) {

  // snip 

  ["page"]=>
  int(1)
  ["per_page"]=>
  string(1) "5"

  // snip
}

[peterwilsoncc]

Discovered this checking out #34068. Could be a dupe but my reading is the earlier ticket is about the comment pagination portion of comment links always been present.

[boonebgorges]

See #8071. The way it used to work is that comments_template() would fetch *all* comments, and Walker_Comment would handle pagination. [34561] changed this behavior so that only the proper comments are fetched in the initial query. Then, to fool Walker_Comment into displaying all and only the comments we pass to it, we pass 'page=1'.

It appears that the problem with get_comment_link() occurs only when a theme doesn't provide a comment-formatting 'callback' param, or when the callback explicitly passes $args to get_comment_link(). (I guess I'd been testing with default themes - like twentytwelve - that get_comment_link() without any parameters.) Can you share your setup when testing? What theme are you using?

I'll need to do some more analysis, but it may be that in order to fix this, get_comment_link() will need to ignore some of the $args passed to it in at least some cases. Grumble grumble grumble.

[peterwilsoncc]

Replying to boonebgorges:

It appears that the problem with get_comment_link() occurs only when a theme doesn't provide a comment-formatting 'callback' param, or when the callback explicitly passes $args to get_comment_link(). (I guess I'd been testing with default themes - like twentytwelve - that get_comment_link() without any parameters.) Can you share your setup when testing? What theme are you using?

Of course:

• trunk install using twenty fifteen
• five comments per page
• occurred in both first- and last page first modes
• test post had twelve comments

twenty fifteen displays comments with:

wp_list_comments( array(
    'style'       => 'ol',
    'short_ping'  => true,
    'avatar_size' => 56,
) );

[boonebgorges]

In 34716:

Use array hash notation for get_comment_link() $args.

See #34068, #34073.

[boonebgorges]

34073.diff​ is a pass at fixing this ticket as well as #34068. (They're slightly different, but the same technique has to be used in both places.)

The trick in the case of wp_list_comments() was (a) to detect that we were inside of a comments_template() loop (something we already know because of the $wp_query->max_num_comment_pages check), and if so, (b) pass the new cpage parameter down through the stack, all the way to get_comment_link(). When get_comment_link() sees cpage, it trusts it - it doesn't do any calculation at all.

As for get_comment_link() when called outside the context of a comment loop (the problem in #34068), the logic is much the same - calculate total pages, and then remove the 'cpage' query arg when looking at the newest/oldest page, depending on your settings - but it had to go into the calculation logic of the function itself. This meant reorganizing the function quite a bit, mainly for readability, but also to break apart some hairy if/else chains.

My eyes are nearly bleeding from writing tests and doing manual tests of the approach in the patch. I feel pretty confident about it, but a second (and third, and fourth) opinion would be swell.

[peterwilsoncc]

Testing with the same set up as earlier:

• Twenty fifteen
• post with twelve comments
• five comments per page
• comment widget displays latest five comments

Last page of comments first:

• oldest three comment widget links are incorrect
• links within comment walker are correct
• page portion not shown for comments on default page

First page of comments first works as expected in both comment walker and widget.

There is a ​spread sheet.

Edit: fixed references to comment walker (initially wrote loop)

[boonebgorges]

Thanks, peterwilsoncc!

page portion not shown for comments on default page

This was originally a design decision, but ViperBond007 pointed out on #8071 that having the 'cpage'/comment-page-x var here is important to ensure that comment pagination links are permanent when new comments are added.

oldest three comment widget links are incorrect

Can you clarify this? Does this mean that two of the widget links are working? Where do the incorrect links go? Where *should* they go? Are your comments threaded? This may be an unrelated (and existing) bug, perhaps #13939.

[peterwilsoncc]

Replying to boonebgorges:

oldest three comment widget links are incorrect

Can you clarify this? Does this mean that two of the widget links are working? Where do the incorrect links go? Where *should* they go? Are your comments threaded? This may be an unrelated (and existing) bug, perhaps #13939.

That's correct, in last page first:

• two newest comment links worked (comments 11 & 12) linked to page 0
• following three links (comments 8 - 10) linked to page 2 but displayed on page 0

Based on the discussion in #29462 and ViperBond007's comments, it looks like comment pagination in last page first has regressed but I think you mentioned in Slack you were looking into that.

[boonebgorges]

In 34729:

Simplify pagination logic in comments_template().

[34561] "fixed" the problem of newest-first comments showing fewer than
'per_page' comments on the post permalink when the total number of comments
was not divisible by 'per_page'. See #29462. But this fix caused numerous
other problems. First, comment pages reported by get_page_of_comment()
(which expects comment pages to be filled oldest-first) were no longer correct.
Second, and more seriously, the new logic caused comments to be shifted
between pages, making their permalinks non-permanent.

The current changeset reverts the changed behavior. In order to preserve the
performance improvements introduced in [34561], an additional query must be
performed when 'default_comments_page=newest' and 'cpage=0' (ie, you're viewing
the post permalink). A nice side effect of this revert is that we no longer
need the hacks required to determine proper comment pagination, introduced in
[34561].

See #8071. See #34073.

[boonebgorges]

In 34735:

Ensure that comment permalinks reflect pagination.

After [34561], wp_list_comments() no longer passed all of a post's comments
to Walker_Comments. As a result, calls to get_comment_link() occurring
inside the comment loop had insufficient context to determine the proper
'cpage' value to use when generating comment permalinks. This, in turn, caused
comment permalinks to behave erratically.

The current changeset addresses the problem as follows:

• get_comment_link() now accepts a 'cpage' parameter. When present, 'cpage' will be used to build the comment permalink - no automatic calculation will take place.
• When called within the main loop, wp_list_comments() calculates the proper 'cpage' value for comments in the loop, and passes it down to get_comment_link().
• cpage and comment-page-x query vars are generally required in comment permalinks (see #34068), but an exception is made when 'default_comment_page=oldest': the bare post permalink will always be the same as cpage=1, so cpage is excluded in this case.

Props peterwilsoncc for assiduous spreadsheeting.
Fixes #34073.

[boonebgorges]

In 34792:

Remove dead code added to get_comment_link() in [34735].

See #34073.

[rachelbaker]

In 36343:

Comments: Remove unused $default_comments_page variable in get_comment_link().

Left in r34735, fetches the default_comments_page option twice since this variable is unused.

See #34073 and #35511.

Props Latz.