Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#6352 closed enhancement (fixed)

Add parameter to differentiate filter values with bp_core_get_specific_users_count_sql

Reported by: tw2113's profile tw2113 Owned by: boonebgorges's profile boonebgorges
Milestone: 2.3 Priority: normal
Severity: normal Version: 1.2
Component: Core Keywords: has-patch
Cc:

Description

Let's add a string parameter to bp_core_get_specific_users_count_sql to help differentiate which SQL statement is being passed in.

Attachments (1)

class-bp-core-user-6352.diff (1.3 KB) - added by tw2113 10 years ago.

Download all attachments as: .zip

Change History (8)

#1 @DJPaul
10 years ago

  • Milestone changed from Awaiting Review to 2.3
  • Version changed from 2.2.1 to 1.2

I think just changing one or both of the filter names would be best. If we changed both, we could still run the results through the original filter name for backpat.

#2 @boonebgorges
10 years ago

Aesthetically I like the simplicity of tw2113's suggestion, but yeah, we don't do this anywhere else. We need to introduce two new filters here.

#3 @boonebgorges
10 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 9699:

Introduce more specific filter names in BP_Core_User::get_specific_users().

Previously, the same filter name was used for both the 'paged' and the 'total'
SQL strings, making the filter difficult to use.

Props tw2113.
Fixes #6352.

#4 @johnjamesjacoby
10 years ago

Curious what the difference is between passing a string as a parameter, or passing a variable? Obviously we pass tons of variables through filters hundreds of times per page load.

The patched solution maintains backwards compatibility with existing installations, using the filter API to allow manual forking of the filter logic in one place.

What does BuddyPress do in other places where these two types of filters are present?

#5 @boonebgorges
10 years ago

We usually have separate filter names for these sorts of things. See eg https://buddypress.trac.wordpress.org/browser/trunk/src/bp-core/classes/class-bp-core-user.php?marks=387,419#L379. I can't think of anywhere else where we use the same filter for paged/count SQL strings and differentiate in the filter with a 'type' flag of this sort. I'm assuming that tw2113 originally suggested this technique because I'd suggested it as a possibility in a Slack discussion we'd had.

Small argument in favor of different filter names: it keeps the callback signature closer to that of the parent method. function my_get_specific_users_cb( $sql, $user_ids, $limit, $page, $populate_extras ), instead of offsetting with a $type param.

That said, if people prefer the single filter name with a $type param, I don't care too much. Feel free to revert [9699] with something closer to what tw2113 originally suggested.

#6 @johnjamesjacoby
10 years ago

I'm okay with either approach. r9699 is fine as is IMO.

#7 @tw2113
10 years ago

Either works for me, I'm not picky. :)

Note: See TracTickets for help on using tickets.