comments_pre_query doesn't align with other pre_query filters

[dinhtungdu]

This is a follow-up to #45800.

In 45800, if $comment_data isn't null, we return it without assigning it back to the comment property. This is inconsistent with other pre_query filters behavior of other objects (User, Term, ...). Also, it creates a minor inconvenience and possibly a performance issue when querying comments.

For example, a theme/plugin hooks into comments_pre_query, and we use WP_Comment_Query to query comments.

<?php

$args = array(
    'type' => 'comment',
);

// The Query 
$comments_query = new WP_Comment_Query( $args );

To retrieve comments, we need to use $comment_query->get_comments(). Beside, when we initialize a new instance of WP_Comment_Query, the WP_Comment_Query::query() method get executed. So we're running query() method two times to get comments from the query. We can't use $comments_query->comments because it's null.

IMO, the solution here is assigning the comment data to comments property before the short-circuit.

Note that the doc of WP_Comment_Query is incorrect. We always receive an instance of WP_Comment_Query after the initialization, not 'an array of comments' as the documentation states here: ​https://developer.wordpress.org/reference/classes/wp_comment_query/#basic-usage

This PR checks and assigns the value of filtered comment data returned from comments_pre_query to WP_Comment_Query::$comments before short-curcuiting.

Trac ticket: https://core.trac.wordpress.org/ticket/50521

[spacedmonkey]

It is inline with site and network, see https://core.trac.wordpress.org/changeset/46100

But not terms, users or posts. We should fix sites and networks as well I guess.

[dinhtungdu]

@spacedmonkey I can update my PR to include those changes. Anyway, do you have any idea why my PR doesn't show up here?
Update: Here it is.

[spacedmonkey]

@dinhtungdu I can see your PR.

I would create another ticket for networks and sites. It is a different maintainer who looks after those features ( it might be me who replies ).

[spacedmonkey]

Going to ping @adamsilverstein as well, as he committed these two.

[adamsilverstein]

Hello @dinhtungdu thanks for opening this ticket!

Reviewing the original ticket I see in the discussion the decision to not set $this->comments was explicit, see this comment: https://core.trac.wordpress.org/ticket/45800#comment:18 - I removed setting in the patch following that comment.

The reasoning at the time (which I am happy to revisit) is that the function enables passing count=true and in this case we would be setting $this->comments to the count, not the comments. So if we do assign the returned value we need more logic about what/how to assign.

For example, a theme/plugin hooks into comments_pre_query, and we use WP_Comment_Query to query comments.

I'm not sure I understand your use case. The pre_ filter here is designed to enable skipping the regular WordPress db requests for getting the data - instead, data could come from a cache or any datastore, for example Elasticsearch. If you use WP_Comment_Query in your filter callback, does that mean you are using a WP db lookup anyway?

Can you give some wider context for your use case, how you plan to use the pre_ filter in your code?

This is inconsistent with other pre_query filters behavior of other objects (User, Term, ...). Also, it creates a minor inconvenience and possibly a performance issue when querying comments.

Do these other pre filters include the ability to return a count?

Note that the doc of WP_Comment_Query is incorrect

Can you please open a separate ticket with a patch to correct that (assuming this is inline doc you are talking about)?

[dinhtungdu]

@adamsilverstein Thanks for the reply!

The reasoning at the time (which I am happy to revisit) is that the function enables passing count=true and in this case we would be setting $this->comments to the count, not the comments. So if we do assign the returned value we need more logic about what/how to assign.

Agreed. And we have the naming problem exists for the method name as well, with count set to true, WP_Comment_Query::get_comments() return the count, not the comments.

If you use WP_Comment_Query in your filter callback, does that mean you are using a WP db lookup anyway?

Can you give some wider context for your use case, how you plan to use the pre_ filter in your code?

I didn't mean using WP_Comment_Query in the filter callback.

I mean when we use WP_Comment_Query to query comments, the class runs the get_comments method if arguments passed are not empty. If any plugin hooks to comments_pre_query to manipulate the result, then we need to run the get_comments method again to retrieve comments.

<?php
// When we initialize WP_Comment_Query, get_comments method will
// be called if the $args passed is not empty.
$comment_query = new WP_Comment_Query( $args ); // At this point, $comment_query->comments is null because of short-circuiting.

// To get comments, we need to call the get_comments method again
$comments = $comment_query->get_comments();

Do these other pre filters include the ability to return a count?

Other objects like User, Term, Post assign the data back to their properties. The count is handled differently in each object.

Can you please open a separate ticket with a patch to correct that (assuming this is inline doc you are talking about)?

Of course, I will do it.

[adamsilverstein]

Thanks for clarifying that get_comments is called when WP_comment_query is initialized with query args, that clears up the issue you are trying to resolve.

Agreed. And we have the naming problem exists for the method name as well, with count set to true, WP_Comment_Query::get_comments() return the count, not the comments.

I don't think we can change the naming at this point. I do think we can set the $comments_query->comments to the comments or the comment ids as you are suggesting; in the case where a count is requested we should not set anything since we only have and only requested a count.

Can you update your patch to add that logic?

We will need to document this change for developers; this change shouldn't break anything for existing uses so should be safe to add.

[dinhtungdu]

@adamsilverstein I updated my patch, both logic and documentation. Please give it another review.

[imath]

Hi @dinhtungdu

I've just had a look to your PR. Could you include a check for the count query var as suggested by @adamsilverstein?

=> if ( is_array( $comment_data ) && ! $this->query_vars['count'] )

I'll test the PR asap.

[dinhtungdu]

@imath I updated my PR as you suggested, please give it another look. Thank you!

[imath]

@dinhtungdu thanks for updating your PR.

I've just tested it and I confirm it works as expected. 50521.diff is a patch I made out of your PR. 50521.test.diff is the test checking the comments property of the comment's query is set with the array containing the object to override the db query.

I've also tested it using the filter into WordPress admin. I believe like @adamsilverstein that this change shouldn't break anything for existing uses so we should be safe to add this in next release.

Can you confirm @davidbaumwald?

[SergeyBiryukov]

In 48990:

Comments: Assign the array of comment data returned from the comments_pre_query filter to the comments property of the current WP_Comment_Query instance.

This avoids the performance overhead of calling WP_Comment_Query::get_comments() twice: first when creating the object instance, then to retrieve the filtered results.

This also makes the filter a bit more consistent with other similar filters, e.g. posts_pre_query, terms_pre_query, or users_pre_query.

Follow-up to [46086].

Props dinhtungdu, imath, spacedmonkey, adamsilverstein, SergeyBiryukov.
Fixes #50521.

[SergeyBiryukov]

Follow-up: #51333

Merged in changeset https://core.trac.wordpress.org/changeset/48990.