Comments: Pass $page as argument to comments functions

[SantosGuillamot]

Follow-up to #6291

As discussed in the original pull request and ticket, it seems set_query_var should be removed. However, that change was breaking the comments pagination in some scenarios (see #6291 (review)). In this pull request I'm removing set_query_var while fixing the mentioned issue.

In order to do that, I'm passing a new $page argument to the relevant functions and only use the cpage query var if that is not defined.

This pull request has to be tested with WordPress/gutenberg#63698 (comment), which modifies the comment pagination blocks to pass the $page argument.

Trac ticket: https://core.trac.wordpress.org/ticket/60806

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

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

Core Committers: Use this line as a base for the props when committing in SVN:

Props santosguillamot, bernhard-reiter, gziolo, cybr.

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

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[SantosGuillamot]

Not sure if this unit test is valuable anymore.

[ockham]

Thank you, this is looking good! (Well, the argument order of get_next_comments_link -- max_page before page -- is a bit unfortunate, as is the inconsistency with get_prev_comments_link, but there's not a lot we can do about that 😅)

The tricky part is how to land this, and WordPress/gutenberg#63698. It's a bit of a 🐔🥚 problem:

• If we land this PR first, we'd re-introduce this bug.
• If we land the GB PR first, we'd pass an extraneous argument to both get_next_comments_link and get_prev_comments_link -- even thought they don't accept that extra argument yet. This will cause an error.

I think we can solve this problem via the following strategy:

1. In this PR, we only keep the changes to src/wp-includes/link-template.php -- i.e. the extra arguments added to get_next_comments_link and get_prev_comments_link. (In other words, we remove all changes that are also present in Remove query alteration from build_comment_query_vars_from_block() #6291). We can then land this PR. (We might want to add a bit of unit test coverage though.)
2. We add some safeguards to the GB PR. (I'll comment there with more detail.) We can then land that PR.
3. We wait for the GB changes to be synced to Core. This will likely happen on the Friday or Monday before Beta 1, i.e. on Sept 27 or 30.
4. In the (short) window that's left before Beta 1, we merge Remove query alteration from build_comment_query_vars_from_block() #6291.

This plan should ensure that nothing breaks at any point.

@SantosGuillamot Could you take care of item number 1.? 🙌 😊

[SantosGuillamot]

If we land the GB PR first, we'd pass an extraneous argument to both get_next_comments_link and get_prev_comments_link -- even thought they don't accept that extra argument yet. This will cause an error.

I just answered here, but I am not sure if this will cause an error. From what I understood, the extra argument passed to those functions will just be ignored, but it won't throw an error. If that's the case, I believe we could still manage everything in the same merge:

1. Merge Gutenberg PR.
2. Wait for GB changes to be synced to Core.
3. Merge this PR.

Having said that, I might be mistaken. I'll start working on adding some unit test coverage until it is clarified.

[SantosGuillamot]

I've added a couple of unit tests in this commit based on the existing ones for those functions. Let me know if you think that's not enough.

[ockham]

While it doesn't affect the tests, this is a bit misleading, so let's change it to

Suggested change

	$link = get_previous_comments_link( 'Next', 3 );
	$link = get_previous_comments_link( 'Previous', 3 );

[SantosGuillamot]

You're right. I copied it from the other tests in that file and didn't realize. I can change those as well even if they are not related to this PR.

[SantosGuillamot]

Done in this commit.

[ockham]

Test coverage looks fine, since most code paths are shared with the case where the $page arg isn't set (but instead inferred from the cpage query arg), and those have some solid coverage already.

I just answered here, but I am not sure if this will cause an error. From what I understood, the extra argument passed to those functions will just be ignored, but it won't throw an error.

Ah yeah, looks like you're right; I replied there.

If that's the case, I believe we could still manage everything in the same merge: [...]

Alright, we can do that. I left one more minor note; otherwise this PR is looking good, code-wise. I'll give it some more smoke testing for good measure.

[ockham]

Test coverage looks fine, since most code paths are shared with the case where the $page arg isn't set (but instead inferred from the cpage query arg), and those have some solid coverage already.

Ah, one more request -- we should probably add one test for each get_next_comments_link and get_prev_comments_link where we both pass the $page arg and set the cpage query var (but to different values) and assert that the functions respect the value of $page and ignore the cpage query var 😊

[SantosGuillamot]

Maybe it even makes sense to include it in the same test?

[SantosGuillamot]

I've pushed this commit that I believe should suffice for that use case: f3d0fc3

[ockham]

Yep, looks good! Thank you 👍

[SantosGuillamot]

The relevant Gutenberg pull request, that pass the $page argument in core blocks, has just been merged. As mentioned in this comment, I believe once those changes are synced, this should be ready.

[SantosGuillamot]

Now that the packages sync has been committed, I've rebased this branch, and it should be ready. In my testing, the old bug is not there anymore and we are removing the conflicting set_query_var.

[gziolo]

@desrosj, is it possible that the props bot doesn't pick up edits to the PR's description when the link to Trac gets added after the comment was created? See this:

Let's see if the props-bot label helps here.

[gziolo]

Still, no luck. No changes in #7275 (comment). There should be listed the author of the ticket that helped in the process.

[sybrew]

I'll just leave this comment here; may that resolve the props-bot issue 🕺

[gziolo]

@sybrew, that helped. I listed you in props manually, anyway. Major props for all your help to identify and fix the issue😀

Changed committed with https://core.trac.wordpress.org/changeset/59081.