Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

#7477 closed enhancement (fixed)

BP_Groups_Group::group_exists() should be cached.

Reported by: dcavins's profile dcavins Owned by: dcavins's profile dcavins
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.8.2
Component: Groups Keywords: has-patch
Cc: dcavins

Description

In working to add support for finding groups by old slugs, I'm using BP_Groups_Group::group_exists() more than once per page load, and that's causing duplicate queries. It seems straightforward to use some of our incremented caching here.

Attachments (4)

7477.1.diff (3.4 KB) - added by dcavins 8 years ago.
Add cache check in BP_Groups_Group::group_exists() and related tests.
7477.2-change-get.diff (4.2 KB) - added by dcavins 8 years ago.
Update BP_Groups_Group::get() to accept slug as an argument.
7477.3-use-get-slug.diff (1.4 KB) - added by dcavins 8 years ago.
Use get() to find a group by slug in group_exists().
7477.4.diff (5.9 KB) - added by dcavins 8 years ago.
Allow slug argument to accept string, array or comma-delimited string.

Download all attachments as: .zip

Change History (17)

@dcavins
8 years ago

Add cache check in BP_Groups_Group::group_exists() and related tests.

#1 @johnjamesjacoby
8 years ago

Can we call the get() method internally here, and return the same true/false based on whether a group is returned? I haven't looked, but that might be easier than having another bit of MySQL to maintain.

What's the table_name parameter in there for. That's pretty odd (maybe cool?) now that I see it like this.

#2 @dcavins
8 years ago

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

@johnjamesjacoby I like the way you think, but, alas, the get() method doesn't have a slug option. We could add it to get() and then we'd also have caching.

I have no idea why there's a table name parameter. So people could use a separate and super fast table for looking up groups?

The table name parameter would have to be deprecated if we moved to using get() internally, if that's an issue.

@dcavins
8 years ago

Update BP_Groups_Group::get() to accept slug as an argument.

@dcavins
8 years ago

Use get() to find a group by slug in group_exists().

#3 @dcavins
8 years ago

7477.2 and 7477.3 change get() to accept a slug, and then uses that change in group_exists(), as @johnjamesjacoby suggested. I like this approach, and don't see any downsides, unless I'm missing some performance effect.

@boonebgorges, do have an opinion on the approach to use?

Thanks for your feedback!

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


8 years ago

#5 @r-a-y
8 years ago

I'm concerned about the cache purging strategy when we use bp_core_get_incremented_cache() to fetch the group cache, especially when working with the edit group slug ticket (#6014).

If group A changes their slug from old-slug to new-slug and Group B is created with the slug old-slug. Wouldn't using BP_Groups_Group::group_exists( 'old-slug' ) return Group A due to how the cache key in bp_core_get_incremented_cache() works?

How does cache invalidation work here?

Version 1, edited 8 years ago by r-a-y (previous) (next) (diff)

#6 @dcavins
8 years ago

Thanks for your comments, @r-a-y!

I'm assuming that the better method is the .2 and .3 diff strategy which uses BP_Groups_Group::get() internally.

In that case, if group A's slug is changed via groups_edit_base_group_details() ( per the routine in #6014), then the action groups_details_updated will be triggered, so Group A's specific cache entry will be deleted.
https://buddypress.trac.wordpress.org/browser/tags/2.8.2/src/bp-groups/bp-groups-cache.php#L64

Either approach also relies on the cache incrementor--whether I'm calling it directly or get() is. As far as I understand, the cache incrementor is reset when the groups_group_after_save action is called, so queries after a group is updated will miss the cache and need to be re-run since the group table or meta table has been changed in a meaningful way.
https://buddypress.trac.wordpress.org/browser/tags/2.8.2/src/bp-groups/bp-groups-cache.php#L257

Am I understanding that correctly?

Thanks!

#7 @r-a-y
8 years ago

As far as I understand, the cache incrementor is reset when the groups_group_after_save action is called, so queries after a group is updated will miss the cache and need to be re-run since the group table or meta table has been changed in a meaningful way.

Yeah, you're right. For some reason, I thought the group cache invalidation was less of a sledgehammer, but it works in this context! :)

That was my main hang-up. The rest of the patch looks good to me.

#8 @boonebgorges
8 years ago

  • Keywords dev-feedback removed

Seems like a good approach to me. Go forth, young man.

#9 @dcavins
8 years ago

Thanks, commenters!

@boonebgorges, one specific question: I wrote the change to get() to accept a single slug argument as a string. Would you prefer that it accept an array or comma-separated string and use IN() SQL logic instead of = logic? I was imagining it for finding a group by slug, but I guess I can imagine the case where someone would want to find groups with slug IN ( 'apples', 'oranges' ). Either way seems OK to me. If you have a preference, let me know.

#10 @boonebgorges
8 years ago

Yes, let's make it IN() right off the bat. Looking through BP, we have a preference for single-purpose query params, instead of slug vs slug__in. So let's stick with it here.

@dcavins
8 years ago

Allow slug argument to accept string, array or comma-delimited string.

#11 @dcavins
8 years ago

I've attached a new version of the patch to get() that changes the slug argument to accept a string, array, or comma- or space-delimited string.

There are a few ways to get from an array to a quoted, comma-separated list for use in IN(). The new patch follows the pattern used by post_name__in in WP_Query:

$r['slug'] = array_map( 'sanitize_title', $r['slug'] );
$slug_in = "'" . implode( "','", $r['slug'] ) . "'";

but this pattern I came up with before I looked at WP_Query also works:

foreach ($r['slug'] as $key => $value) {
        $r['slug'][$key] = $wpdb->prepare( "%s", sanitize_title( $value ) );
}
$slug_in = implode( ",", $r['slug'] );

If anyone has a preference, let me know. :)

#12 @r-a-y
8 years ago

I'd go with array_map() if we want to avoid the $wpdb->prepare() call, which will waste a few more CPU cycles.

#13 @dcavins
8 years ago

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

In 11525:

Use BP_Groups_Group::get() in BP_Groups_Group::group_exists()

Retire the one-off, direct database query in
BP_Groups_Group::group_exists() in favor of using
`BP_Groups_Group::get()’ internally. Also, the new form takes advantage
of the built-in incremented caching in `BP_Groups_Group::get()’.

Fixes #7477.

Note: See TracTickets for help on using tickets.