Logic for friend queries in BP_User_Query

[dontdream]

The code in BP_User_Query prevents plugins from limiting (with the 'include' array) generic friend request queries.

BP_User_Query offers the 'include' array to limit results to a set of users, and the 'user_id' value to limit results to friends of the given user_id. In my opinion both these limits should be allowed to operate together.

I suggest removing the tests for (empty( $include )) on lines 302 and 310 of 'bp-core-classes.php'.

[dontdream]

edited copy of bp-core-classes.php

[boonebgorges]

Hi dontdream. I'm not sure I understand the use case. Would you mind sharing more details (and code) about how a plugin would "limit generic friend request queries"?

Also - it's great to be receiving contributions, but it would be even greater if you could submit them as svn patches. A general overview of how WP/BP use svn is here ​https://codex.wordpress.org/Using_Subversion. The short version is:

• Check out the BP svn repo: svn co http://buddypress.svn.wordpress.org/trunk /local/path/to/buddypress
• Make your fixes
• svn diff > /local/path/to/patchfile.diff
• Attach patchfile.diff to your ticket

Thanks :)

[dontdream]

I am thinking of a plugin (BP Profile Search) filtering members in the Members Directory page with the 'include' array.

When the filter is active, if you click on the tab 'My Friends' you get the same results you get in the tab 'All Members'. That's because, being active the 'include' array, the friends WHERE clause is not activated.

Thanks for the patch instructions! I'll resubmit my changes the proper way.

[boonebgorges]

Thanks for formatting as a patch, dontdream! Looks great.

I think that the change you've suggested is a good one, but we should be clear on what it means. The attached patch (4938.patch) contains unit tests that demonstrate the two consequences of making your suggested change:

• When you pass an 'include' parameter as well as 'user_id' parameter, you end up with two separate WHERE ... IN clauses. For example, BP_User_Query( array( 'user_id' => 1, 'include' => array( 2,3) ) ) will result in a clause that looks like this (assuming that user 1's friends are 4 and 5): u.user_id IN (4,5) AND u.user_id IN (2,3). Results will only be returned that match *all* of the clauses (it's an AND, not an OR). For the use case that you describe here, that's definitely what you want, and I think it will generally be the preferred behavior, but people who want to have inclusive SQL (where you return results that are EITHER friends of 1 OR in the array (2,3)) will have to use a different technique.

• By extension, when a user has no friends, it doesn't matter what gets passed via 'include' - zero users should always be returned. For our situation, we're appending the 'no_results' clause WHERE 0=1 in these cases (which speeds up the query, because SQL parses it first and skips the rest of the clauses).

[boonebgorges]

In 6934:

In BP_User_Query, allow 'include' params to be combined with the 'user_id' filter

This enhancement allows plugins to filter friend lists by passing an 'include'
parameter to BP_User_Query.

Fixes #4938

Props dontdream

[r-a-y]

In 6939:

Add additional unit tests for BP_User_Query.

• test_bp_user_query_include_on_user_page() - tests using only the 'include' parameter with BP_User_Query
• test_bp_user_query_friendship_requests() - emulates the 'Friends > Requests' page

See #4938.

[r-a-y]

r6934 kind of reverts r6638, which breaks friendship requests and also a workaround fix for #4563.

The unit tests in r6939 should cover both cases, which currently fail in 1.8-bleeding.

I'm not sure we can have it both ways without introducing a lot more logic.

[boonebgorges]

Good catch, r-a-y. Let's have a look to see if we can make this work more elegantly.

[boonebgorges]

Ok, I've thought about this a bit.

r-a-y - I think the regressions that you point out are not issues with BP_User_Query, but with bp_has_members(). That is, the user query class should be context-agnostic - it should respond only to the parameters that are passed into it - which, IMO, is what it's currently doing. Context-specific logic should occur in the template function (since templates are always loaded in a context). And, in this specific case, it's in bp_has_members() that the context-sniffing is already happening, in the form of the bp_displayed_user_id() check.

So, I'm going to suggest something like 4938.patch. First, it switches the tests to use bp_has_members() (they should probably be moved out of the core/classes.php file, but whatevs). Then, I've made a change in the way that the context-sniffing happens in bp_has_members(). Instead of our blanket if ( bp_displayed_user_id() ) check, I've changed it so that the user_id param only gets set if you're looking at a Friends list (not requests) page. This fixes the friend-request issue immediately. It will fix the member-loop-on-a-member-page issue for all cases except when that loop is appearing on member/friends, but I think best practices should probably dictate manually passing a user_id=0 param to bp_has_members() in those cases anyway. (That's what we do with the Members widget, for instance.)

The only concern here is backpat. Are there plugins and themes that are taking advantage of the current behavior, by displaying a list of user friends somewhere on that user's page, using bp_has_members() but *without* passing a user_id? I'm sure *someone* is doing it, but it also seems like an edge case.

If we only risk breaking a small number of things, I think it's probably worth it. Using bp_displayed_user_id() for this purpose is way too broad, if its only purpose is to filter member/friends.

[r-a-y]

I think this covers all instances.

I was thinking of removing the hardcoded bp_displayed_user_id() check from bp_has_members(), but wasn't sure of any unintended consequences.

Only adding bp_displayed_user_id() on a user's friends page makes the most sense b/c it's only used in this context in BP anyway.

Great job! I had some wild, elaborate alternatives, but I'm glad we don't have to go there!

[boonebgorges]

Thanks for the feedback, r-a-y!

[boonebgorges]

In 6941:

In bp_has_members(), only auto-set user_id if looking at a user friends page

Previously, the 'user_id' param was being pre-filled anytime you were looking
at a user page. As a result, it was impossible to do certain kinds of query
logic while looking at friends pages.

Adds unit tests for the change.

Fixes #4938