Remove usage of deprecated BP_Core_User::get_users()

[tw2113]

Presently used for three active filters:

friends_get_recently_active
friends_get_alphabetically
friends_get_newest

[tw2113]

Let's get this one knocked out.

[tw2113]

Unless I'm mixing things up, I believe it should go as such. From:

return apply_filters( 'friends_get_recently_active', BP_Core_User::get_users( 'active', $per_page, $page, $user_id, $filter ) );

To:

return apply_filters( 'friends_get_recently_active', BP_User_Query( array( 'type' => 'active', 'per_page' => $per_page, 'page' => $page, 'user_id'  => $user_id, 'include' => $filter ) ) );

[tw2113]

Attached is an initial attempt, but I'm almost certain that it's not right yet, due to the fact we'd be returning a BP_User_Query object, instead of an array of total+paged users. However, I'm not quite seeing any methods on the BP_User_Query function that would be used to necessarily fetch the results.

Perhaps remove the new BP_User_Query to above, and construct an array ourselves based on the results, to avoid backcompat breaking for the filter?

[boonebgorges]

Perhaps remove the new BP_User_Query to above, and construct an array ourselves based on the results, to avoid backcompat breaking for the filter?

Yes. See bp_core_get_users() for an example.

[tw2113]

Perfect, that function already returns what we need. So, attached is a new version of the diff that simply passes the arguments into bp_core_get_users() and lets that return the value to the filter itself.

[boonebgorges]

This looks good at a glance. But I don't feel comfortable making non-critical changes to these functions without having decent test coverage for them.

[DJPaul]

A few tests should finish this up nicely and give us reassurance that we've not changed the return value or some other aspect of the function.

[tw2113]

Note to self:

Add tests to following functions:

1. friends_get_recently_active
2. friends_get_alphabetically
3. friends_get_newest

1. Need to set 3 users, make 2 of them friends with the 3rd, current user. Mark first two of them as recently active. Confirm return value of other 2.

2. Need to set 3 users, make 2 of them friends with the third, current user. Set users first/last names. Confirm they are alphabetical.

3. Need to set 3 users, make 2 of them friends with the third, current user. Confirm that returned value is the most recently added friend.

[tw2113]

Thanks to working up some unit tests, I realized that the function required associative arrays and not indexed arrays like originally provided. version3 will amend that detail. Lastly, the tests diff will provide unit tests for the three functions in question. At present time, all are passing.

[DJPaul]

Are you ready for review on your work here?

[tw2113]

Yes, please

[boonebgorges]

I'm getting PHP notices when running the tests *before* applying 6889-remove-bp-core-user-use-version3.diff​. I think it's because bp_core_get_users() returns WP_User objects (which have an ID property) while the legacy function does not. This suggests that a bit more exploration should be done surrounding back compat :)

[tw2113]

hrm...will look into and report back. Sorry about that.

[tw2113]

Attached is updated tests patch.

So, interesting results. I have amended the tests patch to use "id" instead of "ID", after noticing the BP_Core_User::get_users version returned that as well as the newer, non-deprecated method. So that conflict should be resolved.

However, with that taken care of, the tests pre-patch are mostly getting the deprecated notice for the original version. The one exception is the function to get users in alphabetical order. This has me thinking the current version of the function, which is not used by BP Core anyway, is actually buggy and not returning in alphabetical order.

With the patched version above, removing the deprecated methods, the tests are passing cleanly. So in regards to the "back compat" topic, I think this is a case that we can accept, because of the new version passing cleanly. Updating from the deprecated methods, actually fixes an issue.

[boonebgorges]

Thanks for continuing to work on this, @tw2113.

The alphabetical test is broken not because of a core bug, but because the fixtures are not set up properly. BP_Core_User::get_users() does its alphabetical sorting against xprofile data, but wp_update_user() doesn't update xprofile data.

I'll clean things up (including your space indentation!!!!!!!) and commit.

[boonebgorges]

In 11353:

Add tests for legacy friends query functions.

Props tw2113.
See #6889.

[boonebgorges]

In 11354:

Replace calls to deprecated user query in friends legacy functions.

Props tw2113.
Fixes #6889.