BP_Groups_Group::group_exists() should be cached.

[dcavins]

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.

[dcavins]

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

[johnjamesjacoby]

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.

[dcavins]

@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]

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

[dcavins]

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

[dcavins]

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!

[r-a-y]

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() is generated?

How does cache invalidation work here?

[dcavins]

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!

[r-a-y]

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.

[boonebgorges]

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

[dcavins]

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.

[boonebgorges]

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]

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

[dcavins]

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. :)

[r-a-y]

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

[dcavins]

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.