Use WP functions to retrieve user name, display name, nicename, and email.

[dcavins]

BuddyPress is doing quite a bit of work to fetch and cache some basic user info that I think we can rely on WP to fetch and cache. After looking at bp-members-functions.php for a related discussion about using WP functions to fetch the current user ID (see #7658), it appears to me that some of our original functions to fetch basic user data like user name, display name, nicename, and email could be replaced with core WordPress functions that have come along or been improved in the ten years since BP was first built. This would save on redundancy of code and also remove some one-off caches that were built to avoid multiple lookups.

Please see the attached patch for the first round of changes I'd propose.

Note that a few tests with display names are failing, but I'm not sure if the issue is with the tests or the change in code. (The tests set the xprofile data for display name, but the WP sync isn't happening because of the way it's being done.) I'm unfamiliar with the profile data sync setup, so it could be that I've caused a problem by changing bp_core_get_user_displaynames().

Thanks for your comments.

[dcavins]

Use WP functions to fetch some user data.

[DJPaul]

I think we can probably safely remove a lot of this stuff. John perhaps has some historical interest in updating the user cache code, too.

(If you search on the names of the caches you're removing, you'll find a few extra bits to remove.)

[DJPaul]

You might also be interested in looking at #5979

[dcavins]

Thanks for the referral to #5979, @DJPaul. It's a closely related idea, but I think this ticket is a little smaller than that change. I've updated the patch to remove some cache manipulations that should no longer be needed, and also to remove tests that don't apply to the new behavior. I imagine I'll need to write some new tests to check some behaviors, but I'm not sure what those are yet.

Note also that I've changed the WP<->BP profile sync hook a little bit so that the sync happens on the lower-level function xprofile_set_field_data(), so will catch direct updates, as opposed to only catching the update from one of the profile screens form submissions. I'd like someone who knows about the sync to make sure that that makes sense. Thanks!

[dcavins]

Updated patch.

[dcavins]

Some notes about changing bp_core_get_user_displaynames(), a function that used a custom-to-BP bulk caching mechanism (and now relies on WP's user cache).

I used this code (inserted in the body of WP page) as a check, with numbers reported by Query Monitor:

$users = array( 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26 );
var_dump( bp_core_get_user_displaynames( $users ) );

Before change:
Page generation time 0.49S
Peak memory usage: 25,636kB
Database query time: 0.1794S
Database queries: 41Q

With change:
Page generation time: 0.48S
Peak memory usage: 25,765kB
Database query time: 0.1896S
Database queries: 42Q

Comments welcome!

[DJPaul]

Did you have a persistent cache running when you took those numbers?

[dcavins]

Nope, I do not have a persistent cache running on my test environment. I can test with a Redis environment if that's interesting, though.

[DJPaul]

I'm interested in whether the change makes a difference to the number of database queries if the data's already been cached.
If you don't have a persistent cache, just duplicate this test code so it's called twice.

[dcavins]

Hi @DJPaul-

If you run the same function twice, you get the same number of queries (in both cases). With the persistent cache, I get these numbers in both cases:
Fresh after cache flush 15Q
Second load 7Q

I would expect the WP users cache to be hit more often than the bp_user_fullname_{$id} cache item in a persistent cache setup. :)

[DJPaul]

This looks great. If you are ready to commit after looking at my feedback, go for it.

Note also that I've changed the WP<->BP profile sync hook

I like it!

Advanced level nit-picking ;)

1) In xprofile_sync_wp_profile_on_single_field_set(), if the fields don't match, return early. Then outside the braces, on the next line, then call xprofile_sync_wp_profile(). Why? Indenting units of code can be seen as increasing their complexity. This is a measure static analysis tools rank on, but (for really large functions) I also think it makes it more readable, as we keep the intended code at the base level of indentation, and use the indentation to mark the "edge case".

if ( bp_xprofile_fullname_field_id() !== $data->field_id ) { 
return;
} 

xprofile_sync_wp_profile( $data->user_id ); 

2. "// Nicename and login were not passed." I know this comment pre-exists but does it add value in its new location? Is it not obvious, given the proximity to the start of the function, that these are optional variables? When I read it, it does seem redundant, but if you think otherwise, by all means keep it.

[dcavins]

Thanks for your thorough comments, @DJPaul. I've updated the patch and plan to commit the changes this evening. I even (especially?) appreciate the "advanced level nit-picking," ha ha!

[DJPaul]

👯‍♀️

[dcavins]

In 11817:

Change the display name WP/BP sync hook.

Instead of firing the display name synchronization routine on the general hook xprofile_updated_profile, which is run when the profile screen forms are submitted, use the lower level hook xprofile_data_after_save which is run when a profile field is updated. This catches more cases where we'd like the sync to run, like programatically updating the xprofile field.

See #7661.

[dcavins]

In 11818:

Use WP functions to retrieve user data.

Use WP functions to retrieve user name, display name, nicename, and email.

Switch functions like bp_core_get_username() to use native WordPress functions. This also removes the need for maintaining redundant cached items, like caching individual usernames, and instead relies upon WP's userdata cache.

Props dcavins, djpaul.

Fixes #7661.