`bp_core_userdata_` cache is unnecessary

[johnjamesjacoby]

A note in bp-core-classes.php incorrectly states that:

// WP_User_Query doesn't cache the data it pulls from wp_users,
// and it does not give us a way to save queries by fetching
// only uncached users. However, BP does cache this data, so
// we set it here.

WP_User_Query does cache data pulled from wp_users via calls to WP_User for each queried result. WP_User::get_user_by gets from and sets each user object as needed.

Our judicious use of bp_core_get_core_userdata() helps act as a funnel for calls to deeper methods, so our abstraction will help keep any fixes relatively small and low-impact.

[johnjamesjacoby]

Related to #5733.

[johnjamesjacoby]

Related to #5903.

[johnjamesjacoby]

and it does not give us a way to save queries by fetching only uncached users

WP_User_Query does this for us by calling cache_users() which uses _get_non_cached_ids() to get the missing users, then calling update_user_caches() and update_meta_cache( 'user', $ids ); on them.

[boonebgorges]

cache_users() is only called if we call fields=all_with_meta, which BP_User_Query does not do. But yes, you are correct about WP_User.

Would be nice to have a unit test that shows it's cached by WP, and I shudder at your ternary syntax, but I'm on board with tearing out our redundant caching.

[johnjamesjacoby]

Replying to boonebgorges:

cache_users() is only called if we call fields=all_with_meta, which BP_User_Query does not do. But yes, you are correct about WP_User.

Just ran into that. You beat me to the updated patch. I see no reason why BP_User_Query shouldn't assume all_with_meta considering our current usages.

Would be nice to have a unit test that shows it's cached by WP, and I shudder at your ternary syntax, but I'm on board with tearing out our redundant caching.

I'll get test coverage next.

[boonebgorges]

Just ran into that. You beat me to the updated patch. I see no reason why BP_User_Query shouldn't assume all_with_meta considering our current usages.

all_with_meta is pretty heavy-duty - it primes the usermeta cache for each matched user. On the typical BP setup, I don't think we use much usermeta in the loop that we're not fetching elsewhere, and in my experience, a lot of plugins can leave a lot of junk in usermeta. So I'm wary of defaulting to it. As long as the core wp_users data is being cached (which it is in WP_User) I lean toward not using all_with_meta, and maybe allowing it as an option in BP_User_Query.

Regarding tests, just to give you a head start: look at $wpdb->num_queries before and after running :)

[johnjamesjacoby]

If we don't use all_with_meta then we don't get the cache goodness. And we do make quite a few calls to bp_get_user_meta() that (without access to the code at the moment) I assume boil back down to a cached user object. I agree that needlessly getting usermeta doesn't make sense, but neither does skipping the cache just because we don't need the meta at that point.

It's definitely worth scrutinizing more. I have a hunch the tests will be pretty revealing. Thanks for the hint on where to look.

[boonebgorges]

And we do make quite a few calls to bp_get_user_meta() that (without access to the code at the moment) I assume boil back down to a cached user object

If this is true, then yes, we should probably cache it. Your best resource here will be SAVEQUERIES: if you turn on all_with_meta, load the user directory, and see an extra 20 database queries, you'll know that we don't need the extra data. It's also worth remembering that user_login, user_nicename, and user_email will be cached by WP_User, and IIRC they are the only values that we regularly need in the loop. But all of this should of course be driven by metrics, so thanks for looking into it more deeply!

[johnjamesjacoby]

We also have our own caches for nicenames and such that I'm intentionally not touching until I have a chance to get real metrics about what we are and should be caching for user objects.

[johnjamesjacoby]

In 9116:

Use uppercase ID in WP_User based objects to prevent deprecated notices. See #5979.

[djpaul]

In 9119:

Core: partial revert of r9116 (user->id to user->ID changes).

In our legacy user fetching functions, we use something similar to "select ... ID as id" in the SQL. In the reverted changes, we are only using the property to return an array of integers, and the change of the property name was causing PHP to throw "undefined property" messages.

As these lines would have never thrown a WordPress core deprecated warning about using id instead of ID, I'm changing them back to clear the warning message and fix the unit tests.

See #5979

[johnjamesjacoby]

In 9122:

Partial revert of r9116 (user->id to user->ID changes)

Has potential for breakage in themes, and requires more investigation before fully addressing.

See #5979.

[r-a-y]

Not sure what the status of this ticket is, but I ran into another 'bp_core_userdata_' cache invalidation issue when marking a user as a spammer.

I've attached a patch.

[boonebgorges]

Replying to r-a-y:

Not sure what the status of this ticket is, but I ran into another 'bp_core_userdata_' cache invalidation issue when marking a user as a spammer.

I've attached a patch.

Hm. 5979-tests.patch​ shows that your fix is not necessary. And it's because on 'bp_make_spam_user' and 'bp_make_ham_user', we call bp_core_remove_data(), which flushes the entire cache!! Um, we probably should not do this.

[johnjamesjacoby]

Replying to boonebgorges:

we call bp_core_remove_data(), which flushes the entire cache!! Um, we probably should not do this.

This sounds vaguely familiar. I think because spamming a user intersects several counts and queries, it made more sense to brute-force-flush at the time this was put in. Maybe to flush friends or group memberships or something else too?

We'll no doubt run into a few more cases like this, where a more targeted caching strategy should be implemented.

[dcavins]

This issue was revisited in #7595 and fixed in r11705.