Add short circuits for WP_Comment_Query

[spacedmonkey]

Now that WP_Query and WP_User_Query have filters to short circuit results in from a different source. WP_Comment_Query should have some ability to do the same.

[spacedmonkey]

Related: #44169 #45749 #41246

[desrosj]

This still needs a patch. 5.2 beta is in less than 2 days. Going to punt, but feel free to move it back @adamsilverstein if a patch is created and you have time to shepherd.

[felipeelia]

Probably not soon enough to 5.2, but I'll try to submit a patch for this during the next few days.

[adamsilverstein]

Thanks for the patch @felipeelia! I'll take a look at this soon.

[felipeelia]

Just uploaded a patch with the necessary changes and a new unit test. FWIW, here is a callback that could be hooked to the new comments_pre_query filter and serve as an example:

<?php
function wp45800_comments_pre_query( $comments, $query ) {
        // Necessary to avoid an infinite loop for hierarchical queries.
        if ( ! empty( $query->query_vars['parent'] ) ) {
                return [];
        }
        $comments = [
                (object) [
                        'comment_ID'      => 100,
                        'comment_content' => 'Lorem Ipsum',
                ],
                (object) [
                        'comment_ID'      => 200,
                        'comment_content' => 'Ipsum lorem',
                ],
        ];
        if ( $query->query_vars['count'] ) {
                return count( $comments );
        }
        if ( 'ids' == $query->query_vars['fields'] ) {
                return wp_list_pluck( $comments, 'comment_ID' );
        }
        return $comments;
}
add_filter( 'comments_pre_query', 'wp45800_comments_pre_query', 10, 2 );

Thank you, @adamsilverstein. Let me know if any change is needed :)

[spacedmonkey]

Uploaded new patch 45800.2.diff.

This is a copy and paste from #45749 . If you are happy @adamsilverstein can you test and commit.

[felipeelia]

Hi @spacedmonkey!

I gave a look at your patch (trying to understand the differences between mine and yours) and there are some database queries running even when the filter is used, probably because it's caching the results.

Also, it looks like as it's always getting only an array of ints -- due to that $comment_ids = array_map( 'intval', $comment_ids ); -- the user will be forced to hook again into the_comments to pass the correct comments. I don't know if that is intentional or if I'm missing something here though.

Just to clarify and make things easier to @adamsilverstein (or whoever will review this ticket), my patch was made based on #44169 solution :)

Thanks!

[spacedmonkey]

I worked on both patches for user and site / network pre filters. I guess it is a query of what the filter is trying to do. Is the filter trying to highjack query on the database and return filtered comment ids or trying to stop any queries running in the first place.

I would agrue that other such filter, only highjack the query to get the data, not let you return your own data in place.

[felipeelia]

Thanks for replying, @spacedmonkey.

This line (present on our both patches), says it would have to stop any queries running:

$this->assertSame( $num_queries, $wpdb->num_queries );

This test failed here using your patch, by the way.

Also, if it should only return comment ids, the argument description in your patch should be changed from Return an array of comment data to short-circuit WP's comment query to Return an array of comment ids to short-circuit WP's comment query, explaining that the user should return the actual data somewhere else.

For sites_pre_query and networks_pre_query filters, yeah, the programmer would have to use another filter to really set the data but for posts_pre_query and users_pre_query, it's possible (and, as far as I got it, the expected behavior).

I can be missing some obvious things here. If so, sorry in advance :) As I'm trying to learn here, inside test_comments_pre_query_filter_should_bypass_database_query you're creating a $result var that isn't used. Is there any difference between instantiating the WP_Comment_Query class and then calling its query method from calling it directly with the query args? Thanks!

[spacedmonkey]

I have updated the patch. Can you retest @felipeelia as your feedback was correct.

[felipeelia]

Hi @spacedmonkey! I didn't run automated tests on it but giving a quick look it seems to work fine. I think the $comment_ids doc should explicit the special cases, like fields=ids and count=true, though.

I do like the simplicity of your patch. If it is clear somewhere that using the comments_pre_query filter, the developer takes the full responsibility of returning the right type, i.e.,

• an int when $this->query_vars['count']
• an array of ids when 'ids' == $this->query_vars['fields'] OR
• an array of WP_Comment objects, since it passes through $comments = array_map( 'get_comment', $_comments ); near the end

This should work good (if we consider the scenario with that wp45800_comments_pre_query I wrote before, for example). IMHO opinion these tests should be kept inside the method just to improve consistency on responses but perhaps I'm being too cautious here :)

[adamsilverstein]

In 45800.4.diff​ I updated the doc block to clarify the values developers should return from the filter as per https://core.trac.wordpress.org/ticket/45800#comment:14.

@felipeelia thanks for the head start on the wording/conditions - is this close to what you were thinking?

[adamsilverstein]

@boonebgorges if you have a few moments I would appreciate a code review here before committing this change.

Do you see any issue with exiting early here, leaving counts, caching, pagination and query_args handling up to the filter users? Anything else we should highlight in the docblock or in the eventual Dev note?

[felipeelia]

Replying to adamsilverstein:

In 45800.4.diff​ I updated the doc block to clarify the values developers should return from the filter as per https://core.trac.wordpress.org/ticket/45800#comment:14.

@felipeelia thanks for the head start on the wording/conditions - is this close to what you were thinking?

Yep. I'm happy to see this moving forward!

The only consideration I have is that the value returned in $comment_ids can also be an int (if count is set), so perhaps the params doc clause should be @param array|int|null or @param mixed, I don't know which one is better.

Also, while we're on it, do we need to assign $this->comments = $comment_ids;? Why don't we just return $comment_ids? If count is set, for example, having an int on $this->comments won't make much sense, IMHO.

ps.: English isn't my first language, so I'm sorry if I'm saying something obviously wrong here, but shouldn't it be bypass WordPress' default instead of bypass WordPress's default? :)

[boonebgorges]

Thanks for continuing to work on this, @adamsilverstein.

The pattern in 45800.4.diff​ is similar to the other query short-circuit filters, and those filters similarly require the client to follow the proper protocol regarding return values, count_total, etc. So I think it's fine to repeat the pattern here. In the dev note, it's probably worth showing an example, and then underscoring that this pattern applies to all similar queries.

[adamsilverstein]

The only consideration I have is that the value returned in $comment_ids can also be an int (if count is set), so perhaps the params doc clause should be @param array|int|null or @param mixed, I don't know which one is better.

Good point. Since we know the exact types that can be potentially returned, we should list them out rather than using the ambiguous 'mixed'.

Also, while we're on it, do we need to assign $this->comments = $comment_ids;? Why don't we just return $comment_ids? If count is set, for example, having an int on $this->comments won't make much sense, IMHO.

That is true. I think the main reason we do this is to ensure any subsequent (in the sdme load) uses of the query object will reuse the retrieved comments. It might be better to leave this up to filter consumers though since we pass a reference to the query object. That way they could set ids, comment objects or nothing at all (for the count query). If we take this approach we should also re-examine the other recently added short-circuit filters to ensure we build these consistently.

ps.: English isn't my first language, so I'm sorry if I'm saying something obviously wrong here, but shouldn't it be bypass WordPress' default instead of bypass WordPress's default? :)

Thanks for asking - this looks right to me. We are referring here to the filters that belong to WordPress and in particular the singular WordPress software. Adding a possessive s after a singular noun uses 's (ref: ​https://www.thepunctuationguide.com/apostrophe.html).

[felipeelia]

Thanks for the prompt reply, @adamsilverstein! Hopefully, the code I made in this comment can be a start for the needed dev-note.

If so, let me just leave a reminder here: as we are exiting from get_comments() earlier, we should cast those objects into WP_Comment instances. get_comments() has a $comments = array_map( 'get_comment', $_comments ); near its end and it won't be executed in short-circuited cases.

Let me know if I can help with something else, I'd be glad to.

[adamsilverstein]

If so, let me just leave a reminder here: as we are exiting from get_comments() earlier, we should cast those objects into WP_Comment instances. get_comments() has a $comments = array_map( 'get_comment', $_comments ); near its end and it won't be executed in short-circuited cases.

We leave it up the the filter consumer to handle the proper return format, if the request args are for 'fields' => 'ids' the expected return is ids instead of comment objects.

[felipeelia]

Replying to adamsilverstein:

If so, let me just leave a reminder here: as we are exiting from get_comments() earlier, we should cast those objects into WP_Comment instances. get_comments() has a $comments = array_map( 'get_comment', $_comments ); near its end and it won't be executed in short-circuited cases.

We leave it up the the filter consumer to handle the proper return format, if the request args are for 'fields' => 'ids' the expected return is ids instead of comment objects.

Sorry, I should've been clearer. I was referring to the code example that should go with the dev note, i.e., the one filter consumers will "copy/adapt" :)

[adamsilverstein]

I was referring to the code example that should go with the dev note

Ah right! I misunderstood.

[adamsilverstein]

45800.5.diff​:

• slight doc updates
• improve tests (full query)
• don't set $this->comments

[adamsilverstein]

@felipeelia / @spacedmonkey can you please review 45800.5.diff​ as a final check and I'll get it committed?

[felipeelia]

Seems 100% for me, @adamsilverstein.
Actually, the $comment_data param type should be @param array|int|null, shouldn't it? That int covers the $this->query_vars['count'] scenario.

[spacedmonkey]

Seems good to me. @adamsilverstein

[davidbaumwald]

#47168 was marked as a duplicate.

[adamsilverstein]

the $comment_data param type should be @param array|int|null, shouldn't it? That int covers the $this->query_varscount? scenario.

Updated in 45800.6.diff​

[adamsilverstein]

fixed tests in 45800.7.diff​

[adamsilverstein]

In 46086:

Comments: add a new comments_pre_query filter to short circuit WP_Comment_Query 'get_comments' queries.

Return a non-null value to bypass WordPress's default comment queries.

Props felipeelia, spacedmonkey.
Fixes #45800.

[SergeyBiryukov]

In 46087:

Docs: Clarify the int return value in comments_pre_query filter DocBlock.

See #45800.

[desrosj]

This was documented in the following dev note: ​https://make.wordpress.org/core/2019/04/11/new-network-and-sites-query-filters/.