Skip to:
Content

Opened 4 years ago

Last modified 14 months ago

#6059 new defect (bug)

BP_Activity_Activity:get() doesn't respect 'max' parameter

Reported by: dcavins 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)

#1 @boonebgorges
4 years ago

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.

#2 @dcavins
4 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 @boonebgorges
4 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 @dcavins
4 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 @boonebgorges
4 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!

#6 @DJPaul
4 years ago

  • Milestone changed from Awaiting Review to 2.2

#7 @DJPaul
4 years ago

  • Milestone changed from 2.2 to Future Release

#8 @tw2113
14 months ago

@dcavins did you ever do any changes for the docblocks? Curious what ones need it, which may need it still, and which are already done.

#9 @dcavins
14 months ago

@tw2113 I don't believe I made any changes. :( I think the comments that really need to be updated are those that only change the reported "found items" count, but don't actually limit anything.

Note: See TracTickets for help on using tickets.