Remove query alteration from build_comment_query_vars_from_block()

[sybrew]

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 cybr, santosguillamot, bernhard-reiter, gziolo.

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.

[sybrew]

You can test this by adding the comment block to a singular page using an FSE "block" theme and turning on comment pagination.
Inspect the canonical URL and test whether pagination still works.

[sybrew]

Test test_build_comment_query_vars_from_block_sets_cpage_var() ought to be deleted, for it no longer serves a purpose.

[SantosGuillamot]

Thanks for the report and I'm sorry for introducing the problem in the first place. As shared below, this code was introduced in this pull request solving another bug, and we assumed it made sense to update cpage because it is actually the comments page.

More context can be found in this comment, but basically the bug was that when visiting a post with comments ordered from newest to oldest, the pagination links weren't showing as expected:

Before (this is wrong)

After (this is right)

We might want to take a look at different alternatives rather than just removing that code.

[SantosGuillamot]

I'm afraid removing this without other solutions will cause another bug. This code was introduced in this pull request for a reason. More context can be found there and in this comment that code was suggested. We might want to revisit other potential solutions to that bug. There are a few suggestions on that discussion, although it is a bit old.

[sybrew]

That might be because of an off-by-one error. Patching the query should not be considered a fix for the original error.

[SantosGuillamot]

As mentioned in this comment, I would probably fix the conditional to follow old comments approach instead of removing it. According to my tests, something like this should work:

if ( '' == get_query_var( 'cpage' ) && isset( $comment_args['paged'] ) && $comment_args['paged'] > 1 ) {
	set_query_var( 'cpage', $comment_args['paged'] );
}

[sybrew]

I recommend removing that code as proposed in this PR and fixing the cause that deemed the code necessary.

[ockham]

I recommend removing that code as proposed in this PR and fixing the cause that deemed the code necessary.

I wouldn't presently merge this PR as it would cause a regression of comments pagination, as @SantosGuillamot mentioned, so we'd be trading one bug for another 😕 (We have e2e test coverage for the issue that was originally fixed by WordPress/gutenberg#39227, and I've verified that this PR would indeed break one of the tests. To reproduce, apply the diff locally and run npm run test:e2e -- editor/blocks/comments.spec.js from within the Gutenberg repo.)

I had a bit of a deeper look into comments pagination related functions. FWIW, the result of get_previous_comments_link is determined by the value of the cpage query var, as is get_next_comments_link. Both functions are widely used by Classic Themes.

That might be because of an off-by-one error. Patching the query should not be considered a fix for the original error.

However, that is what the (equally "classic") comments_template() function does as well:

wordpress-develop/src/wp-includes/comment-template.php

Lines 1596 to 1600 in dea027e

	if ( '' == get_query_var( 'cpage' ) && $wp_query->max_num_comment_pages > 1 ) {
	set_query_var( 'cpage', 'newest' === get_option( 'default_comments_page' ) ? get_comment_pages_count() : 1 );
	$overridden_cpage = true;
	}

My impression is that what we're doing in the Comments Pagination blocks is largely following established WP Core practices, so we should probably continue to set the cpage query var (as it does seem to be the "correct" way that get_previous_comments_link and friends are controlled). There's probably a bug somewhere in the logic that we use to set the value of cpage (and/or maybe one of the other $comment_args?) that we'll need to fix.

---

Maybe we can add an e2e test to cover the bug that this PR seeks to fix; that would make it easier to develop a solution that fixes both this issue without introducing a regression of WordPress/gutenberg#39227.

[SantosGuillamot]

I've been running some tests and it seems cpage was behaving the same way even before introducing that new block in 6.0 while using comment_template: link.

According to my tests, when I set the option to “show last page displayed by default”, cpage is always a value greater than 1 when there is pagination. It takes the latest page. These are the use cases I tested:

• Block theme in trunk using the new comments block.
• Block theme in trunk using the legacy comments block, which uses comments_template.
• Classic theme in trunk.
• Classic theme in 5.9 branch.

I've been using the Query Monitor plugin to check the value of cpage. If I check it using get_query_var( 'cpage' ) in an init or wp_head filter, it is still an empty string at that point.

For this reason, I believe this would require a deeper discussion about what is the expected value of cpage with the mentioned settings.

[ockham]

@SantosGuillamot Thank you for your investigation!

@sybrew It seems like we aren't able to find any discernible differences of how cpage is set by Block Themes vs Classic Themes. Could you let us know which URLs are specifically broken when using Block Themes (e.g. which <link> tags in the <head>, which <a>s in the <body>, etc.), and what Discussion settings will cause them to break? Knowing what to look out for -- and to compare to the Classic Themes case -- will help us resolve this issue.

[ockham]

@DAreRodz and I dug a bit deeper into this issue today. With the TSF plugin installed and active, it was quite easy to reproduce that there's a difference between Classic Themes and Block Themes. We tried once again with the following settings:

This resulted in the following markup for a post with 31 comments (first page):

With TT1:

<!doctype html>
<html lang="en-US" >
<head>
	<meta charset="UTF-8" />
	<meta name="viewport" content="width=device-width, initial-scale=1" />
	<title>Hello world! &#x2d; gutenberg</title>

<!-- The SEO Framework by Sybre Waaijer -->
<meta name="robots" content="max-snippet:-1,max-image-preview:large,max-video-preview:-1" />
<link rel="canonical" href="http://localhost:8888/?p=1" />

With TT4:

<!DOCTYPE html>
<html lang="en-US">
<head>
	<meta charset="UTF-8" />
	<meta name="viewport" content="width=device-width, initial-scale=1" />

<!-- The SEO Framework by Sybre Waaijer -->
<meta name="robots" content="noindex,max-snippet:-1,max-image-preview:large,max-video-preview:-1" />
<meta name="description" content="Welcome to WordPress. This is your first post. Edit or delete it, then start writing!" />

Note the missing canonical <link>, and the (undesired) noindex in the first <meta> tag with TT4.

[sybrew]

Howdy! Sorry for my absence.

To reproduce the issue consistently:

1. WordPress 6.6.1
2. Theme: Twenty Twenty-Four
3. Plugins: None.
4. (optional) "Settings > Permalinks": Select "Post name" and save.
5. Create a Post. For that Post, set a title, add a paragraph with some text, and add a comment block (the HTML of this is below).

The block HTML (click me to view)

<!-- wp:paragraph -->
<p>Testing...</p>
<!-- /wp:paragraph -->

<!-- wp:comments -->
<div class="wp-block-comments"><!-- wp:comments-title /-->

<!-- wp:comment-template -->
<!-- wp:columns -->
<div class="wp-block-columns"><!-- wp:column {"width":"40px"} -->
<div class="wp-block-column" style="flex-basis:40px"><!-- wp:avatar {"size":40,"style":{"border":{"radius":"20px"}}} /--></div>
<!-- /wp:column -->

<!-- wp:column -->
<div class="wp-block-column"><!-- wp:comment-author-name {"fontSize":"small"} /-->

<!-- wp:group {"style":{"spacing":{"margin":{"top":"0px","bottom":"0px"}}},"layout":{"type":"flex"}} -->
<div class="wp-block-group" style="margin-top:0px;margin-bottom:0px"><!-- wp:comment-date {"fontSize":"small"} /-->

<!-- wp:comment-edit-link {"fontSize":"small"} /--></div>
<!-- /wp:group -->

<!-- wp:comment-content /-->

<!-- wp:comment-reply-link {"fontSize":"small"} /--></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->
<!-- /wp:comment-template -->

<!-- wp:comments-pagination -->
<!-- wp:comments-pagination-previous /-->

<!-- wp:comments-pagination-numbers /-->

<!-- wp:comments-pagination-next /-->
<!-- /wp:comments-pagination -->

<!-- wp:post-comments-form /--></div>
<!-- /wp:comments -->

Important: Add one comment to the post.

Reproducing the issue, 1:

1. Go to "Settings > Discussion".
   • Check "Break comments into pages with ANY top level comments per page and the FIRST page displayed by default.
2. View the source of the post with the comment. Search for canonical.
   • Notice how the Canonical URL has appended #comments. This hinders indexing for the pages affected.

Reproducing the issue, 2:

1. Go to "Settings > Discussion".
   • Check "Break comments into pages with ANY top level comments per page and the LAST page displayed by default.
2. View the source of the post with the comment. Search for canonical.
   Notice how the Canonical URL has appended /comment-page-1/#comments. This can block the page's indexing and harm its ranking.

Takeaway: Do not use set_query_var() after the main query is set up. Even if this was used in the past without issue; it was by accident. This change in Gutenberg now exposed the issue of modifying the main query.

Aside: @ockham's comment above mentions The SEO Framework (my plugin). It automatically sets "noindex" for paginated comments and removes the canonical URL to speed up deindexing unless the canonical URL is different from the expected request. This is intended behavior. The SEO Framework does not suffer from the bug this PR attempts to tackle because I implemented a patch as soon as I discovered the bug.

[SantosGuillamot]

Thanks a lot for the reproduction steps, those are really useful 🙂

I've been triaging it again, and I believe all the issues mentioned in this thread can be solved by changing this conditional to check $comment_args['paged'] > 1 instead of > 0. It follows the same logic as the old comments implementation, which seems to be the appropriate approach: link.

These are the things I reproduced:

Canonical URL appends #comments

As reported by @sybrew here, the current implementation is adding #comments in the canonical URL.

With the suggested fix, the #comments is not added anymore and it works as expected.

"Older"/"Newer" comments links keep working as expected

The modified code was introduced to solve an issue with the Older/Newer comments links here.

With the suggested fix, it keeps working as expected, while the rest of the issues are solved.

Missing canonical URL

As reported by @ockham here, there wasn't a canonical tag caused by this logic.

With the suggested fix, the canonical link is added as expected.

---

With that in mind, I believe we should adapt the conditional to follow the old approach instead of removing it.

@sybrew, Could you please confirm if that solves the issues you were facing and if everything keeps working as expected on your side?

[sybrew]

The main query should not be adjusted after it has been established. Using set_query_var() is an anti pattern, something that should be reserved for niche plugin authors only.

I recommend removing its usage and adjusting the off-by-one logic where necessary. The pagination query variables will flow from the requested URL and should not be tinkered with afterwards.

[SantosGuillamot]

It seems the logic to set_query_var in that specific use case has been there for 16 years (since 2008): link. I would personally feel more comfortable replicating how the old comments system has always worked. There could be other plugins relying on cpage being the updated value because it has been like that for a long time. I'd love to know more opinions though.

[sybrew]

Correct. This issue was introduced back in 2008 via a patch, but its implications weren't apparent because set_query_var() only runs the moment the comment fields are being outputted, which almost always happens near the end of the page, where most links have already been rendered.

With FSE, all the page's blocks, including the comment blocks, are parsed right before action wp_head (after wp). Because of this, when it's time to render the page, the main query is already affected, so all links on the page are changed.

Dump of actions until wp_head in the test scenario described in my earlier comment (click me to view)

The comment: #6291 (comment)

mu_plugin_loaded
muplugins_loaded
registered_taxonomy
registered_taxonomy_category
registered_taxonomy_post_tag
registered_taxonomy_nav_menu
registered_taxonomy_link_category
registered_taxonomy_post_format
registered_taxonomy_wp_theme
registered_taxonomy_wp_template_part_area
registered_taxonomy_wp_pattern_category
registered_post_type
registered_post_type_post
registered_post_type_page
registered_post_type_attachment
registered_post_type_revision
registered_post_type_nav_menu_item
registered_post_type_custom_css
registered_post_type_customize_changeset
registered_post_type_oembed_cache
registered_post_type_user_request
registered_post_type_wp_block
registered_post_type_wp_template
registered_post_type_wp_template_part
registered_post_type_wp_global_styles
registered_post_type_wp_navigation
registered_post_type_wp_font_family
registered_post_type_wp_font_face
plugin_loaded
plugins_loaded
load_textdomain
sanitize_comment_cookies
wp_roles_init
setup_theme
unload_textdomain
after_setup_theme
auth_cookie_malformed
auth_cookie_valid
set_current_user
init
widgets_init
wp_register_sidebar_widget
wp_default_styles
wp_sitemaps_init
registered_taxonomy_faq_cat
registered_taxonomy_faq_tag
registered_post_type_faq
wp_loaded
wp_cache_set_last_changed
parse_request
parse_query
pre_get_posts
posts_selection
parse_term_query
pre_get_terms
metadata_lazyloader_queued_objects
send_headers
wp
template_redirect
wp_default_scripts
admin_bar_init
add_admin_bar_menus
parse_tax_query
loop_start
the_post
loop_no_results
render_block_core_template_part_file
parse_comment_query
pre_get_comments
comment_form_before
comment_form_top
comment_form_logged_in_after
comment_form
comment_form_after
loop_end
wp_head

In short, the old way was also wrong, and this mistake should not be repeated for the appeal of tradition. I already mentioned this in the ticket (comment:6) and its undesired implications because of existing filters (i.e., comments_template), among other things.

I believe the inconsistency following this patch in the comment block is acceptable. Especially since the Block Editor already largely deviates from the Classic Editor and is unaffected by the same filters, it's worth considering improving upon things with 16 years of gained knowledge.

[gziolo]

Thank you @sybrew for all the detailed insights. It’s super helpful to work on the proper fix. In particular all the nuances between the classic and block themes are extremely important here. Excellent work pointing it out so precisely which is crucial to make the good final decision.

I have an idea to consider of how to apply the fix proposed but at the same time mitigate the impact explained by @sybrew. We could clone the global $wp_query before calling these functions that depend on it and restore it to the previous state afterward. A good example in the codebase for REST API preload in the editor:
https://github.com/WordPress/wordpress-develop/blob/9ed3553da6a61a38a1d4c5585e5ac3c9d13d351a/src/wp-includes/block-editor.php#L743L776

I saw the same pattern in other places.

[ockham]

With FSE, all the page's blocks, including the comment blocks, are parsed right before action wp_head (after wp). Because of this, when it's time to render the page, the main query is already affected, so all links on the page are changed.

To corroborate this, here's an inconsistency in individual comments' permalinks that @DAreRodz and I found:

[SantosGuillamot]

If we get rid of the set_query_var I feel we need to solve the mentioned issue it was solving.

As suggested by @ockham , I believe it could be solved passing a new argument to get_previous_comments_link and get_next_comments_link functions.

I started a draft pull request to modify the comments block and pass it as an argument. It would need modifications in core functions receive $page as an argument and only run get_query_var if $page doesn't exist:

In get_previous_comments_link and get_next_comments_link, wrap that line with something like this:

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

Let me know what you think. I can help pushing that forward.

[SantosGuillamot]

I've created one pull request in core with these changes plus the ones needed to avoid causing the old bug: #7275.

With that, along the relevant Gutenberg PR to modify the blocks, it should be possible to remove the set_query_var while keeping the pagination links working as expected.

[gziolo]

Landed the alternative patch with https://core.trac.wordpress.org/changeset/59081. Thank you for all the help.