Skip to content
Permalink
Browse files

FIX ArrayList sorting now caseinsensitive

  • Loading branch information...
dhensby committed Aug 22, 2016
1 parent 6e86470 commit 4998b8044530a83c617194d544b76a98f742386e
Showing with 87 additions and 13 deletions.
  1. +6 −1 model/ArrayList.php
  2. +81 −12 tests/model/ArrayListTest.php
@@ -430,21 +430,26 @@ public function sort() {
// This the main sorting algorithm that supports infinite sorting params
$multisortArgs = array();
$values = array();
$firstRun = true;
foreach($columnsToSort as $column => $direction) {
// The reason these are added to columns is of the references, otherwise when the foreach
// is done, all $values and $direction look the same
$values[$column] = array();
$sortDirection[$column] = $direction;
// We need to subtract every value into a temporary array for sorting
foreach($this->items as $index => $item) {
$values[$column][] = $this->extractValue($item, $column);
$values[$column][] = strtolower($this->extractValue($item, $column));

This comment has been minimized.

Copy link
@jonom

jonom Sep 30, 2016

Contributor

I think this might have introduced a bug for sorting negative numeric values: #6124

This comment has been minimized.

Copy link
@dhensby

dhensby Oct 4, 2016

Author Member

@jonom if you change this so it checks if the value is numeric or not before doing strtolower does that fix the sorting of numbers issue?

This comment has been minimized.

Copy link
@jonom

jonom Oct 4, 2016

Contributor

Sorry, I should have tested this instead of speculating. This line doesn't seem to hurt actually.

This comment has been minimized.

Copy link
@jonom

jonom Oct 4, 2016

Contributor

Aside: I don't know if it would make much of a difference for performance but it seems like SORT_FLAG_CASE was introduced in php5.4 with SORT_NATURAL for case insensitive sorting, so you could maybe skip the strtolower conversion here if SORT_FLAG_CASE is defined, and pass it as a sort arg below in your php5.4+ case?

This comment has been minimized.

Copy link
@dhensby

dhensby Oct 4, 2016

Author Member

I did try a load of different combinations and this approache was the most consistent and simplest to solve the problem I was attempting to resolve.

}
// PHP 5.3 requires below arguments to be reference when using array_multisort together
// with call_user_func_array
// First argument is the 'value' array to be sorted
$multisortArgs[] = &$values[$column];
// First argument is the direction to be sorted,
$multisortArgs[] = &$sortDirection[$column];
if ($firstRun) {
$multisortArgs[] = defined('SORT_NATURAL') ? SORT_NATURAL : SORT_STRING;
}
$firstRun = false;

This comment has been minimized.

Copy link
@jonom

jonom Oct 4, 2016

Contributor

If I comment out this new section here numeric sorting works fine again.

This comment has been minimized.

Copy link
@jonom

jonom Oct 4, 2016

Contributor

Neither SORT_NATURAL nor SORT_STRING put negative numbers in the order I'd expect. SORT_REGULAR does the trick though.

This comment has been minimized.

Copy link
@jonom

jonom Oct 4, 2016

Contributor

Does SORT_NATURAL mimic ORM sorting better or could we just use SORT_REGULAR here every time?

This comment has been minimized.

Copy link
@dhensby

dhensby Oct 4, 2016

Author Member

You can change it and see how your tests and my tests perform ;)

This comment has been minimized.

Copy link
@kinglozzer

kinglozzer Oct 4, 2016

Member

Changing it back to SORT_REGULAR will cause #5900 to reoccur. This is the best I can come up with: kinglozzer@cf31b92

This comment has been minimized.

Copy link
@jonom

jonom Oct 4, 2016

Contributor

@dhensby I made a test to see if the ORM does natural sorting, and at least in my environment it doesn't seem to, in respect of something2 coming in before something10. I don't know if that would vary depending on what type of database you use etc. but in general it would probably be best for ArrayList and DataList to sort things the same way (or as close as we can get)?

@kinglozzer SORT_REGULAR shouldn't reintroduce #5900 as long as we're forcing lower case right? Which I think we could continue to do with strtlower for php5.3 and combine SORT_REGULAR with SORT_FLAG_CASE for php5.4+. Or just keep using strtlower for everything, that's easy.

I'm a bit worried about the riskiness of this bug fix. Changing the sorting algorithm used seems like a breaking change. I noticed the issue here for instance because the Grouped-CMS-Menu module suddenly couldn't put menu items in the right order.

Always using SORT_REGULAR but forcing lowercase seems like it would solve #5900 and limit the side effects of this bugfix (maximum backwards compatibility). Natural sorting is nice but I don't think we should introduce it in a bugfix. Here's a candidate: jonom@1bb5c02

This comment has been minimized.

Copy link
@dhensby

dhensby Oct 4, 2016

Author Member

Always using SORT_REGULAR but forcing lowercase seems like it would solve #5900

agreed - ok, let's do that. sorting numbers naturally is a nice to have

}
$multisortArgs[] = &$originalKeys;
@@ -247,39 +247,108 @@ public function testSortSimpleDefaultIsSortedASC() {
$list = new ArrayList(array(
array('Name' => 'Steve'),
(object) array('Name' => 'Bob'),
array('Name' => 'John')
array('Name' => 'John'),
array('Name' => 'bonny'),
));
// Unquoted name
$list1 = $list->sort('Name');
$this->assertEquals($list1->toArray(), array(
$this->assertEquals(array(
(object) array('Name' => 'Bob'),
array('Name' => 'bonny'),
array('Name' => 'John'),
array('Name' => 'Steve')
));
array('Name' => 'Steve'),
), $list1->toArray());
// Quoted name name
$list2 = $list->sort('"Name"');
$this->assertEquals($list2->toArray(), array(
$this->assertEquals(array(
(object) array('Name' => 'Bob'),
array('Name' => 'bonny'),
array('Name' => 'John'),
array('Name' => 'Steve')
));
array('Name' => 'Steve'),
), $list2->toArray());
// Array (non-associative)
$list3 = $list->sort(array('"Name"'));
$this->assertEquals($list3->toArray(), array(
$this->assertEquals(array(
(object) array('Name' => 'Bob'),
array('Name' => 'bonny'),
array('Name' => 'John'),
array('Name' => 'Steve')
));
array('Name' => 'Steve'),
), $list3->toArray());
// Check original list isn't altered
$this->assertEquals($list->toArray(), array(
$this->assertEquals(array(
array('Name' => 'Steve'),
(object) array('Name' => 'Bob'),
array('Name' => 'John')
array('Name' => 'John'),
array('Name' => 'bonny'),
), $list->toArray());
}
public function testNaturalSort() {
//natural sort is only available in 5.4+
if (version_compare(phpversion(), '5.4.0', '<')) {
$this->markTestSkipped();
}
$list = new ArrayList(array(
array('Name' => 'Steve'),
(object) array('Name' => 'Bob'),
array('Name' => 'John'),
array('Name' => 'bonny'),
array('Name' => 'bonny1'),
array('Name' => 'bonny10'),
array('Name' => 'bonny2'),
));
// Unquoted name
$list1 = $list->sort('Name');
$this->assertEquals(array(
(object) array('Name' => 'Bob'),
array('Name' => 'bonny'),
array('Name' => 'bonny1'),
array('Name' => 'bonny2'),
array('Name' => 'bonny10'),
array('Name' => 'John'),
array('Name' => 'Steve'),
), $list1->toArray());
// Quoted name name
$list2 = $list->sort('"Name"');
$this->assertEquals(array(
(object) array('Name' => 'Bob'),
array('Name' => 'bonny'),
array('Name' => 'bonny1'),
array('Name' => 'bonny2'),
array('Name' => 'bonny10'),
array('Name' => 'John'),
array('Name' => 'Steve'),
), $list2->toArray());
// Array (non-associative)
$list3 = $list->sort(array('"Name"'));
$this->assertEquals(array(
(object) array('Name' => 'Bob'),
array('Name' => 'bonny'),
array('Name' => 'bonny1'),
array('Name' => 'bonny2'),
array('Name' => 'bonny10'),
array('Name' => 'John'),
array('Name' => 'Steve'),
), $list3->toArray());
// Check original list isn't altered
$this->assertEquals(array(
array('Name' => 'Steve'),
(object) array('Name' => 'Bob'),
array('Name' => 'John'),
array('Name' => 'bonny'),
array('Name' => 'bonny1'),
array('Name' => 'bonny10'),
array('Name' => 'bonny2'),
), $list->toArray());
}
public function testSortSimpleASCOrder() {

0 comments on commit 4998b80

Please sign in to comment.
You can’t perform that action at this time.