Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

#7238 closed enhancement (fixed)

Change bp_sort_by_key() to allow key preservation.

Reported by: dcavins Owned by: dcavins
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.2
Component: Core Keywords: has-patch
Cc: dcavins

Description

bp_sort_by_key() internally uses usort() which discards keys once the sort is completed. I'd like to make it possible to keep the keys when helpful.

In the attached patch, I've added a new parameter to bp_sort_by_key() that runs the results through uasort() instead of usort().

Attachments (1)

7238.1.patch (3.3 KB) - added by dcavins 3 years ago.
Add $preserve_keys parameter to bp_sort_by_key().

Download all attachments as: .zip

Change History (10)

@dcavins
3 years ago

Add $preserve_keys parameter to bp_sort_by_key().

#1 @dcavins
3 years ago

  • Owner set to dcavins
  • Status changed from new to accepted

There's a related convenience function bp_alpha_sort_by_key() that I did not update to keep keys. It would be simple to do, but do we want to?

https://buddypress.trac.wordpress.org/browser/trunk/src/bp-core/bp-core-functions.php#L104

#2 @r-a-y
3 years ago

Patch looks nice and simple to me! +1!

#3 @r-a-y
3 years ago

  • Keywords commit added

#4 @DJPaul
3 years ago

what problems would just changing the current implementation cause, rather than adding a new parameter?

#5 @dcavins
3 years ago

I'm not sure it would cause any (I just searched the repo for uses, which are not too numerous), but I'm not going to make that call. :)

It doesn't appear to throw a notice if applied to a non-associative array, so at least it's not a no-go.

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


3 years ago

#7 @dcavins
3 years ago

Well, this is interesting. Simply changing usort() out for uasort() seems to work OK for our uses of bp_sort_by_key(), but we've got a test that sorts an object that breaks:
https://buddypress.trac.wordpress.org/browser/trunk/tests/phpunit/testcases/core/functions.php#L295

Other tests of non-assocative arrays that also fail can be fixed like this (assertEquals compares keys, values and order):
$this->assertEquals( $expected, array_values( bp_sort_by_key( $items, 'value', 'num' ) ) );

Is that test sorting an object a good case for keeping usort(), or is it testing an unlikely use of bp_sort_by_key()?

Opinions welcome! :)

#8 @boonebgorges
3 years ago

  • Keywords commit removed

The fact that a tests is failing suggests to me that we should take the conservative route, and add the preserve_keys parameter. This is a semi-standard convention in PHP's array functions, so I think it's transparent enough.

#9 @dcavins
3 years ago

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

In 11082:

bp_sort_by_key(): Add $preserve_keys parameter.

Add a parameter to bp_sort_by_key() so that it calls either usort()
or uasort(), depending on the whether the array keys are important or
not.

Fixes #7238.

Note: See TracTickets for help on using tickets.