CRM-20345 - CRM_Utils_SQL_Select::orderBy() (#4)

* CRM-20345 - CRM_Utils_Array::crmArraySortByField - Add test. Allow multiple fields.

* CRM-20345 - CRM_Utils_SQL_Select::orderBy - Use more deterministic ordering

The technique of computing default `$weight = count($this->orderBys)`
addresses a valid point: we need to preserve ordering for existing callers
who don't specify weights -- while also allowing weights.

However, it feels weird in my gut. Not sure why -- maybe it's something like this:

```php
// A1: Non-deterministic ordering
$select->orderBy('alpha', 1);
$select->orderBy('beta');
$select->orderBy('delta', 2);
$select->orderBy('gamma', 3);

// A2: Deterministic ordering
$select->orderBy('alpha', 10);
$select->orderBy('beta');
$select->orderBy('delta', 20);
$select->orderBy('gamma', 30);

// B1: Deterministic ordering
$select->orderBy('alpha');
$select->orderBy('beta');
$select->orderBy('delta');
$select->orderBy('gamma');

// B2: Non-deterministic ordering
$select->orderBy('alpha', 1);
$select->orderBy('beta', 1);
$select->orderBy('delta', 1);
$select->orderBy('gamma', 1);
```

As a reader, I would expect A1/A2 to be the same, and I would expect B1/B2
to be the same.  But they're not.  If there's a collision in the `weight`s,
the ordering becomes non-deterministic (depending on obscure details or
happenstance of the PHP runtime).

Of course, there's no right answer: in A1/A2, you can plausibly put `beta`
before `alpha` or after `alpha` or after `gamma`.  But it should be
determinstic so that it always winds up in the same place.

diff --git a/CRM/Utils/Array.php b/CRM/Utils/Array.php
index 3664a9647706aadf36f01cb12f039d405d34cb3e..f879edb6c825cd2428aaa70eb78e3374cf7b2d07 100644 (file)
--- a/CRM/Utils/Array.php
+++ b/CRM/Utils/Array.php
@@ -473,15 +473,23 @@ class CRM_Utils_Array {
    *
    * @param array $array
    *   Array to be sorted.
-   * @param string $field
+   * @param string|array $field
    *   Name of the attribute used for sorting.
    *
    * @return array
    *   Sorted array
    */
   public static function crmArraySortByField($array, $field) {
-    $code = "return strnatcmp(\$a['$field'], \$b['$field']);";
-    uasort($array, create_function('$a,$b', $code));
+    $fields = (array) $field;
+    uasort($array, function ($a, $b) use ($fields) {
+      foreach ($fields as $f) {
+        $v = strnatcmp($a[$f], $b[$f]);
+        if ($v !== 0) {
+          return $v;
+        }
+      }
+      return 0;
+    });
     return $array;
   }
 

diff --git a/CRM/Utils/SQL/Select.php b/CRM/Utils/SQL/Select.php
index a467baf4c96c179135a3e77aa4b6bbea71dab094..54128213d6c6396f6e3a1e28614daeb579a8d9d0 100644 (file)
--- a/CRM/Utils/SQL/Select.php
+++ b/CRM/Utils/SQL/Select.php
@@ -326,14 +326,12 @@ class CRM_Utils_SQL_Select implements ArrayAccess {
    * @param int $weight
    * @return \CRM_Utils_SQL_Select
    */
-  public function orderBy($exprs, $args = NULL, $weight = NULL) {
+  public function orderBy($exprs, $args = NULL, $weight = 0) {
+    static $guid = 0;
     $exprs = (array) $exprs;
-    if ($weight === NULL) {
-      $weight = count($this->orderBys);
-    }
     foreach ($exprs as $expr) {
       $evaluatedExpr = $this->interpolate($expr, $args);
-      $this->orderBys[$evaluatedExpr] = array('value' => $evaluatedExpr, 'weight' => $weight++);
+      $this->orderBys[$evaluatedExpr] = array('value' => $evaluatedExpr, 'weight' => $weight, 'guid' => $guid++);
     }
     return $this;
   }
@@ -578,7 +576,8 @@ class CRM_Utils_SQL_Select implements ArrayAccess {
       $sql .= 'HAVING (' . implode(') AND (', $this->havings) . ")\n";
     }
     if ($this->orderBys) {
-      $orderBys = CRM_Utils_Array::crmArraySortByField($this->orderBys, 'weight');
+      $orderBys = CRM_Utils_Array::crmArraySortByField($this->orderBys,
+        array('weight', 'guid'));
       $orderBys = CRM_Utils_Array::collect('value', $orderBys);
       $sql .= 'ORDER BY ' . implode(', ', $orderBys) . "\n";
     }

diff --git a/tests/phpunit/CRM/Utils/ArrayTest.php b/tests/phpunit/CRM/Utils/ArrayTest.php
index e851920cf14d66aa5a5c6b5e9d60da876de073e5..852a99a844a8d9e1b1eb16540e574aa013f30c6c 100644 (file)
--- a/tests/phpunit/CRM/Utils/ArrayTest.php
+++ b/tests/phpunit/CRM/Utils/ArrayTest.php
@@ -142,4 +142,92 @@ class CRM_Utils_ArrayTest extends CiviUnitTestCase {
     $this->assertEquals(3, $arr['zoo']['half']);
   }
 
+  public function getSortExamples() {
+    $red = array('label' => 'Red', 'id' => 1, 'weight' => '90');
+    $orange = array('label' => 'Orange', 'id' => 2, 'weight' => '70');
+    $yellow = array('label' => 'Yellow', 'id' => 3, 'weight' => '10');
+    $green = array('label' => 'Green', 'id' => 4, 'weight' => '70');
+    $blue = array('label' => 'Blue', 'id' => 5, 'weight' => '70');
+
+    $examples = array();
+    $examples[] = array(
+      array(
+        'r' => $red,
+        'y' => $yellow,
+        'g' => $green,
+        'o' => $orange,
+        'b' => $blue,
+      ),
+      'id',
+      array(
+        'r' => $red,
+        'o' => $orange,
+        'y' => $yellow,
+        'g' => $green,
+        'b' => $blue,
+      ),
+    );
+    $examples[] = array(
+      array(
+        'r' => $red,
+        'y' => $yellow,
+        'g' => $green,
+        'o' => $orange,
+        'b' => $blue,
+      ),
+      'label',
+      array(
+        'b' => $blue,
+        'g' => $green,
+        'o' => $orange,
+        'r' => $red,
+        'y' => $yellow,
+      ),
+    );
+    $examples[] = array(
+      array(
+        'r' => $red,
+        'g' => $green,
+        'y' => $yellow,
+        'o' => $orange,
+        'b' => $blue,
+      ),
+      array('weight', 'id'),
+      array(
+        'y' => $yellow,
+        'o' => $orange,
+        'g' => $green,
+        'b' => $blue,
+        'r' => $red,
+      ),
+    );
+
+    return $examples;
+  }
+
+  /**
+   * @param array $array
+   * @param string|array $field
+   * @param $expected
+   * @dataProvider getSortExamples
+   */
+  public function testCrmArraySortByField($array, $field, $expected) {
+    $actual = CRM_Utils_Array::crmArraySortByField($array, $field);
+
+    // assertEquals() has nicer error output, but it's not precise about order.
+    $this->assertEquals($expected, $actual);
+
+    $aIter = new ArrayIterator($actual);
+    $eIter = new ArrayIterator($expected);
+    $this->assertEquals($eIter->count(), $aIter->count());
+    $pos = 0;
+    while ($aIter->valid()) {
+      $this->assertEquals($eIter->key(), $aIter->key(), "Keys at offset $pos do not match");
+      $this->assertEquals($eIter->current(), $aIter->current(), "Values at offset $pos do not match");
+      $aIter->next();
+      $eIter->next();
+      $pos++;
+    }
+  }
+
 }