From: Tim Otten Date: Sat, 1 Apr 2017 01:59:44 +0000 (-0700) Subject: CRM-20345 - CRM_Utils_SQL_Select::orderBy() (#4) X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=6db70618decf91e004452cf1a8f36c6536a71e8c;p=civicrm-core.git 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 3664a96477..f879edb6c8 100644 --- 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 a467baf4c9..54128213d6 100644 --- 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 e851920cf1..852a99a844 100644 --- 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++; + } + } + }