Pass the comments query paged arg to functions get_next_comments_link and get_previous_comments_link

[SantosGuillamot]

What?

According to this report, the solution introduced in this pull request to solved the mentioned bug, where cpage was updated based on the query, is causing problems with plugins relying on it being 0 or undefined.

This pull request explores another alternative to solve the initial bug without modifying cpage.

It needs to be tested with this other core pull request, which modifies the relevant functions to accept the new parameter.

How?

Pass the paged argument from the query to get_next_comments_link and get_previous_comments_link so they only read the cpage when it is not defined. A check like the following must be added in the core functions:

	if ( empty( $page ) ) {
		$page = get_query_var( 'cpage' );
	}

Testing Instructions

It needs to be tested with this other core pull request, which modifies the relevant functions to accept the new parameter.

1. Add comments to a post.
2. Go to Settings -> Discussion and set "Break comments into pages with 2 top-level comments per page and the
   last page displayed by default.
3. Go to the post and check that the pagination links are correct.

[gziolo]

Is there a similar fix required for the paginate_comments_links in Comments Pagination Numbers block?

It looks like it also should get covered as it calls get_query_var:

https://github.com/WordPress/wordpress-develop/blob/c8b744d12b2af5bc8eb01b2e8782c20d681fdadd/src/wp-includes/link-template.php#L3249

[SantosGuillamot]

Is there a similar fix required for the paginate_comments_links in Comments Pagination Numbers block?

I believe you are right and we should cover paginate_comments_links as well if we decide to follow this approach.

[gziolo]

@ockham, can you confirm this approach would work from your perspective? As far as I'm concerned, it might resolve the issue.

[ockham]

Apologies for the late reply. Yeah, I think making the argument explicit should be the way forward to make this work. We will have to implement the "other side" -- i.e. the changes to get_next_comments_link and get_previous_comments_link in Core -- to know for sure, though.

@SantosGuillamot Do you have some bandwidth to take that on? I'm a bit swamped with other things these days 😅

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: ockham <bernhard-reiter@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[SantosGuillamot]

I have created this alternative pull request in core, which removes the problematic set_query_var while keeping the pagination blocks working as expected.

That PR plus this one should solve both problems.

Is there a similar fix required for the paginate_comments_links in Comments Pagination Numbers block?

Regarding this, I've been taking a deeper look and I believe it is not needed. Let me explain my reasoning:

• In the pagination numbers block, we are populating a query with current storing the page: link.
• paginate_comments_links is using get_query_var, but only to define the "defaults": link.
• Those defaults are overridden with the values passed in the args, which contains the proper page: link.

[ockham]

As mentioned in WordPress/wordpress-develop#7275 (comment), we need to add some safeguards to make sure that we're not passing this extra argument if get_next_comments_link() -- as provided by whatever WordPress version is running -- doesn't support the additional parameter yet.

To do so, I think our best course of action is PHP's ReflectionFunction:

Something like (untested):

Suggested change

	$next_comments_link = get_next_comments_link( $label, $max_page, $comment_vars['paged'] );
	$get_next_comments_link_reflection = new ReflectionFunction( 'get_next_comments_link' );
	if ( $get_next_comments_link_reflection->getNumberOfParameters() >= 3 ) {
	$next_comments_link = get_next_comments_link( $label, $max_page, $comment_vars['paged'] );
	} else {
	$next_comments_link = get_next_comments_link( $label, $max_page );
	}

and accordingly for get_previous_comments_link.

[SantosGuillamot]

Do we really need to add this safety check? If I am not mistaken, when passing the third argument to a function that only accepts two, PHP will just ignore it, right? At least, that's what I can see from my testing.

[gziolo]

There are functions like func_get_args that allow to handle any number of params, so as far as I tested it, providing an additional param that isn't handled works just fine.

<?php
function foo()
{
    $numargs = func_num_args();
    echo "Number of arguments: $numargs \n";
    if ($numargs >= 2) {
        echo "Second argument is: " . func_get_arg(1) . "\n";
    }
    $arg_list = func_get_args();
    for ($i = 0; $i < $numargs; $i++) {
        echo "Argument $i is: " . $arg_list[$i] . "\n";
    }
}

foo(1, 2, 3);
?>

[ockham]

Ah, fair enough! I was assuming that PHP would at least issue a warning or a notice if strict error reporting was enabled, but it seems that that isn't the case. (Here's a related SO thread.)

[gziolo]

Yes, it fatals on built-in functions I tested, but not on custom functions 🤷🏻

[SantosGuillamot]

I tested it directly with get_next_comments_link in a branch without the changes and it doesn't seem to break.

[ockham]

Thank you! LGTM 👍

[michalczaplinski]

I found an issue that was likely caused by changes in this PR: #65424

[ockham]

I found an issue that was likely caused by changes in this PR: #65424

I just came here to report the same thing 😕

[SantosGuillamot]

I've started a PR trying to solve that: #65435