bp_has_activities to accept multi scope

[henrywright]

It would be nice to have both personal and friend's activities in same activity stream (similar to how Facebook's feed works). This isn't currently possible.

Currently, you can pass bp_has_activities just one of the following arguments

just-me, friends, groups, favorites, mentions

[trishasalas]

Is this something I could implement with the work I'm doing for custom post types? Possibly switch from the current drop-down to checkboxes? (asking as I'm not as familiar as some are with how core works.)

[boonebgorges]

It's a nice thought, though I'm not sure if it'd be possible to make this work without refactoring the entire activity query class. Moving to Future Release for discussion.

trishasalas - This is really an issue with how the underlying activity queries work, more than how the filters are represented on the front end.

[boonebgorges]

Marked #3201 as duplicate.

[r-a-y]

01.patch supports multiple scopes.

The patch does a few things:

• Adds 'scope' as an actual parameter in BP_Activity_Activity::get(). Before, 'scope' was only used in bp_has_activities().
• Moves the hardcoded scope code from bp_has_activities() to the new BP_Activity_Activity::get_scope_query_sql() method. Component scopes are now also handled in their respective components instead of inside the activity component.
• Adds a new BP_Activity_Query class to handle multiple relation clauses. (Think of this as a simplified version of WP_Meta_Query.)
  • Adds a new 'filter_query' parameter in BP_Activity_Activity::get(), which uses this.
  • BP_Activity_Activity::get_scope_query_sql() also uses this.

Passes existing unit tests. Also added a unit test to show how multiple scopes will work.

There are some things in get_scope_query_sql() that needs some feedback. You'll notice an 'override' array. This is necessary because scopes need to be able to supercede some existing activity arguments (eg. user_id=0 for friends and groups scopes). Read the inline comments for more info.

Feedback appreciated.

[boonebgorges]

r-a-y - A really nice start. The technique you've chosen for translating 'scope' into a filter_query looks good at a glance. I definitely like that this is now an option available through the whole stack, not just at the level of bp_has_activities()

A couple thoughts about how the proposed API syntax looks. Some of this is in contradiction to my initial thoughts yesterday, so bear with me :)

1. There's currently no way to do negative filters. If I want to get all activity from components *other than* 'groups', there should be a way to do that.
2. WP_Meta_Query and WP_Tax_Query are limited in that they only support one level of 'relation' - no nesting. While nested queries seem arcane, I'd argue that they inevitably arise in practice. Say you want activity items that meet the following criteria:

( 
  ( item_id = 5 )
  AND
  ( component = 'blogs' OR type = 'activity_update' )
)
OR
(
  ( component = 'groups' )
  AND
  ( item_id = 6 )
)

I wonder if we can approach this in a different way that fixes both problems. Two changes:

1. Instead of multiple columns into a single filter, one column per filter clause. Make it look pretty much just like WP_Meta_Query (though we only have to support a smaller subset of 'compare' operators):

array(
    'column' => 'component', // 'column' is a crappy name, but you get the idea
    'value' => array( 'groups', 'blogs' ),
    'compare' => 'IN',
),

2. Support infinite nesting. All grouping happens with AND or OR relations.

A "my friends" filter is then:

$my_friends_filter_query = array(
    array(
        'column' => 'user_id',
        'value' => $friend_ids,
        'relation' => 'IN',
    ),
),

A "my groups" query is:

$my_groups_filter_query => array(
    'relation' => 'AND',
    array(
        'column' => 'component',
        'value' => 'groups',
        'relation' => 'IN',
    ),
    array(
        'column' => 'item_id',
        'value' => $group_ids,
        'relation' => 'IN', 
    ),
),

scope=groups,friends would be:

array(
    'relation' => 'OR',
    $my_groups_filter_query,
    $my_friends_filter_query,
),

This'll let you get as fancy as you want.

Technically, it'll require some recursion, and it'll require a technique for knowing whether another level of recursion is required. (Looking for a 'column' array key is probably enough.)

What do you think?

[r-a-y]

A couple thoughts about how the proposed API syntax looks. Some of this is in contradiction to my initial thoughts yesterday, so bear with me :)

I kinda like the current syntax as it's easy to write scopes for. We also have to keep in mind that whatever syntax we decide will be used by plugin authors.

---

( 
  ( item_id = 5 )
  AND
  ( component = 'blogs' OR type = 'activity_update' )
)
OR
(
  ( component = 'groups' )
  AND
  ( item_id = 6 )
)

This is probably the worse example for endorsing nested relation queries ;) With that being said, here's how to translate that example into a BP_Activity_Query in the latest patch:

$activity = new BP_Activity_Query( array(
	'relation' => 'OR',

	// nested relation query
	array(
		//'relation' => 'AND', // this can be omitted as the default relation is 'AND'
		'item_id' => array( 5 ),
		array(
			'component' => 'blogs',
			'type'      => 'activity_update',
			'relation'  => 'OR',
		),
	),

	// groups
	array(
	    'component' => array( 'groups' ),
	    'item_id'   => array( 6 ),
	)
) );

---

There's currently no way to do negative filters. If I want to get all activity from components *other than* 'groups', there should be a way to do that.

Updated patch supports this. Here's an example:

$activity = new BP_Activity_Query( array(
	array(
		'compare'   => 'NOT IN',
		'component' => 'groups',
	)
) );

---

What are some other 'compare' types we want to support? I still have to do some further tweaking, but what do you think about diverging from the WP_Meta_Query syntax?

[r-a-y]

Updated patch to support nested relation queries and negative filters.

[boonebgorges]

This is probably the worse example for endorsing nested relation queries ;)

Yes, it was a very terrible example. My brain was too tired to think of more realistic ones.

I kinda like the current syntax as it's easy to write scopes for. We also have to keep in mind that whatever syntax we decide will be used by plugin authors.

Yes. Aesthetically I prefer the 'component' => array( 'groups' ) etc syntax. However, the introduction of 'compare' alongside 'relation' in the same subquery shows how the syntax is limiting. Your example above for using 'compare' kinda assumes (at least semantically - and, at a glance, technically, though I'd need further testing) that the current clause only contains a single filter. Consider the following:

array(
    'compare' => 'NOT IN',
    'component' => 'groups',
    'item_id' => array( 4, 10 ),
),

What does this do? Does 'NOT IN' apply to *both* clauses? What if I only want it to apply to one? In some cases, I might be able to work around this by breaking it out into two separate clauses:

array(
    'relation' => 'AND',
    array(
        'compare' => 'NOT IN',
        'component' => 'groups',
    ),
    array(
        'compare' => 'IN',
        'item_id' => array( 4, 10 ),
    ),
),

But this might not work in all cases, because of the various kinds of groupings that might be required. (Do we want to require plugin authors to do De Morgan's transformations in order to fit their meaning into our syntax?)

Another way to put the general point: 'relation' talks about the relationship between siblings in the clause, while 'compare' talks about the internal structure of a sibling within the clause. Permitting them both on the same level of subquery makes the syntax a bit more terse, but also introduces the opportunity for confusion. (This confusion does not exist in my recursive suggestion: a 'compare' param can only appear in a lowest-level subquery, while a 'relation' only works to relate subqueries.)

So, (a) does the above make sense?; (b) Am I right? (it's confusing!); (c) Do we want to sacrifice some expressiveness and clarity in order to make the syntax a bit simpler (less nesting required)? Regarding (c), I'm kinda leaning toward the following answer: let's sketch up what our *core* queries would look like on each syntax (you've already done it for your syntax, I'd just have to do something comparable for what I'm suggesting). If it turns out that core queries require several levels of confusing nesting, it's likely to be even worse for plugin authors building complex stuff, and we should lean toward the simpler syntax (sacrificing some expressiveness). If it turns out our core queries are fairly easy to understand, let's go with the more general and flexible syntax.

What do you think?

What are some other 'compare' types we want to support? I still have to do some further tweaking, but what do you think about diverging from the WP_Meta_Query syntax?

Honestly, for now, I think we stick with IN and NOT IN. Once we're settled on the API format, let's get complete test coverage, and then worry about what others we might want to support. (My gut says: arithmetic operators (<, <=, >=, >, BETWEEN, NOT BETWEEN), but not LIKE/REGEXP.)

[boonebgorges]

Following up on the comments above. In WP 4.1 we will be introducing recursive, nested query syntax for WP_Meta_Query, WP_Tax_Query, and WP_Date_Query; see eg ​https://core.trac.wordpress.org/ticket/29642. This is an argument in favor of supporting the same syntax here, though I think we should still talk about how much is to be gained by sticking close to the syntax of WP's query classes.

[r-a-y]

This is an argument in favor of supporting the same syntax here, though I think we should still talk about how much is to be gained by sticking close to the syntax of WP's query classes.

Let's stick to the proposed core syntax since you will be making enhancements in those areas in WP and to BP XProfile. Doesn't make sense to have a different syntax solely for activity queries! :)

[r-a-y]

02.patch switches the syntax to use the same recursive, nested query syntax as introduced by boonebgorges to WP and to BP_XProfile_Query. (See comment:7.)

I've also opted to use the approach by boonebgorges to separate the recursive query syntax into its own class - BP_Recursive_Query - though this is not in WP core. (See ​https://core.trac.wordpress.org/ticket/29642#comment:6 for more details.) BP_Activity_Query extends this class; at the moment, BP_XProfile_Query doesn't extend this class, though my hope is that it eventually will.

Supports the same compare operators as BP_XProfile_Query except 'EXISTS' and 'NOT EXISTS' because those operators rely on JOINs and I don't really think we need them. If we do need 'EXISTS' / 'NOT EXISTS', I'll do up another patch.

Still need a bit of feedback from comment:4 regarding the 'override' approach that I've chosen to adopt. I might need to write a few more unit tests that shows off the problem. But, I think we're in a better spot than before.

[boonebgorges]

In 9213:

Don't use extract() in BP_Activity_Activity::get().

Props r-a-y.
See #5698, #4988.

[boonebgorges]

Thanks for your work on this, r-a-y! I think it's headed in a great direction. Some thoughts as I read over the patch:

• I committed the extract() stuff in [9213] just to get it out of the way
• The override technique here looks fine. If we think of 'scope' as a shorthand way of signifying certain groups of arguments, then it makes sense that we'd force arguments in these cases.
• We should probably accept an array as well as comma-separated values for 'scope'.
• Regarding the 'hide_sitewide' override. Doing this at the level of the SQL clause construction seems precarious to me, especially when checking bp_loggedin_user_id(). This makes it hard to, say, do certain kinds of queries programatically. Is it possible to move that logic up a level, like maybe into bp_has_activities() or something like that? Also worth noting: hide_sitewide should be true when inside of a group for members of that group (or private group activity will never appear).
• EXISTS and NOT EXISTS don't seem that important to me. Let's leave them out, and we can look at adding them in the future if people need them.
• Regarding 'type', we only really need to support this if we're going to allow for numerical comparisons. Like, I guess, "show me the activities associated with users whose IDs are between 10 and 15". Pretty unlikely. (Date comparisons are excluded from what you're doing here, and are covered by date_query.)
• Very small point: let's not trim 'value'. I think WP_Meta_Query etc do this only for legacy reasons, but IMO it makes no sense. People should be able to store and compare whatever values they want.

4988.03.patch​ is the same as 02.patch, minus the extract() stuff.

[r-a-y]

04.patch addresses most of the points raised by boonebgorges in comment:18.

The most important being the hide_sitewide call is moved back into bp_has_activities() (see $show_hidden variable in the patch) so the behavior is like before this patch was made.

I could probably write more unit tests for multiple scopes and hide_sitewide, but I think we're at a stage where we can commit and iterate.

[boonebgorges]

I think we're at a stage where we can commit and iterate.

Agreed.

[r-a-y]

In 9256:

Activity: Introduce new class - BP_Activity_Query.

BP_Activity_Query is a helper class, which extends the
BP_Recursive_Query class, for generating complex queries against data in
the activity database table.

A 'filter_query' argument has been introduced to the bp_has_activities()
stack to support the use of BP_Activity_Query across the majority of
activity fetching functions.

See #4988.

[r-a-y]

In 9257:

Activity: Support multiple scopes in bp_has_activities().

This commit:

• Allows the 'scope' parameter to support multiple scopes. For example, scope=friends,mentions. This can be a comma-delimited string or an array.
• Makes the 'scope' parameter available across the bp_has_activities() stack. Previously, 'scope' was only available for use in the bp_has_activities() loop.
• Parses scopes using the BP_Activity_Query class to handle complex conditions.
• Allows components to declare their own custom activity scopes. Components can also override existing activity arguments by setting the 'override' key in their scope callback. For an example, see how the friends component declares the 'friends' scope in bp_friends_filter_activity_scope(). Previously, it was not possible for components or third-party plugins to declare their own scopes without doing a fair bit of workarounds.

See #4988.

[r-a-y]

In 9258:

Add unit tests for 'scope' and 'filter_query' arguments for bp_has_activities().

See #4988.

[r-a-y]

array_replace_recursive.patch is to fix the unit tests failing on PHP 5.2:
​https://travis-ci.org/buddypress/BuddyPress/builds/44875361

Tested on PHPUnit 3.6.12 with PHP 5.2.17.

[boonebgorges]

Replying to r-a-y:

array_replace_recursive.patch is to fix the unit tests failing on PHP 5.2:
​https://travis-ci.org/buddypress/BuddyPress/builds/44875361

Tested on PHPUnit 3.6.12 with PHP 5.2.17.

Looks good to me.

[r-a-y]

In 9263:

Add a PHP-agnostic version of array_replace_recursive() in BP_Activity_Activity

r9257 implemented the ability to query activity items by multiple scopes.
array_replace_recursive() is a function that is used in the above commit
to help override certain arguments in the activity loop by scopes. However, array_replace_recursive() is a PHP 5.3 function and BuddyPress (and
WordPress) currently supports a minimum version of PHP 5.2.

This commit adds a PHP-agnostic version of array_replace_recursive() for
internal use by the BP_Activity_Activity class. This helps to avoid
fatal errors for those using PHP 5.2 and to fix up our unit tests.

Anti-props r-a-y.

See #4988.

[r-a-y]

In 9264:

Activity: Fix wrong variable from r9257.

Ugh!

See #4988.

[DJPaul]

Is anymore planned to happen with this for 2.2? If not, can someone make a new ticket detailing the next steps, and close this one so we have it assigned with the milestone.

[r-a-y]

I think we're good now! Will open new tickets against the 2.2 milestone should issues arise.

Will probably also write up some docs for the codex.

Thanks everyone!

[imath]

In 9325:

Move activity scopes adjustment in filters

Separate the hardcoded scopes away from BP_Activity_Activity::get_scope_query_sql() to manage scope adjustments within components and to allow developers to make adjustments to them if needed.

See #4988
See #6040

props r-a-y