Skip to:
Content

#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 21 months ago.
Add $preserve_keys parameter to bp_sort_by_key().

Download all attachments as: .zip

Change History (10)

@dcavins
21 months ago

Add $preserve_keys parameter to bp_sort_by_key().

#1 @dcavins
21 months 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
21 months ago

Patch looks nice and simple to me! +1!

#3 @r-a-y
21 months ago

  • Keywords commit added

#4 @DJPaul
21 months ago

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

#5 @dcavins
21 months 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.


21 months ago

#7 @dcavins
21 months 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
21 months 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
21 months 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.