WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 35 hours ago

#45800 assigned enhancement

Add short circuits for WP_Comment_Query

Reported by: spacedmonkey Owned by: adamsilverstein
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.1
Component: Comments Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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.

Attachments (3)

45800.diff (6.8 KB) - added by felipeelia 3 months ago.
45800.2.diff (4.5 KB) - added by spacedmonkey 3 weeks ago.
45800.3.diff (2.5 KB) - added by spacedmonkey 40 hours ago.

Download all attachments as: .zip

Change History (17)

#2 @SergeyBiryukov
6 months ago

  • Component changed from General to Comments

#3 @desrosj
5 months ago

  • Milestone changed from Awaiting Review to Future Release

#4 @adamsilverstein
4 months ago

  • Milestone changed from Future Release to 5.2
  • Owner set to adamsilverstein
  • Status changed from new to assigned

#5 @desrosj
3 months ago

  • Milestone changed from 5.2 to 5.3

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.

#6 @felipeelia
3 months ago

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

@felipeelia
3 months ago

#7 @adamsilverstein
3 months ago

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

#8 @felipeelia
3 months ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

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
3 weeks ago

#9 @spacedmonkey
3 weeks ago

Uploaded new patch 45800.2.diff.

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

#10 @felipeelia
2 weeks ago

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!

#11 @spacedmonkey
2 weeks ago

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.

#12 @felipeelia
2 weeks ago

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!

#13 @spacedmonkey
40 hours ago

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

#14 @felipeelia
35 hours ago

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

Note: See TracTickets for help on using tickets.