Fast follow: Ensure block comments filter does not affect subsequent queries

[peterwilsoncc]

What?

Follow up to earlier PR fixing this bug to do the filtering on the pre_get_comments filter.

This is preferable than messing around with the database WHERE clause directly.

I've also added a bunch of unit tests to ensure that the code is working as expected.

Closes #71711
Follow up to #71712

Why?

Messing around with the database query clauses is flakier than using the query variables.

How?

Moves the changes back to the previous hook.

Testing Instructions

1. Create and publish a post
2. Write two block comments on the post via editor
3. Write two actual comments on the post via the front end
4. Approve the front end comments if required by your moderation settings
5. Run the following in wp shell, replacing the post ID with the ID created above

wp> get_comments( ['post_id' => 89, 'fields' => 'ids'] );
// Ensure the resulting IDs do not include the block comments
wp> get_comments( ['post_id' => 89, 'fields' => 'ids', 'type' => 'block_comment'] );
// Ensure the resulting IDs include the block comments and not the front end comments

Testing Instructions for Keyboard

N/A

Screenshots or screencast

N/A

[andrewserong]

Closes #71727

I think you might mean #71711? 😄

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: jeffpaul <jeffpaul@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[andrewserong]

I figure the least I could do after commenting is to take this PR for a spin!

This is testing well for me and looks like a good logical change to filter pre_get_comments rather than dealing with the SQL query 👍

✅ Querying get_comments or running wp comment list defaults to only showing actual comments, excluding block comments
✅ Calling get_comments with block_comment type reveals just the block comments
✅ Test coverage looks good

LGTM! 🚀

[t-hamano]

@peterwilsoncc, I think we don't need to prepare a core backport PR, as this feature is still experimental.

Edit: Sorry, I was mistaken, the core backports only have unit tests, so that's fine.

[adamsilverstein]

nice to have: another test that confirms that query is unchanged when block comments are explicitly excluded (type__not_in).

[peterwilsoncc]

@adamsilverstein Added in 265b4c2, is that what you were after?

[adamsilverstein]

Looks good, thanks @peterwilsoncc!

[t-hamano]

Can we merge this PR?

[Mamaduka]

I was working on a different issue and noticed that the exclude_block_comments_from_admin filter still affects some sub-queries. Currently, it prevents the generation of embeddable children links in REST API responses.

The $comment->get_children returns nothing for the block_comment type. If I remove the filter it's working correctly.

[peterwilsoncc]

Thanks George, the filter might need to bypass the changes if the parent ID is set. I'm on leave at the moment so won't get to it until next week. I'm happy if someone wants to push to this branch.

[Mamaduka]

Thanks for the pointer, @peterwilsoncc!

I pushed the changes for the parent query vars. Unfortunately, that turned out to be just one part of the puzzle.

Now response returns the correct link for the children embedding, but the URL itself returns no results. I can only get that URL working if I specify type: 'block_comment' and status: 'all'.

This is due to the default collection values for these arguments. We'll probably have to make changes there without breaking backward compatibility. That's outside of the scope here.

// Children embed URL, that doesn't work.
wp.data.select( 'core' ).getEntityRecords( 'root', 'comment', {
  parent: 240,
} )

// Modified query that works.
wp.data.select( 'core' ).getEntityRecords( 'root', 'comment', {
  type: 'block_comment',
  status: 'all',
  parent: 240,
} )

[Mamaduka]

The comment type was changed to match the final feature name. Ref: ##72310.

[t-hamano]

Based on this feedback, we added type__not_in directly to the WP_Comments_List_Table class, rather than directly manipulating the database WHERE clause.

Can we apply a similar approach to this PR? In other words, we should be able to use the comments_list_table_query_args hook.

[Mamaduka]

I just tested the filter. It works for the list table items, but the Menu indicator and list filters still display the count.

Screenshot

[t-hamano]

I just tested the filter. It works for the list table items, but the Menu indicator and list filters still display the count.

@Mamaduka You're right. Additionally, there appear to be other items that need to be addressed.

cc @swissspidy

[t-hamano]

Based on this feedback, we added type__not_in directly to the WP_Comments_List_Table class, rather than directly manipulating the database WHERE clause.

Can we apply a similar approach to this PR? In other words, we should be able to use the comments_list_table_query_args hook.

I just tested the filter. It works for the list table items, but the Menu indicator and list filters still display the count.

In the core backport PR, I was able to address this issue by adding a type__not_in query in the get_comment_coint() function.

https://github.com/WordPress/wordpress-develop/pull/10280/files#diff-2b8fe4b1429ba7f3a26288d345bbffe07feed5bd7ee515ad139f844a1d3c14a2R420

However, the query parameters of the get_comment_count() function are not filterable, so using pre_get_comments might be our only option in Gutenberg 🤔

[t-hamano]

As I understand it, this PR is an improvement for the Gutenberg plugin and does not affect the core, so I will remove the Backport to WP 6.9 Beta/RC label.

@peterwilsoncc @adamsilverstein I have resolved the conflicts for now, but I would like to confirm whether this PR is still valid.

[t-hamano]

This is an issue within the Gutenberg plugin, so let's address it in a point update.

[Mamaduka]

The plugin-specific issues don't have to be in WP-specific milestones.

[jeffpaul]

@Mamaduka reminder ping on this PR as something impacting the Gutenberg plugin specifically (i.e. not needed for WP 6.9 as not impacting WP Core)