-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport functions required by Comment Query Loop and related blocks #2543
Backport functions required by Comment Query Loop and related blocks #2543
Conversation
<?php | ||
/** | ||
* Comment Template block tests | ||
* | ||
* @package WordPress | ||
* @subpackage Block Library | ||
* @since 6.0.0 | ||
*/ | ||
|
||
/** | ||
* Tests for the Comment Template block. | ||
* | ||
* @since 6.0.0 | ||
* | ||
* @group block-library | ||
*/ | ||
class Test_Block_Library_CommentTemplate extends WP_UnitTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've defined a new @subpackage
and @group
for this test, as this is the first one we have for a specific block (as far as I saw).
I've also modified the class name to follow the same format as the other test files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if this file should be placed under a different folder/subpackage/etc. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine as is. It's an implementation detail I can look at later.
Uh, there's a missing function, I'm taking a look. |
b2d9654
to
269aef2
Compare
This reverts commit 2c0a02d.
); | ||
} | ||
|
||
function test_build_comment_query_vars_from_block_with_context() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this test is failing in my local environment. I'm taking a look into that. 👀
1) Test_Block_Library_CommentTemplate::test_build_comment_query_vars_from_block_with_context
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
'status' => 'approve'
'no_found_rows' => false
'update_comment_meta_cache' => false
- 'post_id' => 4
'hierarchical' => 'threaded'
'number' => 5
'paged' => 1
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the same error locally. It's really surprising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, the block is not being instantiated here with context; it's empty.
$block = new WP_Block( | |
$parsed_blocks[0], | |
array( | |
'postId' => self::$custom_post->ID, | |
) | |
); |
I see the same happening in all the tests where context is used. Could it be related to the block not included in WordPress core yet? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I know why, we need to have the block code first and we can then land the tests. All good then. Let's wait for #2564 and see how it looks afterwards. Good detective work 😄
I committed all the changes but tests with https://core.trac.wordpress.org/changeset/53138 to unblock #2564. |
I backported 3 out of 5 unit tests to WordPress core with https://core.trac.wordpress.org/changeset/53172. I had some issues with:
I would appreciate some help. It can be another PR if that makes it easier. |
Hey, @gziolo, sure! Let me create a new PR for that. I think it would be easier as there are some changes already integrated into |
The problem was using render_block_core_comment_template( null, null, $block ) instead of $block->render() in the failing tests. The second call returns (almost) the same output, but also updates ―among other things― The first call works in Gutenberg, though; I don't know why. 🤷 I've created a new PR here with the test fixes: #2613. I can close this one if you feel so. cc: @gziolo |
Awesome, thank you so much 💯 |
// Set the `cpage` query var to ensure the previous and next pagination links are correct | ||
// when inheriting the Discussion Settings. | ||
if ( 0 === $page && isset( $comment_args['paged'] ) && $comment_args['paged'] > 0 ) { | ||
set_query_var( 'cpage', $comment_args['paged'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why write to a query variable here? It breaks everything else in WP. It should remain 0
on the first page, as it always had been.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-referencing #6291, where this is now being discussed, and WordPress/gutenberg#39227, where it was originally introduced.
Backport changes from Gutenberg to add functions required by Comment Query Loop and related blocks.
From
/lib/compat/wordpress-6.0/blocks.php
:build_comment_query_vars_from_block
functionget_comments_pagination_arrow
functiongutenberg_extend_block_editor_settings_with_discussion_settings
filter's callbackgutenberg_rest_comment_set_children_as_embeddable
filter's callbackTests:
class-block-library-comment-template-test.php
unit testAlso being tracked in WordPress/gutenberg#39889.
Trac ticket: https://core.trac.wordpress.org/ticket/55505 and https://core.trac.wordpress.org/ticket/55567
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.