Opened 8 years ago
Closed 8 years ago
#7477 closed enhancement (fixed)
BP_Groups_Group::group_exists() should be cached.
Reported by: | dcavins | Owned by: | 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)
Change History (17)
#1
@
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
@
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.
#3
@
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
@
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?
#6
@
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
@
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
@
8 years ago
- Keywords dev-feedback removed
Seems like a good approach to me. Go forth, young man.
#9
@
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
@
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.
#11
@
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. :)
Add cache check in BP_Groups_Group::group_exists() and related tests.