Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#44169 closed enhancement (fixed)

New filter to short circuit WP_User_Query results

Reported by: tlovett1's profile tlovett1 Owned by: adamsilverstein's profile adamsilverstein
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch has-unit-tests has-dev-note
Focuses: Cc:

Description

#41700 added the filter posts_pre_query to WP_Query. This filter lets you short circuit the WP_Query MySQL query to return your own results. We use this filter in ElasticPress to return results from Elasticsearch instead of MySQL.

This ticket (patch incoming) is to add a similar filter to WP_User_Query.

Attachments (7)

users-pre-query-1.patch (2.7 KB) - added by tlovett1 6 years ago.
users-pre-query-test-1.patch (1.3 KB) - added by tlovett1 6 years ago.
44169.1.diff (4.0 KB) - added by adamsilverstein 6 years ago.
44169-indent-fixed.diff (3.1 KB) - added by adamsilverstein 6 years ago.
44169.2.diff (4.0 KB) - added by adamsilverstein 6 years ago.
44169.3.diff (4.3 KB) - added by spacedmonkey 6 years ago.
44169.diff (4.4 KB) - added by adamsilverstein 6 years ago.

Download all attachments as: .zip

Change History (44)

#1 @tlovett1
6 years ago

  • Keywords has-patch added

#2 @birgire
6 years ago

  • Keywords needs-unit-tests added

@tlovett1 Thanks for the patch.

It passes the current unit tests for the user group.

I will mark that this will need unit tests, if this goes forward.

Regarding the use cases, I can imagine this would be mostly used to override the search query of WP_User_Query, both front and backend?

I wonder if this could affect other processes? I don't think WP_User_Query is used during the login progress, but one could e.g. test what happens if WP_User_Query returns no users at all, during login.

ps: It seems that posts_pre_query was introduced to WP_Query ticket #36687.

#3 @tlovett1
6 years ago

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

Tests added.

#4 @tlovett1
6 years ago

Any update on this?

#5 @helen
6 years ago

@tlovett1 Would like some reassurance that this doesn't impact login and to clarify that you aren't returning early because you still want to continue on to the caching part of the query() function.

#6 @tlovett1
6 years ago

@helen this won't impact login. Yes, I would still want WP_User_Query to cache.

#7 @tlovett1
6 years ago

Any update here? We really need this.

#8 @adamsilverstein
6 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@tlovett1 - Thanks for the ticket, patch and tests! This looks good and I can see how this would be useful!

I posted a new combined diff with your patch and tests in 44169.1.diff.

The diff is unfortunately complicated and hard to look at because of the existing lines that are intended in the patch (because they sit inside a conditional); the diff has them oddly interspersed and that makes it hard to see what you are actually changing.

I created 44169-indent-fixed.diff as a workaround - in this diff i left the existing lines unindented so you can much more clearly see what is added by your patch. in this diff it is clearer that your code runs the filter passing null, then if the result is still null it runs the existing code that queries the database. if the result is not null it continues as normal with the results just after the existing database code executes.

I'm seeing some test failures I'll look into: https://travis-ci.org/adamsilverstein/wordpress-develop-fork/jobs/431694171

I'd love to see what @boonebgorges thinks since he did so much of the original work on #36687 and has a deep understanding of the implications of any changes here.

#9 @10up
6 years ago

Thanks, Adam!

#10 @boonebgorges
6 years ago

Thanks for the ping, @adamsilverstein. The approach in 44169.2.diff looks good to me. To be clear, callbacks will have to following a similar pattern to what I outlined in https://core.trac.wordpress.org/ticket/36687#comment:8:

<?php
function wp44169_do_external_users_query( $users, WP_User_Query $query ) {
    $response = wp_remote_get( 'https://my-remote-data-store/foo/bar' );
    
    if ( 200 === wp_remote_response_code( $response ) ) {
        $response_body = wp_remote_retrieve_body( $response );
        
        // Assuming the API response is a JSON object containing `user_ids` and an int `found_users`
        $response_body = json_decode( $response_body );

        $users              = array_map( 'intval', $response_body->user_ids );
        $query->found_users = (int) $response_body->found_users;
    }

    return $posts;
}
add_filter( 'users_pre_query', 'wp44169_do_external_users_query', 10, 2 );

As in the case of posts_pre_query, callbacks will have to be careful about fields - the external API may have to return user IDs or quasi-WP_User objects, depending on the value of fields.

#11 @adamsilverstein
6 years ago

  • Milestone changed from Awaiting Review to 4.9.9

@boonebgorges - Thanks for the review and documentation details about how the filter would be used. I'll get this committed - is this the type of thing that warrants dev notes?

#12 @boonebgorges
6 years ago

@adamsilverstein I don't recall a dev note about the corresponding posts_pre_query, but I think it couldn't hurt to have one here. Thanks!

#13 @adamsilverstein
6 years ago

  • Keywords needs-dev-note added

#14 @pento
6 years ago

  • Milestone changed from 4.9.9 to 5.1

#15 @adamsilverstein
6 years ago

  • Milestone changed from 5.1 to 5.0.1

#16 @adamsilverstein
6 years ago

@pento can we consider this for 5.0/.01? This change is a dependency for user support in elasticpress' upcoming version.

#17 @pento
6 years ago

Yup, 5.0.1 is fine.

#18 @spacedmonkey
6 years ago

I have been working on making user queries cachable. See the code here. I have some of the tickets to add filters to the WP_User_Query class. See #45153, #43679, #43680.

I think we should use this filter, as it much more powerful than the ones that I purposed. Love to see this in 5.0.1.

@spacedmonkey
6 years ago

#19 @spacedmonkey
6 years ago

I updated @adamsilverstein 's patch, so it now applies to trunk.

#20 @adamsilverstein
6 years ago

Thanks @spacedmonkey - I'll get this committed as soon as trunk is open for business.

#21 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#22 @tlovett1
6 years ago

@adamsilverstein can we get this committed?

#23 @adamsilverstein
6 years ago

@tlovett1 Yes! Currently we are working on merging changes from the 5.0 branch back to trunk; I need to wait for those to be completed before proceeding. once that is complete I will commit this!

@pento please let me know if that isn't right!

#24 @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#25 @adamsilverstein
6 years ago

In 44169.diff - refresh against trunk & set @since versions to '5.0.3'

#26 @audrasjb
6 years ago

Hello,

5.0.3 is going to be released soon. We are currently sorting the remaining tickets in the milestone. Do you think this ticket can be committed in the next few days? If not, let's address this in 5.1 which is coming in February.

Thanks,
Jb

#27 @adamsilverstein
6 years ago

@audrasjb yep, committing this now.

#28 @adamsilverstein
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 44373:

Users: Add a users_pre_query filter to short circuit WP_User_Query results.

Add a new filter users_pre_query - filters the users array before the query takes place. Return a non-null value to bypass WordPress's default user queries. Similar to the posts_pre_query filter for WP_Query added in #36687. This filter lets you short circuit the WP_User_Query MySQL query to return your own results.

Developers should note that filtering functions that require pagination information are encouraged to set the total_users property of the WP_User_Query object, passed to the filter by reference. If WP_User_Query does not perform a database query, it will not have enough information to generate these values itself.

Props tlovett1, birgire, boonebgorges, spacedmonkey.
Fixes #44169.

This ticket was mentioned in Slack in #core-committers by adamsilverstein. View the logs.


6 years ago

#30 @adamsilverstein
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

re-opening to consider 5.0.3 backporting

Last edited 6 years ago by adamsilverstein (previous) (diff)

#31 @adamsilverstein
6 years ago

@pento shall I go ahead and backport this to the 5.0 branch/5.0.3 or is this more appropriate for the 5.1 release? @boonebgorges suggested in slack that 5.1 is more appropriate since this adds a new filter.

#32 @adamsilverstein
6 years ago

  • Milestone changed from 5.0.3 to 5.1

tagging as 5.1 based on feedback in slack #committers channel.

#33 @adamsilverstein
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

closing as this is already merged to trunk/5.1

#34 @SergeyBiryukov
6 years ago

In 44410:

Docs: Correct @since tag for users_pre_query filter added in [44373].

Revert unintended change to the @since tag for the WP_User_Query instance added to found_users_query filter in [43660].

See #44169, #43679.

#35 @adamsilverstein
6 years ago

Thanks for fixing that @SergeyBiryukov!

#36 @desrosj
6 years ago

  • Keywords has-dev-note added; needs-dev-note removed

#37 @spacedmonkey
6 years ago

#45153 was marked as a duplicate.

Note: See TracTickets for help on using tickets.