Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#6426 closed enhancement (fixed)

'fields' => 'ids' for activity queries

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 2.4 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch commit
Cc:

Description

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.

Attachments (4)

6426.01.patch (10.9 KB) - added by r-a-y 5 years ago.
6426.02.patch (22.0 KB) - added by r-a-y 5 years ago.
6426.diff (4.3 KB) - added by boonebgorges 5 years ago.
6426.2.diff (8.5 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (18)

#1 @DJPaul
5 years ago

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.

#2 @boonebgorges
5 years ago

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.

#3 @r-a-y
5 years ago

+1.

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

#4 @r-a-y
5 years ago

  • Keywords has-patch added; needs-patch good-first-bug removed

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.

Last edited 5 years ago by r-a-y (previous) (diff)

@r-a-y
5 years ago

#5 @boonebgorges
5 years ago

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.

#6 @r-a-y
5 years ago

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.

#7 @boonebgorges
5 years ago

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?

#8 @r-a-y
5 years ago

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. In cases where you call fields=action, but do not add the other required fields like user_id. Blargh!

Last edited 5 years ago by r-a-y (previous) (diff)

@r-a-y
5 years ago

#9 @r-a-y
5 years ago

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,
    ...
) );

#10 @boonebgorges
5 years ago

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.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


5 years ago

@boonebgorges
5 years ago

@boonebgorges
5 years ago

#12 @r-a-y
5 years ago

  • Keywords commit added

Looks great! Commit at will!

#13 @boonebgorges
5 years ago

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

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.

#14 @boonebgorges
4 years ago

In 11049:

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

This was missed in [10217].

See #6426.

Note: See TracTickets for help on using tickets.