Opened 10 years ago
Last modified 8 years ago
#6059 new defect (bug)
BP_Activity_Activity:get() doesn't respect 'max' parameter
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Awaiting Contributions | Priority: | normal |
Severity: | minor | Version: | |
Component: | Activity | Keywords: | |
Cc: | david.cavins@… |
Description
BP_Activity_Activity:get()
accepts three SQL limit parameters: $page
, $per_page
and $max
. $page
and $per_page
are used to generate pagination SQL, but $max
is only used to modify the found-row count.
It seems that at the least we'd have to decide that $page
and $per_page
trump $max
or vice-versa. Both can't be used in the same request.
One minimal option would be to set $page = 1
and $per_page = $max
if $args['max']
is set (and $args['per_page']
is not set) at about line 473 and leave the pagination logic in place.
Source:
https://buddypress.trac.wordpress.org/browser/trunk/src/bp-activity/bp-activity-classes.php#L296
I'll happily produce a patch if we can decide on the desired behavior.
Change History (9)
#2
@
10 years ago
- Cc david.cavins@… added
Here are some examples of how "max" is currently used:
Activity
bp_has_activities()
accepts a max and plugs it in for per_page sometimes
// Do not exceed the maximum per page if ( !empty( $max ) && ( (int) $per_page > (int) $max ) ) $per_page = $max;
BP_Activity_Activity:get()
uses max to modify the found-items count only.
Blogs
bp_has_blogs()
allows max to override per_page in some cases.
// Set per_page to maximum if max is enforced if ( ! empty( $r['max'] ) && ( (int) $r['per_page'] > (int) $r['max'] ) ) { $r['per_page'] = (int) $r['max']; }
BP_Blogs_Blog::get()
, search_blogs()
don't have a 'max' parameter
* @param int|bool $limit Optional. The maximum records to return. * Default: false. * @param int|bool $page Optional. The page of records to return. * Default: false (unlimited results).
$limit here is used like 'per_page':
$pag_sql = ( $limit && $page ) ? $wpdb->prepare( " LIMIT %d, %d", intval( ( $page - 1 ) * $limit), intval( $limit ) ) : '';
Friends, Members
bp_has_members()
accepts a max and uses it like bp_has_blogs()
(the comment is incorrect):
// Set per_page to max if max is larger than per_page if ( !empty( $r['max'] ) && ( $r['per_page'] > $r['max'] ) ) { $r['per_page'] = $r['max']; }
BP_Friends_Friendship::search_friends()
: 'Limit' is used as per_page.
BP_User_Query
doesn't accept max. It accepts page and per_page.
Groups
bp_has_groups()
accepts max and passes the unmodified value to BP_Groups_Template
.
BP_Groups_Template::__construct
accepts 'max' and uses it only to modify the found-group count.
BP_Groups_Group::filter_user_groups()
, search_groups()
: 'Limit' is used as per_page.
BP_Groups_Group::get()
doesn't accept a max. Uses 'per-page' and 'page'.
* @type int $per_page Optional. Number of items to return per page * of results. Default: null (no limit). * @type int $page Optional. Page offset of results to return. * Default: null (no limit).
BP_Groups_Membership_Requests_Template
uses 'max' only to modify the found-group count.
Messages
bp_has_message_threads()
accepts max and passes the unmodified value to BP_Messages_Box_Template
.
BP_Messages_Box_Template
accepts a 'max' which is used to modify the found-messages count only.
Notifications
bp_has_notifications()
accepts 'max' but adds this brutally honest note:
// these are additional arguments that are not available in // BP_Notifications_Notification::get() 'max' => false,
BP_Notifications_Template
accepts a 'max' which is used to modify the found-messages count only.
I dug around in the logs, but couldn't get a handle on any big thoughts. It was interesting to go through a bunch of commits found by git pickaxe, though. It seems like the more recently remodeled code doesn't use 'max' at all--BP_User_Query
and bp_has_notifications()
for instance.
In my case, I was hoping to use the max arg with bp_activity_get()
to find certain activity items to exclude from bp_legacy_theme_ajax_querystring
. But I didn't need every result, just the first hundred or so out of the possible tens of thousands. So "max" makes sense to me in a utility function like bp_activity_get()
, but less so in bp_has_activities()
which is meant to be used as a loop.
#3
@
10 years ago
Thanks for doing this research, dcavins, though I wish I could say it shed much light on the situation :)
I don't have a strong feeling about how to handle all this. My first inclination would be to leave it pretty much as-is, and encourage people to use per_page
in the official documentation. If we can think of a relatively consistent way to define the concept of 'max' such that it doesn't completely duplicate 'per_page', that's fine - but we should be extremely careful about breaking backward compatibility, unless there's a good reason to do so.
So, what do you think we should do about this mess?
#4
@
10 years ago
Ha ha. What to do indeed.
Well, I'm not sure it's not worth doing a whole bunch of work on it. Ideally, we could deprecate the argument where it's useless (bp_has_notifications()
) or does something that just seems weird (like modifying the found-items count only--I can't imagine why you'd want to change the found-items count but not change the set of found items).
Adding code comments where appropriate would go a long way. For instance, the docblock for BP_Activity_Activity::get()
lies:
* @type int|bool $max Maximum number of results to return. * Default: false (unlimited).
I can update the docblocks that need updating, if that seems useful to you.
Can we not use 'max' in future components? :)
#5
@
10 years ago
I can update the docblocks that need updating, if that seems useful to you.
Yes, I'd be delighted to get a documentation patch that clears up this mess. Where relevant, the documentation could include language to the effect that you should use 'per_page' instead.
Can we not use 'max' in future components? :)
Wish granted!
I've always found 'max' to be confusing and unclear. I guess I interpreted it as: if 'max' is set, then return this number of results max, regardless of 'per_page'. Then there would be a use case for using this alongside of 'page'/'per_page'. Let's say you wanted the first 3 results, but starting with item 21. You could use 'page=3&per_page=10&max=3'. That being said, this is me trying to rationally reconstruct the purpose of a mysterious parameter.
dcavins, before moving forward with anything, I'd appreciate if you could look at other places in BP for the use of a 'max' param, and if necessary, in the logs, to try to get a better sense of what 'max' is/was intended to do. I want to make sure we don't break a huge number of things by changing the behavior here.