Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 months ago

#55658 closed defect (bug) (fixed)

Backport a bugfix of Comment Template block pagination

Reported by: czapla's profile czapla Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.0 Priority: normal
Severity: normal Version: 6.0
Component: Comments Keywords: has-patch has-unit-tests commit
Focuses: Cc:


Setting paged to 1 (instead of 0) if there are no comments will cause them to fail.

More details in in

This is a backport of the fix for that issue.

Change History (18)

michalczaplinski commented on PR #2663:

2 years ago

I'm not quite sure why PHP lint is failing. Locally, running composer run-script lint throws up an error like:

, Object(PHP_CodeSniffer\Ruleset), Object(PHP_CodeSniffer\Config))
#5 /Users/czapla/Projects/wordpress-develop/vendor/squizlabs/php_codesniffer/src/Runner.php(542): PHP_CodeSniffer\Runner->processChildProcs(Array)
#6 /Users/czapla/Projects/wordpress-develop/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run()
#7 /Users/czapla/Projects/wordpress-develop/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS()
#8 {main}
  thrown in /Users/czapla/Projects/wordpress-develop/vendor/squizlabs/php_codesniffer/src/Runner.php on line 606
Script @php ./vendor/squizlabs/php_codesniffer/bin/phpcs --report=summary,source handling the lint event returned with error code 255

Unfortunately, there is no information in the CI on where the mis-formatting is happening.

ockham commented on PR #2663:

2 years ago

Ah, indeed! Looks like the GB PR actually also had that (but I didn't notice it since it's covered by the GHA workflow for PHP Unit tests, and I only saw the unit test error there):

ockham commented on PR #2663:

2 years ago

Ooh, one of the errors is actually visible in _this_ PR's inline diff:

Love those annotations 😄

ockham commented on PR #2663:

2 years ago

Here's a diff with the PHP lint fixes on top of this PR (since I can't push to @michalczaplinski's GH repo):

commit 4db5e1840a42c4ecc3bd4e07a5f50f2555d92496
Author: Bernie Reiter <ockham@…>
Date: Tue May 3 10:22:55 2022 +0200

PHP lints

diff --git a/src/wp-includes/blocks.php b/src/wp-includes/blocks.php
index ff15b9568e..a9a7535530 100644
--- a/src/wp-includes/blocks.php
+++ b/src/wp-includes/blocks.php
@@ -1325,8 +1325,8 @@ function build_comment_query_vars_from_block( $block ) {

} elseif ( 'oldest' === $default_page ) {

$comment_argspaged? = 1;

} elseif ( 'newest' === $default_page ) {

  • $max_num_pages = (int) ( new WP_Comment_Query( $comment_args ) )->max_num_pages;
  • $comment_argspaged? = $max_num_pages === 0 ? 1 : $max_num_pages;

+ $max_num_pages = (int) ( new WP_Comment_Query( $comment_args ) )->max_num_pages;
+ $comment_argspaged? = 0 === $max_num_pages ? 1 : $max_num_pages;

Set the cpage query var to ensure the previous and next pagination links are correct
when inheriting the Discussion Settings.


hellofromtonya commented on PR #2663:

2 years ago

Using the testing instructions here, I was able to reproduce the SQL syntax error. Applying the fix in this patch resolves the issue.

What is this patch doing? When there are no comments, it sets the 'paged' arg in the comments query to the default value of 1.

Should it be setting the default value? Or should it not set 'paged' arg and then let Core natively handle setting the default value. Setting here means that if the default value changes someday, it'll need to be changed in multiple places. cc @dream-encode what do you think?

This fix is needed for 6.0 as the issue was introduced in 6.0's cycle and its severity is severe. The patch works and can in as-is to fix the issue. The question about whether to set a default value or not set 'paged' arg can be handled separately if more time is needed for discussion and consensus.

#7 @hellofromTonya
2 years ago

  • Component changed from General to Comments
  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.0

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

2 years ago

dream-encode commented on PR #2663:

2 years ago

@hellofromtonya Looking at the core docs, I think letting Core handle the default is probably safer, as long as no other bugs are introduced. Seems a bit risky to be changing the paged param in other places this late in the cycle. I'd rather use the default, then investigate possibly setting paged in 6.0.1, if it's even needed.

hellofromtonya commented on PR #2663:

2 years ago

I think that's a safer path @dream-encode. Thank you! I'll make the adjustment and then prep the commit.

hellofromtonya commented on PR #2663:

2 years ago

And of course, I need to also fix the tests with this PR 🤦‍♀️

#12 @hellofromTonya
2 years ago

  • Owner set to hellofromTonya
  • Resolution set to fixed
  • Status changed from new to closed

In 53336:

Editor: Sets 'paged' query arg only when there are comments: build_comment_query_vars_from_block().

A SQL syntax error happened when a post has no comments and "Break comments into pages" is checked in Settings > Discussion. The fix sets the 'paged' query arg only when there are comments. When there are no comments, WP_Comment_Query sets the default 'paged' value to 1.

Props bernhard-reiter, luisherranz, czapla, cbravobernal, davidbaumwald, hellofromTonya.

Follow-up to [53142], [53138].
Fixes #55658.

michalczaplinski commented on PR #2663:

2 years ago

Use Yoda condition checks, you must.
Love those annotations 😄

Admittedly, I didn't realize that this was an actual warning because of how it's worded. I thought this was some inside joke 😅 . TIL Yoda conditions are a thing.

hellofromtonya commented on PR #2663:

2 years ago

@michalczaplinski yeah they are a real thing. And here's where they live in Core's handbook:

if ( true === $the_force ) {

$victorious = you_will( $be );


ockham commented on PR #2663:

2 years ago

Thanks for taking this over the finish line, @hellofromtonya and @dream-encode ! 😄

hellofromtonya commented on PR #2663:

2 years ago

I also resynchronized the changes made in Core back to Gutenberg's compat PR

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

2 months ago

Note: See TracTickets for help on using tickets.