Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#6242 closed defect (bug) (fixed)

Possible conflict with BuddyPress member type user cache and the WordPress term cache

Reported by: imath's profile imath Owned by: boonebgorges's profile boonebgorges
Milestone: 2.2.2 Priority: normal
Severity: normal Version: 2.2
Component: Performance Keywords: 2nd-opinion
Cc:

Description

While building a small plugin to add some UIs to manage member types, i've noticed there was a possible conflict between BuddyPress member types user cache and the WordPress term cache for the 'bp_member_type' taxonomy.

If you use get_terms( 'bp_member_type' ), there's a possibility, if the user_id == the term_id that when getting the member type for a user you get this error array_pop() expects parameter 1 to be array, object given. That's because the cache already contains the term datas. see WordPress update_term_cache() function.

So to avoid this, i suggest to use a different name for the cache group as WordPress is already using the taxonomy name, in our case : 'bp_member_type'

In the case of my plugin, i've used a fields argument != 'all' to avoid the problem.

I'm adding a unit test so that it will be easy to reproduce the problem.

Attachments (1)

6242.unittest.patch (1010 bytes) - added by imath 9 years ago.

Download all attachments as: .zip

Change History (11)

@imath
9 years ago

#1 @boonebgorges
9 years ago

  • Milestone changed from Awaiting Review to 2.2.2

Ugh, this problem stinks. It's pretty annoying that WP uses the bare taxonomy name for the cache group.

Your solution sounds right. Maybe something like 'bp_member_type_values', since we're just storing the member type value, not the whole terms. Do you have any ideas?

Let's fix this for 2.2.2.

#2 @imath
9 years ago

'bp_member_type_values' sounds ok for me.

Or as it's a list of terms object, i'd suggest some alternative names such as

  • 'bp_member_type_objects'
  • 'bp_member_type_terms'

#3 @johnjamesjacoby
9 years ago

bp_member_type_object is the most accurate, but because we always cache full objects when they are available is maybe also redundant.

The only places we use the bp_member_type cache are when touching the type for a single member, so how about bp_single_member_type?

Last edited 9 years ago by johnjamesjacoby (previous) (diff)

#4 follow-up: @boonebgorges
9 years ago

But BP is *not* caching a list of term objects. We're caching an array of term names.

It's also not quite true that we only use the cache when looking at single members. Member type data is prefetched in member loops (see bp_members_prefetch_member_type().

I'm going to go with _value, because it's a bit clearer that what's being cached is the value for a given member (strictly speaking, a term_relationship), not the type itself (which is a term).

#5 in reply to: ↑ 4 @johnjamesjacoby
9 years ago

Replying to boonebgorges:

But BP is *not* caching a list of term objects. We're caching an array of term names.

OIC.

It's also not quite true that we only use the cache when looking at single members. Member type data is prefetched in member loops (see bp_members_prefetch_member_type().

Yes, but the individual caches are still for each single member, retrieved via bp_get_non_cached_ids().

I'm going to go with _value, because it's a bit clearer that what's being cached is the value for a given member (strictly speaking, a term_relationship), not the type itself (which is a term).

Since the collision (and confusion) is between: values for the member-type itself, and values for the member, -- can we aim for something that addresses that ambiguity? bp_member_member_type even?


Worth noting this is only an issue on single-site installations, because bp_member_type is a global cache group and gets prefixed differently as a result in multisite. Yes, we absolutely want to avoid cache pollution, but we could also do that with a namespace for all of our cache groups, like bp_cache_ or something?

#6 follow-up: @boonebgorges
9 years ago

Since the collision (and confusion) is between: values for the member-type itself, and values for the member, -- can we aim for something that addresses that ambiguity? bp_member_member_type even?

Yes, bp_member_member_type is, despite its terribleness, semantically correct.

but we could also do that with a namespace for all of our cache groups, like bp_cache_ or something?

I suppose. If we run into this problem again in the future, we could consider something like that. Seems overkill for the moment.

#7 in reply to: ↑ 6 @johnjamesjacoby
9 years ago

Replying to boonebgorges:

Yes, bp_member_member_type is, despite its terribleness, semantically correct.

Terribleness is next to BP_Activity_Activityness.

#8 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 9533:

Use 'bp_member_member_type' as the member type cache bucket name.

Using 'bp_member_type' was creating the potential for collisions between WP's
taxonomy cache (which uses the taxonomy name 'bp_member_type' and term IDs as
cache keys) and BP's per-member member type cache (which uses the bucket
'bp_member_type' and user IDs as cache keys). The collisions take place only
when there is a 'bp_member_type' term ID that overlaps with a user ID.

The new cache group 'bp_member_member_type' is chosen to underscore that what's
being cached is the relationship between individual members and the user types
to which they belong.

Props imath, johnjamesjacoby.
Fixes #6242.

#9 @boonebgorges
9 years ago

In 9534:

Use 'bp_member_member_type' as the member type cache bucket name.

Using 'bp_member_type' was creating the potential for collisions between WP's
taxonomy cache (which uses the taxonomy name 'bp_member_type' and term IDs as
cache keys) and BP's per-member member type cache (which uses the bucket
'bp_member_type' and user IDs as cache keys). The collisions take place only
when there is a 'bp_member_type' term ID that overlaps with a user ID.

The new cache group 'bp_member_member_type' is chosen to underscore that what's
being cached is the relationship between individual members and the user types
to which they belong.

Props imath, johnjamesjacoby.
Fixes #6242.

#10 @DJPaul
8 years ago

  • Component changed from API - Cache to Performance
Note: See TracTickets for help on using tickets.