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:

Description

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

More details in in https://github.com/WordPress/gutenberg/issues/40758

This is a backport of the fix for that issue.

Change History (18)

michalczaplinski commented on PR #2663:


2 years ago
#2

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
#3

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):

https://i0.wp.com/user-images.githubusercontent.com/96308/166422759-522fc83c-cf86-4bae-93fc-0dafa27166cb.png

ockham commented on PR #2663:


2 years ago
#4

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

https://i0.wp.com/user-images.githubusercontent.com/96308/166423221-c0f4ade5-e45b-4ca4-a0f6-21d2992cbf6e.png

Love those annotations 😄

ockham commented on PR #2663:


2 years ago
#5

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

{{{diff
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
#6

Using the testing instructions here https://github.com/WordPress/gutenberg/issues/40758, 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
#9

@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
#10

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
#11

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
#13

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
#14

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

{{{php
if ( true === $the_force ) {

$victorious = you_will( $be );

}
}}}

ockham commented on PR #2663:


2 years ago
#16

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

hellofromtonya commented on PR #2663:


2 years ago
#17

I also resynchronized the changes made in Core back to Gutenberg's compat PR https://github.com/WordPress/gutenberg/pull/40759.

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


2 months ago

Note: See TracTickets for help on using tickets.