Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#7205 closed enhancement (fixed)

Adds 'orderby' support to Activity queries

Reported by: Mamaduka Owned by: tw2113
Milestone: 2.9 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch commit
Cc:

Description

Currently BP_Activity_Activity::get() doesn't support 'orderby' query arg. ORDER BY is hardcoded to 'date_recorded'.

It would be nice to introduce something similar that WP core query classes are doing, since 4.2 - https://make.wordpress.org/core/2015/03/30/query-improvements-in-wp-4-2-orderby-and-meta_query/.

Attachments (3)

7205-custom-orderby.diff (4.0 KB) - added by tw2113 2 years ago.
7205-custom-orderby-1.diff (4.1 KB) - added by tw2113 2 years ago.
7205-custom-orderby-2.diff (5.1 KB) - added by tw2113 2 years ago.

Download all attachments as: .zip

Change History (13)

#1 @DJPaul
3 years ago

  • Keywords 2nd-opinion needs-testing removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

#2 @tw2113
2 years ago

  • Keywords has-patch added; needs-patch removed

Definitely possible. Something to keep in mind is that we current order by 2 items. date_recorded column and then activity ID. We should probably leave activity ID as the 2nd order by.

First up would be providing a parsed value for the method. I feel we should have it default to what we have now. Since it's never been changeable before, I'm unsure there's much back compat to break.

Second part, that I think we should do, is only allow values from the current columns through. So we'd need to do some sort of comparison or array value checks from our allowed columns. Perhaps fall back to date_recorded if the user-provided value isn't valid.

See attachment for consideration.

#3 @tw2113
2 years ago

  • Milestone changed from Future Release to 2.9

#4 @r-a-y
2 years ago

Patch looks good, @tw2113!

Instead of the array_key_exists() check, maybe switch() would be better?

<?php

switch ( $r['order_by'] ) {
    case 'id' :
    case 'user_id' :
    case 'component' :
    case 'all the other DB columns!':
        break;

    default :
        $r['order_by'] = 'date_recorded';
        break;
}

#5 @tw2113
2 years ago

Amended to use a switch statement and decided to add all the columns, since it could be feasible that someone wanted to order by hide_sitewide or by spam status. Originally I left the last 4 off.

#6 @r-a-y
2 years ago

@tw2113 - Looks good. One last thing is to add a @since tag to the get() method and I think we can commit this one.

#7 @tw2113
2 years ago

Touched up the phpdocs for the parameter.

#8 @r-a-y
2 years ago

  • Keywords commit added

Let's get this in.

Thanks @tw2113!

#9 @tw2113
2 years ago

  • Owner set to tw2113
  • Status changed from new to assigned

#10 @tw2113
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 11580:

Adds support for customizing the orderby parameer for BP_Activity_Acitivty::get() method.

Fixes #7205.

Last edited 2 years ago by tw2113 (previous) (diff)
Note: See TracTickets for help on using tickets.