[DX] Add direction to backdrop_sort()

[docwilmot]

Was trying to build a tablesort table without a database query which required sorting an array. Works fine with backdrop_sort() but only for one click of the header, as this function has no sense of direction. Hacking common.inc works (I only changed the clause for single key, couldnt manage the multiple-key recursive wizardry) but not sure the implications.

function backdrop_sort(array &$array, array $keys = array('weight'), $dir = 'ASC') { <--------------------
  ...
  ...
  if (count($keys) === 1) {
    $key = key($keys);
    $key_sort = reset($keys);

    if ($key_sort === SORT_STRING) {
      uasort($array, function($a, $b) use ($key, $dir) {
        if (!is_array($a) || !is_array($b)) {
          return 0;
        }
        if (!isset($a[$key])) {
          $a[$key] = '';
        }
        if (!isset($b[$key])) {
          $b[$key] = '';
        }
          // Added this here
        if ($dir) {                     <---------------------------
          $dir = strtolower($dir);
          if ($dir == 'asc') {
            return strnatcasecmp($b[$key], $a[$key]);
          }
          if ($dir == 'desc') {
            return strnatcasecmp($a[$key], $b[$key]);
          }
        }
        return strnatcasecmp($b[$key], $a[$key]);
      });
    }

Thoughts?

---

PR by @docwilmot backdrop/backdrop#1535

[quicksketch]

This sounds useful to be able to sort in either direction. The code could be more concise (having strnatcasecmp($b[$key], $a[$key]) in there twice isn't ideal), but as for adding the option of sort order, I think that would be great.

PHP already has constants for sort order: SORT_ASC and SORT_DESC, lets reuse those instead of our own strings.

[quicksketch]

I just thought of this in #449 (comment), but in the mean time even without this parameter we can just array_reverse() the resulting array to make it backwards. I'm sure it's less efficient than just sorting it properly the first time, so lets continue with this idea even if we have a work-around.

[docwilmot]

Pushed PR to see if Travis/Zen would complain. Looked like they're ok. But this hasn't been tested at all. Looks like we don't have particular tests for this function AFAICT.

[quicksketch]

But this hasn't been tested at all. Looks like we don't have particular tests for this function AFAICT.

Yeah a BackdropUnitTestCase for this would appropriate, unit tests don't actually install Backdrop but just test its functions. Could you add one in bootstrap.test? Code in the PR looks great.

[jenlampton]

Is this still a candidate for 1.5? Looks like #449 got in without it, so moving to 1.x-future

[docwilmot]

So here's a curious thing: Tests failed on PHP 7, and I thought they might. The test used an array which has 3 items with the same weight. I did this so we could test sorting by multiple keys ('weight' and 'info'), which worked fine.

However, if sorting by weight alone that gives us a bit of a problem, because apparently uasort(), which is used by this function doesnt give consistent results if sorting an array which has multiple identical keys; the PHP docs says the result is "undefined", whatever that means. Sure enough though, tests passed local on PHP5, and ZenCI PHP5, but different result on PHP7.

Here's the array (note three items with weight = 1):

    $this->test_array = array(
      'breadcrumb' => array(
        'info' => 'Breadcrumb',
        'weight' => 1,
        'description' => 'The current page breadcrumb trail.',
        'render last' => TRUE,
      ),
      'main-menu' => array(
        'info' => 'Primary navigation',
        'weight' => 1,
        'description' => 'A hierarchical list of links for the Primary navigation.',
      ),
      'recent' => array(
        'info' => 'Recent content',
        'weight' => 3,
        'description' => 'A list of recently published content.',
      ),
      'content' => array(
        'info' => 'Content block',
        'weight' => 1,
        'description' => 'Displays a node in a block.',
        'class' => 'NodeBlock',
      ),
      'form' => array(
        'info' => 'Search form',
        'weight' => -2,
        'description' => 'The search form for searching site content.',
      ),
      'custom_block' => array(
        'info' => 'Add a custom block',
        'weight' => -1,
        'description' => 'A basic block for adding custom text.',
        'class' => 'BlockText',
      ),
    );

I actually expected the sort by weight to give the order (for the weight = 1 items):

array (
      'breadcrumb',
      'content',
      'main-menu',
 ) 

i.e fall back to sort alphabetical by the parent array keys (I guessed), but PHP5 insisted that it was:

array (
      'breadcrumb',
      'main-menu',
      'content',
 ) 

So I built the tests with this as the expected result (cheating, I know). But behold, PHP7 says it should be:

array (
      'content',
      'breadcrumb',
      'main-menu',
 ) 

"Undefined" indeed!

So long post, coming to an end. Should I just build a test which avoids identical keys? Or do we need some way of making backdrop_sort() consistently work with identical keys? I've seen some suggestions on the Google, but a bit complex.

Thoughts @quicksketch?

[quicksketch]

Should I just build a test which avoids identical keys? Or do we need some way of making backdrop_sort() consistently work with identical keys?

My take on this is that we should just use PHP's sorting on this, so identical keys cannot be counted on to produce consistent results.

FormAPI does some fancy stuff where it adds "microweights" to everything before the sorting is done, e.g. it makes 1 into 1.001 and then 1.002 etc, so items stay in their original order. I feel like if we need to handle situations like this, the micro-weight work around will need to be done before calling backdrop_sort(), rather than being a part of it.

[quicksketch]

Another concern: What can we do about sorts that might want to have in different directions? e.g. I want to sort by "weight" DESC but "title" ASC?

And to be clear on this question:

Should I just build a test which avoids identical keys?

Yes.

[jenlampton]

Another concern: What can we do about sorts that might want to have in different directions? e.g. I want to sort by "weight" DESC but "title" ASC?

I thought I was going to need this on the status report. In the end though I ended up making my own order instead of using a sort.

[docwilmot]

Another concern: What can we do about sorts that might want to have in different directions?

Hadnt thought of that. This would be necessarily a bit verbose:

backdrop_sort($my_array, array(
  'weight' => array('type' => SORT_NUMERIC, 'direction' => ASC)),
  'title' => array('type' => SORT_STRING, 'direction' => DESC)),
);

[docwilmot]

It wouldnt be possible to do multiple-direction sorts without an API change, so cant do it. And incidentally, should have mentioned I had already re-built the tests with non-identical keys, and tests pass.
So marking for review again.

[jenlampton]

Descoping for 1.7 since we are 4 days from release.

[jenlampton]

Looks like this could use a code review (and a rebase) if we want to get it in for 1.8. We're two weeks from code freeze.

[jenlampton]

No action on this one, bumping milestone.

[quicksketch]

It wouldnt be possible to do multiple-direction sorts without an API change

There may be a solution to this: make the $sort parameter take either the constants (SORT_ASC and SORT_DESC) or allow an array of keys, just like the the $keys parameter. Then you can optionally specify any keys you'd want to SORT_DESC (with the remaining ones using SORT_ASC by default).

e.g.

backdrop_sort(&$array, array('weight' => SORT_NUMERIC, 'name' => SORT_STRING), array('weight' => SORT_DESC, 'name' => SORT_ASC))

Or just

backdrop_sort(&$array, $keys = array('weight' => SORT_NUMERIC, 'name' => SORT_STRING), array('weight' => SORT_DESC))

Which should have the same result (keys not specified use SORT_ASC).

However, we can add that as a later enhancement while making the current PR's API backwards-compatible. Let's go ahead and merge what we have and make a new issue for adding multi-directional sorting.

[quicksketch]

If tests are fully passing we can get this in today.

[quicksketch]

I've merged in backdrop/backdrop#1535. While this is after feature freeze, the code had been ready long before freeze and all tests had been passing. And it's a minor addition they only expands one function. There was no impact here to any existing code.

So I apologize for merging something after freeze, but I hope it's excusable in this case.