'fields' => 'ids' for activity queries

[boonebgorges]

WP_Query and other core query classes allow you to pass fields=ids in order to get back only a list of matching object IDs. This lets you skip a lot of the extra querying that is normally done to pull up object data, prime caches, and so on.

It would be nice to have something similar for BP activity queries. I just built something that needs to query activity for utility purposes, and all of the extra stuff - priming caches, generating activity strings, and so on - adds a ton of overhead.

[DJPaul]

Sounds good. I would also give the implementer a million bucks if we can consider adding fields=<any indexed column name> because I often make that mistaken assumption.

I assume you are putting your name on this one since you've put it in the 2.4 milestone.

[boonebgorges]

We should be careful about how fields=foo support - it should be implemented in a way that plucks from the cache, rather than modifying the SQL query. But yes.

I put good-first-bug on it because it's a good first bug, but I will do it if no one else does, because I want it.

[r-a-y]

+1.

It's super annoying having to use bp_activity_get(), then having to pluck the 'activities' array in order to grab the IDs.

[r-a-y]

Attached is a first pass at a 'fields' parameter in BP_Activity_Activity::get().

If you pass 'fields=ids', this will bypass pagination parameters and return only activity IDs. I think it makes sense to bypass pagination in this case, but let me know if we shouldn't bypass pagination.

If you pass 'fields=component,type', this will return the following format:

Array                                    
(                                          
    [0] => stdClass Object                 
        (                                  
            [id] => 4                      
            [component] => activity        
            [type] => activity_update      
        )                                  
                                           
    [1] => stdClass Object                 
        (                                  
            [id] => 5                      
            [component] => blogs           
            [type] => new_blog_post        
        )                                  
                                           
    [2] => stdClass Object                 
        (                                  
            [id] => 6                      
            [component] => activity        
            [type] => activity_update      
        )                                  
                                           
)

id is always added regardless. My question is should the activity ID be the array key so it is similar to WP_Query's 'fields=id=>parent':

Array
(
    [4] => stdClass Object
        (
            [component] => activity
            [type] => activity_update
        )

    [5] => stdClass Object
        (
            [component] => blogs
            [type] => new_blog_post
        )

    [6] => stdClass Object
        (
            [component] => activity
            [type] => activity_update
        )

) 

Also if you pass 'fields=component,type', you do not get any activity meta cache fetching, activity comment appending or 'has_more_items' marker.

Let me know if this is important to add back.

[boonebgorges]

Cool! Thanks for working on this, r-a-y.

If you pass 'fields=ids', this will bypass pagination parameters and return only activity IDs. I think it makes sense to bypass pagination in this case, but let me know if we shouldn't bypass pagination.

I can see an argument either way, but I think that we should *not* bypass pagination in the case of fields=ids. If for no other reason than parity with WP_Query.

My question is should the activity ID be the array key

My gut feeling is that we should only do this if we're going to do it for *all* activity queries (and perhaps for all content queries). WP's inconsistency on this point is not something we should try to emulate.

​https://core.trac.wordpress.org/ticket/19866#comment:6

Also if you pass 'fields=component,type', you do not get any activity meta cache fetching, activity comment appending or 'has_more_items' marker.

As a developer, I would expect that I *would* get this stuff, except in the case of fields=ids. There are already independent ways of disabling meta cache fetching and activity comment appending. There's no way to turn off the 'has_more_items' checking, but if we think it's important to be able to disable that too, let's add a separate item for it.

[r-a-y]

I think that we should *not* bypass pagination in the case of fields=ids. If for no other reason than parity with WP_Query.

If we do not bypass pagination, I think we'll need to add back the 'has_more_items' marker, so developers know there might be more items.

If this is the case, should the return array be:

Array(
    [0] => 1,
    ...
    ['has_more_items'] => false
)

Or:

Array(
    ['activities'] => array(
         [0] => 1,
         ...
    )
    ['has_more_items'] => false
)

I haven't done this yet in the updated patch.

As a developer, I would expect that I *would* get this stuff, except in the case of fields=ids.

Gotcha. 02.patch should address this.

The return format for fields=component now reverts to the familiar:

Array(
    ['activities'] => array
       [0] => stdClass Object                 
           (                                  
                [id] => 4                      
                [component] => activity        
                [type] => activity_update      
           )   
    ['has_more_items'] => BOOL
)

Had to add a bunch of isset checks everywhere to avoid notices (particularly the activity action callbacks) since a dev might not be fetching all fields.

[boonebgorges]

If we do not bypass pagination, I think we'll need to add back the 'has_more_items' marker, so developers know there might be more items.

Hm. The purpose of 'has_more_items' was (originally, at least) to support our AJAX-powered "Load More" button. Anyone fetching only ids is surely not using this interface. That being said, maybe we should standardize on this format for all return values, as you suggest:

Array(
    ['activities'] => array(
        [0] => // whatever activity format you asked for
    )
    ['has_more_items'] => BOOL
)

Had to add a bunch of isset checks everywhere to avoid notices (particularly the activity action callbacks) since a dev might not be fetching all fields.

Ugh, yeah, that's clunky, but I think it's the right thing to do.

This approach is looking good to me, but it'd be nice to hear from someone else before applying it here and elsewhere. DJPaul, do you have thoughts about it?

[r-a-y]

Had to add a bunch of isset checks everywhere to avoid notices (particularly the activity action callbacks) since a dev might not be fetching all fields.

I've updated 02.patch to be a bit cleaner.

Instead of all the isset checks, I'm now doing this in BP_Activity_Activity::get():

		// Generate action strings
		if ( empty( $fields ) || in_array( 'action', $fields, true ) ) {
			$activities = BP_Activity_Activity::generate_action_strings( $activities );
		}

If you're not asking for the 'action' field in the fields parameter, we don't need to generate the action strings to begin with :)

Edit: Actually, the isset checks are still needed! Blargh!

[r-a-y]

Hm. The purpose of 'has_more_items' was (originally, at least) to support our AJAX-powered "Load More" button. Anyone fetching only ids is surely not using this interface.

Yeah, I'm not really sold on adding 'has_more_items' when 'fields=ids' is in use either. Was just bringing up as a scenario.

I'd be happy to stick with a plain activity IDs array in this case. But, a dev would need to do the following to fetch all activity IDs:

bp_activity_get( array(
    'fields' => 'ids',
    'per_page' => 0,
    ...
) );

[boonebgorges]

I'd be happy to stick with a plain activity IDs array in this case.

Yeah. I keep trying to make us more consistent with WordPress, but it's hard. WP has get_posts(), which returns the posts array from a WP_Query object. We have a query class that returns a structured array (sorta like WP_Query), but our wrapper bp_activity_get() returns that entire multi-d array, instead of just the activities items. So either we follow WP here and return only the IDs, or we remain internally consistent and return array( 'activities' => array( 1, 2, 3 ) ... ). I guess I lean toward the latter, and maybe at some point in the future we can provide a convenience function that acts more like get_posts() in WP.

But, a dev would need to do the following to fetch all activity IDs

This seems like expected behavior. fields is about the format of the return values. It shouldn't be a filter on the records returned.

[r-a-y]

Looks great! Commit at will!

[boonebgorges]

In 10217:

Introduce fields parameter to bp_has_activities() stack.

fields=ids will fetch only the IDs of matched activities. fields=all
(or any other value of fields, for the moment) will return full activity
objects.

The structure of the array returned from BP_Activity_Activity::get() is the
same regardless of the value of fields: matched items are stored in the
'activities' member of the returned array.

Props r-a-y.
Fixes #6426.

[boonebgorges]

In 11049:

Ensure that the 'fields' parameter is passed through bp_activity_get().

This was missed in [10217].

See #6426.