Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 3 years ago

#6889 closed enhancement (fixed)

Remove usage of deprecated BP_Core_User::get_users()

Reported by: tw2113 Owned by: boonebgorges
Milestone: 2.8 Priority: normal
Severity: normal Version:
Component: Friends Keywords: has-patch
Cc:

Description

Presently used for three active filters:

friends_get_recently_active
friends_get_alphabetically
friends_get_newest

Attachments (5)

6889-remove-bp-core-user-use.diff (3.1 KB) - added by tw2113 3 years ago.
6889-remove-bp-core-user-use-version2.diff (2.0 KB) - added by tw2113 3 years ago.
6889-remove-bp-core-user-use-version3.diff (2.2 KB) - added by tw2113 3 years ago.
6889-remove-bp-core-user-use-tests.diff (2.7 KB) - added by tw2113 3 years ago.
6889-remove-bp-core-user-use-tests-1.diff (2.8 KB) - added by tw2113 3 years ago.

Download all attachments as: .zip

Change History (28)

#1 @DJPaul
4 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 2.6

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


4 years ago

#3 @DJPaul
3 years ago

  • Milestone changed from 2.6 to Future Release

#4 @DJPaul
3 years ago

  • Component changed from API to Core

#5 @tw2113
3 years ago

  • Owner set to tw2113
  • Status changed from new to accepted

#6 @tw2113
3 years ago

  • Milestone changed from Future Release to 2.8

#7 @tw2113
3 years ago

Let's get this one knocked out.

#8 @tw2113
3 years ago

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

#9 @tw2113
3 years ago

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?

#10 @boonebgorges
3 years ago

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.

#11 @tw2113
3 years ago

  • Keywords has-patch added; needs-patch removed

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.

#12 @boonebgorges
3 years ago

  • Component changed from Core to Friends
  • Keywords needs-unit-tests added

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.

#13 @DJPaul
3 years ago

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.

#14 @tw2113
3 years ago

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.
  1. Need to set 3 users, make 2 of them friends with the third, current user. Set users first/last names. Confirm they are alphabetical.
  1. 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.

#15 @tw2113
3 years ago

  • Keywords needs-unit-tests removed

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.

#16 @DJPaul
3 years ago

  • Keywords good-first-bug removed

Are you ready for review on your work here?

#17 @tw2113
3 years ago

Yes, please

#18 @boonebgorges
3 years ago

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

#19 @tw2113
3 years ago

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

#20 @tw2113
3 years ago

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.

#21 @boonebgorges
3 years ago

  • Owner changed from tw2113 to boonebgorges
  • Status changed from accepted to assigned

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.

#22 @boonebgorges
3 years ago

In 11353:

Add tests for legacy friends query functions.

Props tw2113.
See #6889.

#23 @boonebgorges
3 years ago

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

In 11354:

Replace calls to deprecated user query in friends legacy functions.

Props tw2113.
Fixes #6889.

Note: See TracTickets for help on using tickets.