WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 26 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 dev-feedback needs-dev-note
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 (5)

45800.diff (6.8 KB) - added by felipeelia 5 months ago.
45800.2.diff (4.5 KB) - added by spacedmonkey 3 months ago.
45800.3.diff (2.5 KB) - added by spacedmonkey 2 months ago.
45800.4.diff (2.9 KB) - added by adamsilverstein 3 weeks ago.
45800.5.diff (2.8 KB) - added by adamsilverstein 26 hours ago.

Download all attachments as: .zip

Change History (31)

#2 @SergeyBiryukov
8 months ago

  • Component changed from General to Comments

#3 @desrosj
7 months ago

  • Milestone changed from Awaiting Review to Future Release

#4 @adamsilverstein
6 months ago

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

#5 @desrosj
5 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
5 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
5 months ago

#7 @adamsilverstein
5 months ago

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

#8 @felipeelia
5 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 :)

#9 @spacedmonkey
3 months 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 months 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 months 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 months 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
2 months ago

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

#14 @felipeelia
8 weeks 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 :)

#15 follow-up: @adamsilverstein
3 weeks ago

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?

#16 @adamsilverstein
3 weeks ago

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

#17 @adamsilverstein
3 weeks ago

  • Keywords dev-feedback added

#18 in reply to: ↑ 15 @felipeelia
3 weeks ago

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

#19 @boonebgorges
3 weeks ago

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.

#20 @JeffPaul
2 weeks ago

  • Keywords needs-dev-note added

#21 @adamsilverstein
2 weeks ago

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

#22 @felipeelia
2 weeks ago

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.

#23 follow-up: @adamsilverstein
27 hours ago

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.

#24 in reply to: ↑ 23 @felipeelia
27 hours ago

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

#25 @adamsilverstein
26 hours ago

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

Ah right! I misunderstood.

#26 @adamsilverstein
26 hours ago

45800.5.diff:

  • slight doc updates
  • improve tests (full query)
  • don't set $this->comments
Note: See TracTickets for help on using tickets.