Incorrect 'ids' param passed to `groups_get_invites()` causes all invites to be returned

[boonebgorges]

After an upgrade to 5.0, I noticed a severe slowdown on a site with a fairly large number of invitations. After some tracing, I found the following:

• groups_get_user_groups(), which is called in many ways throughout BP - including groups_is_user_member(), etc - tries to cache uncached invitation objects
• In the case of my installation (running a Memcached backend) the invitation objects are all being stored in the cache. As a result, $uncached_invitation_ids is empty ​https://buddypress.trac.wordpress.org/browser/tags/5.0.0/src/bp-groups/bp-groups-functions.php?marks=982#L971
• This array is being passed as the ids param to groups_get_invites() ​https://buddypress.trac.wordpress.org/browser/tags/5.0.0/src/bp-groups/bp-groups-functions.php?marks=985#L971
• But groups_get_invites() doesn't appear to accept an ids param

The result is that, on sites running an object cache, every call to groups_get_invites() is triggering every single invite to be fetched from the database. This can be crippling in certain circumstances.
I think that the general fix should be to change the ids param to id on line 985 linked above. Alternatively (or in addition?) when $uncached_invitation_ids is empty, there's no reason to even make the call to groups_get_invites() in the first place.

@dcavins Could you please have a look at the above and let me know what you think?

[dcavins]

Hi @boonebgorges,

Thanks for creating this ticket. Yes, that ids parameter should be id--it's a holdover from an earlier version of the code where I was using ids as the key.

I also agree that there's no reason to run the groups_get_invites() routine if no uncached invitations are found.

Furthermore, this issue highlights the need to improve the handling of special cases of the id parameter in BP_Invitation::get_where_sql(): ​https://buddypress.trac.wordpress.org/browser/tags/5.0.0/src/bp-core/classes/class-bp-invitation.php#L343

WP is a little fuzzy on patterning parameters like this, but I'd expect:

• id = array() should return no results. (Not a database error.)
• id = array( 0 ) should return no results.
• id = 0 should return no results.
• id = "0" should return no results.
• id = "" should return no results.
• id = false should result in no id IN () statement being added to the parameter.

Thanks! Please comment on the patch, which is in line with your suggestions above.

[dcavins]

Correct parameter key; do not run followup routine if nothing to do.

[boonebgorges]

Yup, this is perfect! (And I agree with your semantics of id. Only thing I'd add is that null should probably behave like false.)

[dcavins]

Revise handling of id parameter in BP_Invitation::get(). Add tests for common edge cases.

[dcavins]

I've just added a change to the handling of the id parameter in BP_Invitation::get() as discussed above. I believe this change addresses the handling of "zero-like" values so that the results are as expected--and you won't have to ensure that the array you're passing as the id param to groups_get_invites() is non-empty.

Comments are welcome! Thanks!

One other thing: If this change makes sense, should it go in BP5.1 or 6?

[boonebgorges]

The clarifications of 'id' behavior are good, but I don't think they need to be rushed into BP 5.1. My preference would be to make those changes in a major release, and perhaps do a somewhat broader analysis for consistency - it would be a huge job to do all of our query functions that accept 'id', but at least we could do all of them in (say) the groups component, as a starting point.

[dcavins]

In 12486:

Fix incorrect parameter key in groups_get_invites() call.

BP_Invitation::get() accepts an id parameter, but I was passing a misnamed ids parameter to the function in the cache-building routine within bp_get_user_groups(), resulting in all invitations being returned.

This changeset corrects the misnamed key and also skips the fetch-and-cache routine if no uncached items are found.

Props boonebgorges.

Fixes #8149.

[dcavins]

In 12487:

Fix incorrect parameter key in groups_get_invites() call.

BP_Invitation::get() accepts an id parameter, but I was passing a misnamed ids parameter to the function in the cache-building routine within bp_get_user_groups(), resulting in all invitations being returned.

This changeset corrects the misnamed key and also skips the fetch-and-cache routine if no uncached items are found.

Props boonebgorges.

Fixes #8149.