Merge Comments and Post Comments blocks #41807

DAreRodz · 2022-06-20T08:38:55Z

Tracking issue: #41451

What?

Merge the Comments block into the Post Comments block. This PR is WIP and continues the work in #40522.

The idea is to end up with a single block comprising both blocks:

Comments as the default behavior
Post Comments as a fallback

More info in #40521.

We could also take this opportunity to change the Comments block ID from core/comments-query-loop to core/comments and end up with something like this:

Why?

See #40522 (comment)

How?

The block differentiates between the "legacy" PHP mode and the editable mode using an attribute called legacy. That property is false by default, and it's only set to true when converting the old Post Comments into the Comments block. The block conversion is automatically done when you open the WP editor.

The block is either static or dynamic depending on the value of legacy (false or true, respectively).

Testing Instructions

Check out this branch.
With a block theme installed that uses the Post Comment block in its Single template, visit a post with comments.
Ensure that the Post Comments block renders as usual.
Move to the Site Editor and open the Single template.
Check that the block has turned into a Comments block. It should show a placeholder and a warning message to switch the block to the editable version.
Open the code editor and verify that the block is now . Close the code editor.
Save changes (you would need to make minor changes, like reordering blocks).
Revisit the previous post. It should look exactly as before (i.e., the PHP version).
Go to the Site Editor again, and now click the switch button. Now, inner blocks should appear.
Open the code editor. The block should not have the legacy attribute (it is false by default) and should contain rendered markup and inner blocks.
Save and visit the post again. The new version should render.

Other things to test:

Post Comments should not appear in the block inserter.
Adding the Comments block should show the editable version by default.

DAreRodz · 2022-06-20T08:51:32Z

I thought the following code was working, but it turned out that I hadn't tested all the different possibilities.

gutenberg/packages/block-library/src/comments/save.js

Lines 9 to 19 in f35d33b

    
           const innerBlocksProps = useInnerBlocksProps.save( blockProps ); 
        
           // The `save` function always returns a RawHTML element wrapping the 
        
           // serialized content. We check whether the serialized content is empty or 
        
           // not. 
        
           const rawHTML = Children.only( innerBlocksProps.children ); 
        
           if ( ! rawHTML.props.children ) { 
        
           	return null; 
        
           } 
        
           return <Tag { ...innerBlocksProps } />;

In line 15, rawHTML.props.children always return an empty string, whatever the inner blocks. It looks like the process that parses the block ignores its children, so this is not a valid approach to knowing whether the block has inner blocks or not. ☹️

packages/block-library/src/comments/index.php

spacedmonkey · 2022-06-20T10:02:09Z

packages/block-library/src/comments/index.php

+	$output             = ob_get_clean();
+
+	wp_enqueue_script( 'comment-reply' );
+


Suggested change

wp_reset_postdata();

ockham · 2022-06-20T10:54:24Z

I thought the following code was working, but it turned out that I hadn't tested all the different possibilities.

gutenberg/packages/block-library/src/comments/save.js

Lines 9 to 19 in f35d33b

const innerBlocksProps = useInnerBlocksProps.save( blockProps );

// The `save` function always returns a RawHTML element wrapping the

// serialized content. We check whether the serialized content is empty or

// not.

const rawHTML = Children.only( innerBlocksProps.children );

if ( ! rawHTML.props.children ) {

return null;

}

return <Tag { ...innerBlocksProps } />;

In line 15, rawHTML.props.children always return an empty string, whatever the inner blocks. It looks like the process that parses the block ignores its children, so this is not a valid approach to knowing whether the block has inner blocks or not. ☹️

Sorry to hear -- I remember that it was one of the major problems with #40522 that we never found a straight-forward and reliable way to infer if the block has existing children or not.

I guess it's time we try a different strategy. At the time of conversion from post-comments or comments-query-loop to comments, we fortunately know which kind of block we're dealing with, so maybe our best option is to preserve that information as a block attribute after all. I.e.

<!-- wp:post-comments / -->    ➡️    <!-- wp:comments legacy="true" / -->

<!-- wp:comments-query-loop -->     <!-- wp:comments -->
    <!-- InnerBlocks / -->       ➡️      <!-- InnerBlocks / -->
<!-- /wp:comments-query-loop -->    <!-- /wp:comments -->

ockham · 2022-06-20T13:27:27Z

As discussed in Slack with @DAreRodz and @c4rl0sbr4v0, it might make sense to decouple the block merge from the ID change (to core/comments), for which we already have a PR (#40506).

I think we can continue working on both of them independently; we'll just need to merge #40506 before this one (and then rebase this one to pick up the changed IDs).

ockham · 2022-06-20T14:46:12Z

Another thing that @DAreRodz and I discussed on our video call earlier today is ensuring backwards compatibility.
Specifically, while we plan to add logic that will change core/post-comments and core/comments-query-loop blocks to core/comments in the editor, this will only affect those blocks when edited.

The question thus remains what happens if we have existing post content with those old blocks that haven't been edited.
We believe that:

Since core/comments-query-loop has always been static, the serialized blocks should continue to work on the frontend (since it's just HTML, after all).
For core/post-comments (which is dynamic), we can hopefully follow the precedent set by the PR that transformed core/query-loop into core/post-template. (This should allow us to drop the actual post-comments block folder and have it taken care of by the core/comments code.)

ockham · 2022-06-20T18:34:48Z

packages/block-library/src/comments/index.js

+	variations: [
+		{
+			name: 'default',
+			isDefault: true,
+			innerBlocks: TEMPLATE,
+			scope: [ 'inserter' ],
+		},
+	],


Do we still need the variations? I think we tried this approach to work around the issue with detecting inner blocks, but it looks like we'd be avoiding this issue now anyway if we go with the legacy attribute as discussed during our call 🙂

Good point @ockham. I'll remove the variations attribute. 👍

luisherranz · 2022-06-21T08:35:23Z

In my opinion, whether a block contains inner blocks or not (during save) is a perfectly valid use case because a block may want to output different markup depending on that information.

So even if !!Children.only( useInnerBlockProps.save().children ).props.children would have worked, it should have been a temporary workaround, but that is not a good final API.

My suggestion is to investigate how to add this information to the Block API in a way that:

It works for both edit, save (and view/frontend if it's added in the future) in the same way, so code can be reused between those environments.
A similar API can also be exposed for dynamic blocks.
It is simple and intuitive, hopefully reusing similar concepts/APIs as the existing ones.
It can be augmented in the future (i.e., adding more information about the inner blocks, like their names or attributes).

If you do so, please add it to the Block API tracking issue #41236.

DAreRodz · 2022-06-21T09:21:11Z

So even if !!Children.only( useInnerBlockProps.save().children ).props.children would have worked, it should have been a temporary workaround, but that is not a good final API.

Totally agree; that's even coupled with the useInnerBlockProps.save() implementation, so we would need to change this code the day .save() returns something different.

DAreRodz · 2022-06-21T09:29:46Z

@spacedmonkey, sorry for the ping! I should have created this PR as a draft from the beginning. 😅 All the code you reviewed, I copy-pasted it from /packages/block-library/src/post-comments/index.php, which is basically the behavior we want to include as a fallback after merging the blocks.

I'll have in mind your suggestions, though. 😄

spacedmonkey · 2022-06-21T09:35:25Z

which is basically the behavior we want to include as a fallback after merging the blocks.

Just because the code is copied from elsewhere, doesn't mean it should be just accepted into this new block. As the old block does seem to have unit tests, it hard to valid how it meant to work. I have 9 years of experience working on WordPress core and I am a core committer. Hopefully some of feedback is actioned.

DAreRodz · 2022-06-21T11:19:56Z

@ockham, I'm not sure how to handle styles. Do we need to keep the old class name? Can we include styles only if the block version is the legacy one? 🤔

ockham · 2022-06-21T11:58:49Z

@ockham, I'm not sure how to handle styles. Do we need to keep the old class name? Can we include styles only if the block version is the legacy one? 🤔

Good question! In cases like these, I like to consult #32514 for prior art 😄

I found this, so I think we should keep the old class name.

(But yeah, in our case, we'd only need to keep the class name around if legacy is true.)

DAreRodz · 2022-06-22T17:31:28Z

I have another question regarding styles, though. I'm not sure if this is feasible, but it would be ideal if the old CSS code for wp-block-post-comments is not included when the Comments block doesn't have the legacy attribute.

Is there a way to not include wp-block-post-comments in the block.json's style prop and request that stylesheet only when the block uses the legacy version, during the block rendering (in PHP)?

ntsekouras · 2022-06-23T10:54:06Z

Is there a way to not include wp-block-post-comments in the block.json's style prop and request that stylesheet only when the block uses the legacy version, during the block rendering (in PHP)?

It seems there is not a way for that in _wp_multiple_block_styles and I'm not sure if we can use another filter for that.. --cc @aristath

I haven't checked the code in this PR but maybe an alternative could be to transform the legacy styles to css-in-js so they will be injected conditionally. --cc @ciampo

aristath · 2022-06-23T11:00:59Z

I don't think it's possible to do it in the editor... However, on the frontend I think it should be possible to do it 🤔
The _wp_multiple_block_styles function is hooked in block_type_metadata, so what we can do is remove it from block.json, and then conditionally add it in the block_type_metadata hook if needed.

adamziel · 2022-06-23T15:31:17Z

This is a significant change for a minor point release so I went ahead and removed this issue from the 6.0.1 editor tasks board and also removed the backport label @ockham. LMK if you disagree!

ockham · 2022-06-23T16:23:28Z

This is a significant change for a minor point release so I went ahead and removed this issue from the 6.0.1 editor tasks board and also removed the backport label @ockham. LMK if you disagree!

Yeah, it's kind of a big change 🤔 I think my main rationale for including it in 6.0.1 would be that the Comments block is still fairly fresh -- WP 6.0 was the first release that had it included, and that deprecated the Post Comments block. In a way, merging those two blocks is the logical continuation of that deprecation. We'd discussed it previously with @gziolo, who seemed in favor of including it in 6.0.1.

But yeah, I get your concerns; it wouldn't hurt to have a bit more time to test this... 🤔

packages/block-library/src/comments/index.php

lib/blocks.php

packages/block-library/src/comments/index.php

test/e2e/specs/editor/blocks/comments.spec.js

Co-authored-by: Bernie Reiter <ockham@raz.or.at>

DAreRodz · 2022-07-22T11:44:43Z

I saw some unrelated e2e tests that seem to fail randomly. 😅 They didn't pass two commits ago, and now they did.

ockham

Thanks a lot for all your work on this @DAreRodz, and thanks for everyone who weighed in to makes this happen! 🙌

LGTM 😎

DAreRodz · 2022-07-22T17:47:14Z

I'm going to merge this PR, so it doesn't keep growing. 😅 🚀

As a follow-up, we can go deeper into @spacedmonkey suggestion of using wp_reset_postdata(). I've been reviewing that, and AFAIK everything works without that function because:

comments_template() only works on 'single' pages (see this).
postId value from context usually will match the current global $post variable, so nothing changes after running setup_postdata( $post_id ) (see this).

Another thing I've noticed is that the block renders in those cases where thecomments_template()'s output is empty. This should be a quick fix. 🙂

diff --git a/packages/block-library/src/comments/index.php b/packages/block-library/src/comments/index.php
index 5df2ed550a..ef8c98e391 100644
--- a/packages/block-library/src/comments/index.php
+++ b/packages/block-library/src/comments/index.php
@@ -62,6 +62,10 @@ function render_block_core_comments( $attributes, $content, $block ) {
 	$output = ob_get_clean();
 	$post   = $post_before;
 
+	if ( empty( $output ) ) {
+		return '';
+	}
+
 	$classnames = array();
 	// Adds the old class name for styles' backwards compatibility.
 	if ( isset( $attributes['legacy'] ) ) {

ockham · 2022-07-28T13:23:58Z

Thanks @DAreRodz!

Your reasoning about wp_reset_postdata() makes sense to me -- we can leave things as they are (unless we're missing something cc/ @spacedmonkey).

Another thing I've noticed is that the block renders in those cases where the comments_template()'s output is empty. This should be a quick fix. 🙂

I've filed #42737 for this, but was struggling to reproduce in what cases comments_template()'s output would be empty. (I tried a post with no comments and disabled comments for it to also hide the comments form, but we already have code in trunk to bail early in that case.) Maybe you can help me fill in the testing instructions there? 🙌

DAreRodz · 2022-08-01T10:28:45Z

EDIT: moved my answer here.

Per WordPress/gutenberg#41807

DAreRodz requested review from spacedmonkey and michalczaplinski as code owners June 20, 2022 08:38

DAreRodz marked this pull request as draft June 20, 2022 08:39

spacedmonkey reviewed Jun 20, 2022

View reviewed changes

packages/block-library/src/comments/index.php Show resolved Hide resolved

spacedmonkey reviewed Jun 20, 2022

View reviewed changes

packages/block-library/src/comments/index.php Show resolved Hide resolved

spacedmonkey reviewed Jun 20, 2022

View reviewed changes

ockham mentioned this pull request Jun 20, 2022

Comments block: Change ID to core/comments #40506

Merged

ockham added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jun 20, 2022

ockham mentioned this pull request Jun 20, 2022

Comments blocks backports and fixes: Tracking issue #41451

Closed

10 tasks

ockham reviewed Jun 20, 2022

View reviewed changes

adamziel removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jun 23, 2022

ntsekouras reviewed Jun 28, 2022

View reviewed changes

packages/block-library/src/comments/index.php Outdated Show resolved Hide resolved

DAreRodz force-pushed the merge/comments-into-post-comments branch from e2c5688 to a79556e Compare July 7, 2022 13:16

ockham reviewed Jul 7, 2022

View reviewed changes

lib/blocks.php Outdated Show resolved Hide resolved

ockham reviewed Jul 7, 2022

View reviewed changes

packages/block-library/src/comments/index.php Outdated Show resolved Hide resolved

ockham reviewed Jul 21, 2022

View reviewed changes

test/e2e/specs/editor/blocks/comments.spec.js Outdated Show resolved Hide resolved

ockham reviewed Jul 21, 2022

View reviewed changes

test/e2e/specs/editor/blocks/comments.spec.js Outdated Show resolved Hide resolved

DAreRodz and others added 7 commits July 21, 2022 23:22

Move comment about setOptions inside forEach

81cfd7d

Make setOptions util stateless

cf790fd

Remove legacy: false

2007766

Co-authored-by: Bernie Reiter <ockham@raz.or.at>

Clarify "block version"

3a23e9f

Co-authored-by: Bernie Reiter <ockham@raz.or.at>

Replace unnecessary backticks with single quotes

a93d0c7

Co-authored-by: Bernie Reiter <ockham@raz.or.at>

Use getEditedPostContent

bef407c

Slightly improve some comments

85ec8d3

ockham approved these changes Jul 22, 2022

View reviewed changes

DAreRodz added the [Block] Comments Affects the Comments Block - formerly known as Comments Query Loop label Jul 22, 2022

DAreRodz marked this pull request as ready for review July 22, 2022 12:26

DAreRodz requested review from ajitbohra, ryanwelcher, juanmaguitar and fabiankaegy as code owners July 22, 2022 12:26

DAreRodz merged commit 5de7f23 into trunk Jul 22, 2022

DAreRodz deleted the merge/comments-into-post-comments branch July 22, 2022 17:48

github-actions bot added this to the Gutenberg 13.8 milestone Jul 22, 2022

This was referenced Jul 26, 2022

Post Comments block: Merge Comments Query Loop block #40521

Closed

Comments Block (legacy): Don't render empty block wrapper #42737

Closed

[Block] Transform from Post Comments block (depricated) -> Comments Query Loop block #40223

Closed

bgardner added a commit to wpengine/frost that referenced this pull request Aug 3, 2022

Update Comments block.

15d18bc

Per WordPress/gutenberg#41807

femkreations added the Needs User Documentation Needs new user documentation label Sep 4, 2022

This was referenced Oct 5, 2022

Lesson - Using the comments block WordPress/Learn#508

Closed

Displaying the Comments Block - Lesson Plan WordPress/Learn#723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge Comments and Post Comments blocks #41807

Merge Comments and Post Comments blocks #41807

DAreRodz commented Jun 20, 2022 •

edited

DAreRodz commented Jun 20, 2022

spacedmonkey Jun 20, 2022

ockham commented Jun 20, 2022 •

edited

ockham commented Jun 20, 2022

ockham commented Jun 20, 2022 •

edited

ockham Jun 20, 2022

DAreRodz Jun 21, 2022

luisherranz commented Jun 21, 2022

DAreRodz commented Jun 21, 2022

DAreRodz commented Jun 21, 2022

spacedmonkey commented Jun 21, 2022

DAreRodz commented Jun 21, 2022

ockham commented Jun 21, 2022

DAreRodz commented Jun 22, 2022

ntsekouras commented Jun 23, 2022

aristath commented Jun 23, 2022

adamziel commented Jun 23, 2022 •

edited

ockham commented Jun 23, 2022

DAreRodz commented Jul 22, 2022

ockham left a comment

DAreRodz commented Jul 22, 2022

ockham commented Jul 28, 2022

DAreRodz commented Aug 1, 2022 •

edited

		$output = ob_get_clean();

		wp_enqueue_script( 'comment-reply' );

Merge Comments and Post Comments blocks #41807

Merge Comments and Post Comments blocks #41807

Conversation

DAreRodz commented Jun 20, 2022 • edited

What?

Why?

How?

Testing Instructions

DAreRodz commented Jun 20, 2022

spacedmonkey Jun 20, 2022

Choose a reason for hiding this comment

ockham commented Jun 20, 2022 • edited

ockham commented Jun 20, 2022

ockham commented Jun 20, 2022 • edited

ockham Jun 20, 2022

Choose a reason for hiding this comment

DAreRodz Jun 21, 2022

Choose a reason for hiding this comment

luisherranz commented Jun 21, 2022

DAreRodz commented Jun 21, 2022

DAreRodz commented Jun 21, 2022

spacedmonkey commented Jun 21, 2022

DAreRodz commented Jun 21, 2022

ockham commented Jun 21, 2022

DAreRodz commented Jun 22, 2022

ntsekouras commented Jun 23, 2022

aristath commented Jun 23, 2022

adamziel commented Jun 23, 2022 • edited

ockham commented Jun 23, 2022

DAreRodz commented Jul 22, 2022

ockham left a comment

Choose a reason for hiding this comment

DAreRodz commented Jul 22, 2022

ockham commented Jul 28, 2022

DAreRodz commented Aug 1, 2022 • edited

DAreRodz commented Jun 20, 2022 •

edited

ockham commented Jun 20, 2022 •

edited

ockham commented Jun 20, 2022 •

edited

adamziel commented Jun 23, 2022 •

edited

DAreRodz commented Aug 1, 2022 •

edited