Skip to:
Content

Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#7078 closed enhancement (fixed)

Groups: Replace `BP_Group_Member` static method DB queries with updated group membership functions

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.6 Priority: normal
Severity: normal Version:
Component: Groups Keywords: has-patch
Cc:

Description (last modified by r-a-y)

Addendum to #6327.

This patch replaces all most BP_Group_Member static methods relying on DB queries with the new group membership cached versions available from #6327.

Patch also fixes up groups_check_user_has_invite() and groups_check_for_membership_request() to use the new `bp_get_user_groups() function.

This saves up to 6 queries on single group pages.

One thing that this patch changes though is the return value of null values in the BP_Group_Member static methods. Failures now return integer 0 instead.

Patch passes all existing unit tests.

Attachments (1)

7078.01.patch (7.1 KB) - added by r-a-y 19 months ago.

Download all attachments as: .zip

Change History (11)

@r-a-y
19 months ago

#1 @r-a-y
19 months ago

  • Description modified (diff)

#2 @dcavins
19 months ago

@r-a-y: Nice!

I'm wondering about where the logic should actually live for the request and invite functions, since now we have two functions and a static method for each type that do similar things.

@boonebgorges: Would it architecturally make sense to move the logic from your new function groups_is_user_invited() to BP_Groups_Member::check_has_invite( $user_id, $group_id, $type ) and let both groups_is_user_invited() and groups_check_user_has_invite() use the underlying logic? (I think the difference between them is that one allows the $type param and the return values are slightly different.)

Same for BP_Groups_Member::check_for_membership_request( $user_id, $group_id ), groups_is_user_pending() and groups_check_for_membership_request()? I know part of the reason for your new functions is so that the function naming scheme is not parallel otherwise.

What's the clever way to have all of these pieces work together without repeating logic? Should we be deprecating groups_check_for_membership_request() and groups_check_user_has_invite()(and the underlying static methods) in favor of the new functions?

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


19 months ago

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


19 months ago

#5 @boonebgorges
19 months ago

  • Owner set to boonebgorgesWhat's the clever way to have all of these pieces work together without repeating logic?
  • Status changed from new to assigned

Cool, let's definitely do something like this.

The changes to groups_check_user_has_invite() and groups_check_for_membership_request() look good to me.

It feels funny to replace the guts of a database query method like this. While I'm a fan of the new caching strategy here (obviously!) there may still be legitimate use cases where one wants to fetch hot membership data right from the db - SELECT id FROM wp_bp_groups_members .... My inclination would be to swap out all calls to BP_Groups_Member::check_is_invite() with groups_is_user_member() etc. Then the database query method will no longer be used by BP core, but will stay unchanged for anyone referencing it.

A bonus of this is that we get rid of some direct calls to the BP_Groups_Member methods; generally we like to standardize on the procedural wrappers, for better filter support, etc. I think this would also cover @dcavins's concern about "repeating logic" - or at least it'd make us more internally consistent.

This is mostly a gut feeling, and I don't feel strong about it, so if @r-a-y and @dcavins both think that 7078.01.patch is better, then go for it.

#6 @boonebgorges
19 months ago

  • Owner changed from boonebgorgesWhat's the clever way to have all of these pieces work together without repeating logic? to r-a-y

Weird, I don't know how I assigned the ticket to boonebgorgesWhat's the clever way to have all of these pieces work together without repeating logic?. Cool trick.

#7 @r-a-y
19 months ago

Let's go conservative as @boonebgorges suggests and keep the DB methods the same.

I'll switch out calls to our DB methods with our newly-cached group membership functions instead.

#8 @r-a-y
19 months ago

In 10819:

Groups: Change logic for groups_check_user_has_invite() and groups_check_for_membership_request().

This commit changes the internal logic for groups_check_user_has_invite()
and groups_check_for_membership_request() to use the new group membership
caching logic introduced in #6327.

See #7078.

#9 @r-a-y
19 months ago

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

In 10820:

Replace group membership static DB method calls with group membership functions.

Our group membership functions now have better caching logic due to #6327
and #7078.

This commit switches all our older group membership static DB method calls
with these group membership functions so sites enabled with an object cache
can benefit from these improvements.

Props r-a-y, boonebgorges, dcavins.

Fixes #7078.

#10 @dcavins
19 months ago

Excellent. Thanks for your work on this and making the commits!

Note: See TracTickets for help on using tickets.