From 797be6ac82f6938af06c24c99150648ff214f797 Mon Sep 17 00:00:00 2001 From: Jonathon Menz Date: Tue, 4 Oct 2016 11:14:16 -0700 Subject: [PATCH] FIX Revert natural sort More backwards compatible and more consistent with ORM sorting (fixes #6124) --- model/ArrayList.php | 2 +- tests/model/ArrayListTest.php | 105 +++++++++++++++++++-------------- tests/model/DataListTest.php | 29 +++++++++ tests/model/DataObjectTest.php | 7 +++ tests/model/DataObjectTest.yml | 22 +++++++ 5 files changed, 119 insertions(+), 46 deletions(-) diff --git a/model/ArrayList.php b/model/ArrayList.php index 903b04421..784fb55ce 100644 --- a/model/ArrayList.php +++ b/model/ArrayList.php @@ -448,7 +448,7 @@ class ArrayList extends ViewableData implements SS_List, SS_Filterable, SS_Sorta // First argument is the direction to be sorted, $multisortArgs[] = &$sortDirection[$column]; if ($firstRun) { - $multisortArgs[] = defined('SORT_NATURAL') ? SORT_NATURAL : SORT_STRING; + $multisortArgs[] = SORT_REGULAR; } $firstRun = false; } diff --git a/tests/model/ArrayListTest.php b/tests/model/ArrayListTest.php index 7e2e4630d..1f98fa086 100644 --- a/tests/model/ArrayListTest.php +++ b/tests/model/ArrayListTest.php @@ -314,67 +314,46 @@ class ArrayListTest extends SapphireTest { ), $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'), + public function testMixedCaseSort() { + // Note: Natural sorting is not expected, so if 'bonny10' were included + // below we would expect it to appear between bonny1 and bonny2. That's + // undesirable though so we're not enforcing it in tests. + $original = array( + array('Name' => 'Steve'), (object) array('Name' => 'Bob'), array('Name' => 'John'), array('Name' => 'bonny'), array('Name' => 'bonny1'), - array('Name' => 'bonny10'), + //array('Name' => 'bonny10'), array('Name' => 'bonny2'), - )); + ); + + $list = new ArrayList($original); + + $expected = array( + (object) array('Name' => 'Bob'), + array('Name' => 'bonny'), + array('Name' => 'bonny1'), + //array('Name' => 'bonny10'), + array('Name' => 'bonny2'), + array('Name' => 'John'), + array('Name' => 'Steve'), + ); // 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()); + $this->assertEquals($expected, $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()); + $this->assertEquals($expected, $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()); + $this->assertEquals($expected, $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()); + $this->assertEquals($original, $list->toArray()); } @@ -472,6 +451,42 @@ class ArrayListTest extends SapphireTest { )); } + public function testSortNumeric() { + $list = new ArrayList(array( + array('Sort' => 0), + array('Sort' => -1), + array('Sort' => 1), + array('Sort' => -2), + array('Sort' => 2), + array('Sort' => -10), + array('Sort' => 10) + )); + + // Sort descending + $list1 = $list->sort('Sort', 'DESC'); + $this->assertEquals(array( + array('Sort' => 10), + array('Sort' => 2), + array('Sort' => 1), + array('Sort' => 0), + array('Sort' => -1), + array('Sort' => -2), + array('Sort' => -10) + ), $list1->toArray()); + + // Sort ascending + $list1 = $list->sort('Sort', 'ASC'); + $this->assertEquals(array( + array('Sort' => -10), + array('Sort' => -2), + array('Sort' => -1), + array('Sort' => 0), + array('Sort' => 1), + array('Sort' => 2), + array('Sort' => 10) + ), $list1->toArray()); + } + public function testReverse() { $list = new ArrayList(array( array('Name' => 'John'), diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php index 1a4353127..87a695f17 100755 --- a/tests/model/DataListTest.php +++ b/tests/model/DataListTest.php @@ -23,6 +23,7 @@ class DataListTest extends SapphireTest { 'DataObjectTest_EquipmentCompany', 'DataObjectTest_SubEquipmentCompany', 'DataObjectTest\NamespacedClass', + 'DataObjectTest_Sortable', 'DataObjectTest_Company', 'DataObjectTest_Fan', 'ManyManyListTest_Product', @@ -533,6 +534,34 @@ class DataListTest extends SapphireTest { $this->assertEquals('Phil', $list->last()->Name, 'Last comment should be from Phil'); } + public function testSortNumeric() { + $list = DataObjectTest_Sortable::get(); + $list1 = $list->sort('Sort', 'ASC'); + $this->assertEquals(array( + -10, + -2, + -1, + 0, + 1, + 2, + 10 + ), $list1->column('Sort')); + } + + public function testSortMixedCase() { + $list = DataObjectTest_Sortable::get(); + $list1 = $list->sort('Name', 'ASC'); + $this->assertEquals(array( + 'Bob', + 'bonny', + 'jane', + 'John', + 'sam', + 'Steve', + 'steven' + ), $list1->column('Name')); + } + /** * Test DataList->canFilterBy() */ diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index 3ca70a887..79a685981 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -1702,6 +1702,13 @@ class DataObjectTest extends SapphireTest { } +class DataObjectTest_Sortable extends DataObject implements TestOnly { + private static $db = array( + 'Sort' => 'Int', + 'Name' => 'Varchar', + ); +} + class DataObjectTest_Player extends Member implements TestOnly { private static $db = array( 'IsRetired' => 'Boolean', diff --git a/tests/model/DataObjectTest.yml b/tests/model/DataObjectTest.yml index 19065154e..16c145971 100644 --- a/tests/model/DataObjectTest.yml +++ b/tests/model/DataObjectTest.yml @@ -1,3 +1,25 @@ +DataObjectTest_Sortable: + numeric1: + Sort: 0 + Name: steven + numeric2: + Sort: -1 + Name: bonny + numeric3: + Sort: 1 + Name: sam + numeric4: + Sort: -2 + Name: Bob + numeric5: + Sort: 2 + Name: jane + numeric6: + Sort: -10 + Name: Steve + numeric7: + Sort: 10 + Name: John DataObjectTest_EquipmentCompany: equipmentcompany1: Name: Company corp