WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 6 weeks ago

#50521 new defect (bug)

comments_pre_query doesn't align with other pre_query filters

Reported by: dinhtungdu Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

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

Change History (9)

This ticket was mentioned in PR #375 on WordPress/wordpress-develop by dinhtungdu.


6 weeks ago

  • Keywords has-patch added

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

#2 @spacedmonkey
6 weeks ago

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.

#3 @dinhtungdu
6 weeks ago

@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.

Last edited 6 weeks ago by dinhtungdu (previous) (diff)

#4 @spacedmonkey
6 weeks ago

@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 ).

#5 @spacedmonkey
6 weeks ago

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

#6 follow-up: @adamsilverstein
6 weeks ago

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

#7 in reply to: ↑ 6 @dinhtungdu
6 weeks ago

@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.

#8 @adamsilverstein
6 weeks ago

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.

#9 @dinhtungdu
6 weeks ago

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

Note: See TracTickets for help on using tickets.