`BP_Activity_Activity::get()` args should be translated into `BP_Activity_Query`

[boonebgorges]

BP_Activity_Query is extremely flexible and powerful. But the ability to leverage it from a plugin is limited, because of the way that BP_Activity_Activity::get() parses the arguments it's passed. The existing logic looks something like this in pseudo-code:

if ( $scope ) {
    $where[] = WHERE clauses corresponding to 'scope'
} elseif ( $filter_array ) {
    $where[] = WHERE clauses corresponding to 'filter_query'
}

foreach ( array( 'filter', 'spam', 'search_terms', 'show_hidden', 'include', 'exclude' ) as $arg ) {
    if ( $r[ $arg ] ) {
         $where[] = WHERE clauses corresponding to $arg
    }
}

$where = implode( ' AND ', $where );

In other words:

• 'scope' and 'filter_query' are mutually exclusive
• all of the various clauses are joined with 'AND'

This is pretty limiting. It means that there's no single filter point where a plugin can interject to add a condition. It also means that the conditions are necessarily joined by AND, when someone might reasonably want to get, say, items that are 'hide_sitewide=0' OR 'user_id=123'.

I recommend that the logic be reconstructed so that everything is run through BP_Activity_Query - ideally, a single set of clauses. It'd look something like this:

if ( $scope ) {
    $scope_clauses = translate 'scope' into BP_Activity_Query clauses, much like `get_scope_query_sql()` already does
}

// No longer mutually exclusive with $scope
if ( $filter_query ) {
    $filter_query_clauses = $filter_query;
}

foreach ( $other_relevant_args ) {
     $clause_registry[ $arg ] = list of clauses for $arg
}

// Put them all together, assuming `AND`
$query_args = array_merge(
    array( 'relation' => 'AND' ),
    all the other sets of query clauses
);

// Run it through a single filter, either here or after running `BP_Activity_Query`
$query_args = apply_filters( ... );

// Then get the sql
$aq = new BP_Activity_Query( $query_args );
$where = $aq->get_sql();

Now, any plugin - or even BuddyPress itself - has the power to perform any modifications on any kind of activity filtering. To take just one example, this would allow a privacy plugin to exclude activity matching a set of criteria from every stream for a given user.

@r-a-y - I'm especially interested in your thoughts here, since you designed a bunch of this (and especially because you wrote the logic to translate 'scope' into a BP_Activity_Query).

[r-a-y]

Thanks for opening this ticket, boonebgorges.

I think for 2.4.0 we can, at the very least, allow 'filter_query' to become independent and not tied to 'scope' being available. This was an oversight on my part when I originally introduced this into the BP_Activity_Activity:get() method.

tl;dir: Making the 'filter_query' parameter independent should allow you to do most of the complex filtering without needing to convert our older WHERE conditions over to BP_Activity_Query for the most part.

---

Long reply:

As for translating our older activity WHERE conditions to funnel through BP_Activity_Query, this sounded like a decent idea. I was thinking about doing this originally in v2.2.0, but thought the change might be too drastic and kind of hard due to the 'bp_activity_where_conditions' filter.

The 'bp_activity_where_conditions' filter is an array of where conditions that later generates the SQL statement by joining the array with the 'AND' relation.

By running everything through BP_Activity_Query to generate the SQL statement, this will potentially break how existing plugins are using that filter.

The other issue about running everything through BP_Activity_Query is how a plugin dev will filter complex activity queries.

For example, here's a small example without translating the existing WHERE conditions over:

$query_args = array(
	'relation' => 'AND',

	// Get blog activity items or groupblog posts
	array(
		'relation' => 'OR',

		// general blog activity items
		array(
			'relation' => 'AND',
			array(
				'column' => 'component',
				'value'  => buddypress()->blogs->id
			),
			array(
				'column'  => 'item_id',
				'compare' => 'IN',
				'value'   => (array) $blog_ids
			),
		),

		// groupblog posts
		array(
			'relation' => 'AND',
			array(
				'column' => 'component',
				'value'  => buddypress()->groups->id
			),
			array(
				'column'  => 'item_id',
				'compare' => 'IN',
				'value'   => (array) $group_ids
			),
			array(
				'column'  => 'type',
				'value'   => 'new_groupblog_post'
			),
		),
	),

	// Make sure items are public
	array(
		'column' => 'hide_sitewide',
		'value'  => 0
	)
);

In order to filter the query args, a dev would have to loop (or recursively loop) through each item in the array to analyze what is being queried and to remove or add something. (Note: This is currently what a dev would need to do to filter activity scope arguments.)

Now add on the converted WHERE condition query args and that means more args and logic to loop through.

This kind of shows off the inelegance of advanced activity filtering through a plugin.

Before it even gets to this stage, I think arguments should be filtered through 'bp_after_has_activities_parse_args' to remove the complexity of the query args.

---

I might be overlooking what you are proposing so if I am misunderstanding something, please let me know!

[boonebgorges]

I might be overlooking what you are proposing so if I am misunderstanding something, please let me know!

No, this is totally right. I thought about the SQL filter, but I don't really care much about that. (If you are filtering a SQL statement, and doing it in a way that isn't resilient, then you shouldn't be filtering a SQL statement.)

The complexity issue is more pertinent. Even something simple like hide_sitewide=false becomes

array(
    'column' => 'hide_sitewide',
    'value' => 1,
)

I definitely think your patch is an improvement, though it still requires filtering where_conditions to drop the 'scope' clause if you want filter_array to work in an unfettered way. I guess I'd argue that, while it may be overkill to convert *all* params to BP_Activity_Query clauses, it might make sense to do it in the case of the 'scope' clause. (Plus, it's already happening internally, we're just not exposing it - we return the SQL fragment.)

Before it even gets to this stage, I think arguments should be filtered through 'bp_after_has_activities_parse_args' to remove the complexity of the query args.

Yes, absolutely.

And actually, here's something I just thought of, which would make the bp_parse_args() filter and the scope/filter_array separation even more useful. What if scope were translated into a filter_query somewhere further up the stack - say, in bp_activity_get() or bp_has_activities()? This way, the bp_parse_args() filter in BP_Activity_Activity::get() would, in most cases, receive a more robust and explicit set of arguments, making it easier for devs to figure out what needs to be changed if they want to tweak the logic. (We'd continue to accept 'scope' in BP_Activity_Activity::get(), but we'd never pass the param down that far in core.)

I guess this is all coming from a strong dislike for 'scope'. It's a black box that hides a lot of complexity and assumptions. Unraveling them systematically so that they can be modified in a filter often means reverse-engineering what the various values of 'scope' are intended to do. 'user_id', 'item_id', 'hide_sitewide', and even 'filter_query' can easily be parsed programatically. 'scope' is opaque, and depends on context. I'd be happy if it were never passed to a low-level function or filter.

I won't beat the drum for much longer :) If @r-a-y and others think it's a good idea not to touch the flow of 'scope', that's OK with me. But I will say this: every time I have to deal with it in a plugin, I am confused. And if I'm getting confused, there's not much hope for most people trying to modify activity queries in a systematic way.

@r-a-y - I'm happy to defer to your wisdom here.

[r-a-y]

I definitely think your patch is an improvement, though it still requires filtering where_conditions to drop the 'scope' clause if you want filter_array to work in an unfettered way.

You can also drop 'scope' further up in the 'bp_after_has_activities_parse_args' filter without the patch to accomplish what you need. Though, I see what you mean.

What if scope were translated into a filter_query somewhere further up the stack - say, in bp_activity_get() or bp_has_activities()?

Now, there's a thought! The only problem is a scope needs to override the various default activity parameters. In order to move this up the stack, we might need to change a few parameters since our parameters are not named the same across the template/get functions and class.

I guess this is all coming from a strong dislike for 'scope'. It's a black box that hides a lot of complexity and assumptions. Unraveling them systematically so that they can be modified in a filter often means reverse-engineering what the various values of 'scope' are intended to do. 'user_id', 'item_id', 'hide_sitewide', and even 'filter_query' can easily be parsed programatically. 'scope' is opaque, and depends on context. I'd be happy if it were never passed to a low-level function or filter.

Yeah, I hear ya. When it comes to plugins needing to modify the scope, it is painful. Having needing to use these filters myself, you're not the only one experiencing these growing pains!

Conversely, 'filter_query' is just as painful in my eyes. With the scope filters, at least you can pinpoint what might need to be changed in a given scope.

I won't beat the drum for much longer :)

Please continue in the spirit of Todd Rundgren :) I think this discussion is useful and I do think converting 'scope' back to 'filter_query' is an approach worth exploring. If there's time in 2.4, let's strive for it, but 2.5 might be better.

[DJPaul]

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see ​https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: ​https://make.wordpress.org/chat/