Make WordPress Core

Opened 4 weeks ago

Last modified 4 weeks ago

#60806 new defect (bug)

Remove query alteration from build_comment_query_vars_from_block

Reported by: cybr's profile Cybr Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version: 6.0
Component: General Keywords: has-patch
Focuses: Cc:

Description

In the function build_comment_query_vars_from_block(), set_query_var( 'cpage', ... ) is called. This alters the query in unexpected ways.

The function is called via blocks core/comment-template, core/comments-pagination-next, and core/comments-pagination-numbers.

None of these blocks rely on cpage internally or indirectly. Adjusting cpage was probably vestigial of the initial development of the comment blocks.

This bug causes the canonical URL to be incorrect for singular posts (when not using an SEO plugin). As a result, all these sites' singular content will be misindexed or, at worst, deindexed. Some 200 plugins are also affected by this issue.

This only happens when comment pagination is turned on.

This bug was only discovered recently because blocks had to be parsed before wp_head() for it to become a canonical URL issue, which is happening because of FSE and other custom block implementations.

Discovery: https://wordpress.org/support/topic/the-seo-framework-adds-noindex-to-posts-with-multiple-comment-pages-%e2%9a%a0%ef%b8%8f/
Slack thread: https://wordpress.slack.com/archives/C02RQBWTW/p1710771272739539?thread_ts=1710709493.297079&cid=C02RQBWTW

I was requested to resolve this upstream via Gutenberg, but this is a more pertinent issue with Core. build_comment_query_vars_from_block() is also not part of Gutenberg but was created during its merger with WP 6.0.

I propose to remove using set_query_var() altogether in the method. No functionality changes are apparent, but it resolves all aforementioned issues. PR will follow soon.

Change History (6)

This ticket was mentioned in PR #6291 on WordPress/wordpress-develop by @Cybr.


4 weeks ago
#1

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core by cybr. View the logs.


4 weeks ago

@Cybr commented on PR #6291:


4 weeks ago
#3

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.

@Cybr commented on PR #6291:


4 weeks ago
#4

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

#5 @Cybr
4 weeks ago

Adding a unit test to assert the canonical URL to ensure it won't be mangled would be a great addition. As a bonus, I think that would also reveal issues with the Query Block.

#6 @Cybr
4 weeks ago

The use of set_query_var() should also be reconsidered in comments_template() and wp_list_comments()—these functions also adjust the entire query only to serve their specific needs.

With that, Core would rid itself of such patches.

However, it will become nigh impossible to do so in comments_template() since that will break implementations via filter comments_template or the included custom theme files. wp_list_comments() will only set it after comments_template(), because of globals $overridden_cpage.

I think maintaining focus on the block issue would be best since all FSE sites are affected by this.

Note: See TracTickets for help on using tickets.