How many different ways can you sort elements by weight?

Spot the difference:
- http://api.drupal.org/api/function/element_sort/7 uses ['#weight']
- http://api.drupal.org/api/function/_field_sort_items_helper/7 uses ['_weight']
- http://api.drupal.org/api/function/_field_sort_items_value_helper/7 uses ['_weight']['#value']
- http://api.drupal.org/api/function/drupal_sort_weight/7 uses ['weight']

in my opinion ['weight'] is always wrong, so drupal_sort_weight should be dropped (and all references corrected)
also i think ['_weight']['#value'] is overkill... so we should go for ['_weight'] (or maybe even ['#weight']) but i'm not familiar with the Field API

The problem is that all of these function differently and all rely on how uasort, or the other PHP sorting methods work.

in my opinion ['weight'] is always wrong, so drupal_sort_weight should be dropped (and all references corrected)

We used 'weight' instead of '#weight' in drupal_add_js because it didn't really make much sense to have every property without the # prefix, and then weight with it (#315798: JavaScript Patch #2: Weight).

If we were using PHP 5.3, it would be really easy with closures, but this is not the case. In my opinion, it would make more sense to drop element_sort, _field_sort_items_helper and _field_sort_items_value_helper and somehow turn drupal_sort_weight into a generic sorting mechanism. element_sort doesn't have to be tailored to sorting form elements (maybe rename to drupal_sort_array, or something?).

Hashed this out and I think I've found a solution. I tested this out myself and it seems to work on all the test cases we need:

function drupal_array_sort(&$array, $keys = array('#weight')) {
  function _drupal_array_sort($a, $b) {
    $a_weight = $a;
    $b_weight = $b;
    foreach ($keys as $key) {
      $a_weight = is_array($a_weight) && isset($a_weight[$key]) ? $a_weight[$key] : 0;
      $b_weight = is_array($b_weight) && isset($b_weight[$key]) ? $b_weight[$key] : 0;
    }
    if ($a_weight == $b_weight) {
      return 0;
    }
    return ($a_weight < $b_weight) ? -1 : 1;
  }
  return uasort($array, '_drupal_array_sort');
}

Therefore, the function can be called in a number of ways, all fitting the current use cases:

drupal_array_sort($my_array);
drupal_array_sort($my_array, array('weight'));
drupal_array_sort($my_array, array('_weight', '#value'));
drupal_array_sort($my_array, array('_weight', 'value'));

Does this seem like an acceptable solution? I can start working on replacing all four functions tomorrow morning.

php -r 'function x() { $a = 1; function y() { print $a; } } x();y();'
chx@tumbler:~/Desktop$  

this does not work. php functions are global and do not have a scope like you used.

Based off of Dave solution ins #4:

/**
 * Generic sorting mechanism.
 *
 * @param $array
 *   The array to sort.
 * @param $key
 *   The index to retrieve. Child keys are passed by comma-seperated values ('item,#weight').
 */
function drupal_array_sort(&$array, $key = '#weight') {
  static $weightsort = array();
  if (!isset($weightsort[$key])) {
    $weightsort[$key] = create_function('$a,$b', '$keys = explode(",","'.$key.'");
      foreach ($keys as $key) {
        $a = is_array($a) && isset($a[$key]) ? $a[$key] : 0;
        $b = is_array($b) && isset($b[$key]) ? $b[$key] : 0;
      }
      if ($a == $b) {
        return 0;
      }
      return ($a < $b) ? -1 : 1;');
  }
  return uasort($array, $weightsort[$key]);
}

drupal_array_sort($array); // Uses #weight
drupal_array_sort($array, 'weight'); // Uses 'weight'
drupal_array_sort($array, '_weight,#value'); // Uses ['_weight']['#value']

........... Hate me.

@chx I have no idea why then it works for me.

So on further inspection I get the following errors, but the sort magically works somehow. The code with results is in http://drupalbin.com/7978
* Notice: Undefined variable: keys in _drupal_array_sort() (line 25 of /home/davereid/Projects/drupal-head/test.php).
* Warning: Invalid argument supplied for foreach() in _drupal_array_sort() (line 25 of /home/davereid/Projects/drupal-head/test.php).

EDIT: Ok so I've figured out why my function was giving the expected results. RobLoach's improvement actually works properly instead of mistakenly. :)

Initial patch replacing all the four sort functions with a generic drupal_sort_array().

TODO:
- Benchmarking and profiling
- Check other calls to usort, uasort, etc to see if this method can be expanded to help replace more duplicated code.

Here it is with some tests. Also added the default of '#weight'. There has to be a better way of doing it. Well, at least we have the tests for it, as well as the the API. create_function seems like a huge hack though......

Hmm...since this is such a generic sort function, I'm not sure I agree that we should have a default value for the sort key. I don't know of any other way to get around the create_function. Setting as CNR so I can see how it does with the testing bot.

The array sorting tests passed..........

Array Sorting 3 0 0

catch: Hmmmm? function drupal_sort_array(&$array);

I think this will be much better...

function element_sort($a, $b, $location = array('#weight')) {
  $a_weight = $a;
  $b_weight = $b;
  // traverse the array to the correct location.
  foreach($location as $value) {
    $a_weight = (is_array($a_weight) && isset($a_weight[$value])) ? $a_weight[$value] : 0;
    $b_weight = (is_array($b_weight) && isset($b_weight[$value])) ? $b_weight[$value] : 0;
  }
  if ($a_weight == $b_weight) {
    return 0;
  }
  return ($a_weight < $b_weight) ? -1 : 1;
}

sorry, don't have time for a patch ;)

@alexanderpas If you intend to pass that function to uasort, it won't work because you can't pass additional parameters via uasort.

funtions with callback arguments always get me...

Saving a few lines of code, does it worth to use create_function? I doubt. Can't we instead inspect the data structures being sorted and unify them? At least some of them?

Did some debugging and figured out what was causing the tests to file. Since the array items were references in theme_field_multiple_value_form():

    // Sort items according to '_weight' (needed when the form comes back after
    // preview or failed validation)
    $items = array();
    foreach (element_children($element) as $key) {
      if ($key !== $element['#field_name'] . '_add_more') {
        $items[] = &$element[$key];
      }
    }
    drupal_sort_array($items, '_weight,#value');

The following condition in drupal_sort_array() was causing array elements to be set to '0':

      foreach ($keys as $key) {
        $a = is_array($a) && isset($a[$key]) ? $a[$key] : 0;
        $b = is_array($b) && isset($b[$key]) ? $b[$key] : 0;
      }

which can be fixed by making a local copy of the element:

      $a_weight = $a;
      $b_weight = $b;
      foreach ($keys as $key) {
        $a_weight = is_array($a_weight) && isset($a_weight[$key]) ? $a_weight[$key] : 0;
        $b_weight = is_array($b_weight) && isset($b_weight[$key]) ? $b_weight[$key] : 0;
      }

I also adjusted and improved the tests so it can catch this condition. Also bumping down from critical, since this isn't exactly...critical.

Not sure how far we could get with trying to standardize our data structures. It will required drupal_add_js (and consequently drupal_add_css) to use a FAPI style array. If not, we'll still have two different sort functions. Plus I have no idea about the field API structures, but on initial review, it's not going to be possible to not have more than two other sort functions.

I'd love someone to show me wrong, but I think this utility function has shown it can be useful throughout core.

Some initial benchmarking on #23 w ith ab -c 1 -n 500 http://mysql.drupalhead.local/. FYI, an internal counter in drupal_sort_array() shows that the function is hit 23 times for each request with the patch.

Unpatched w/ four different sort functions:

Requests per second:    5.14 [#/sec] (mean)
Time per request:       194.651 [ms] (mean)
Time per request:       194.651 [ms] (mean, across all concurrent requests)
Transfer rate:          90.69 [Kbytes/sec] received

Patched w/ drupal_array_sort:

Requests per second:    5.14 [#/sec] (mean)
Time per request:       194.631 [ms] (mean)
Time per request:       194.631 [ms] (mean, across all concurrent requests)
Transfer rate:          90.69 [Kbytes/sec] received

Forgive me if I'm missing something here, but why is the drupal_sort_array() faster by 0.02 ms? Shouldn't it be slower?

I'll do some more tests, but yeah, it's a little suprising.

I'd also like to take a look at if and how the following functions could be replaced by a general drupal_sort_array(). These are all functions called with usort() or uasort(). The functions I have starred are probably the most likely candidates for replacement.
_system_sort_requirements
_block_compare
_user_sort*
system_sort_modules_by_info_name*
_fitler_list_cmp*
_update_project_status_sort

That's quite likely to be within error margin. It's very likely that within a page request that function plays so little role that it's not measurable really. This does not mean that we want create_function in core, really.

Recreated from HEAD.

Generated functions are so ugly.

class sorter {
  function __construct($index) { $this->index = $index;}
  function sort() {
    $a_weight = (is_array($a) && isset($a[$this->index])) ? $a[$this->index] : 0;
....
}
uasort($a, array(new sorter('weight'), 'sort'));
}

try that on.

Errant GD-related testbot failure...resetting status.

I'm not sure I understand the purpose of this patch. To me, this patch adds a layer of complexity and abstraction that wasn't there before. It is much easier to understand uasort() than it is to understand the new sort function.

Subscribing. I'm about to make yet another sort function for #445736: Poll does not respect weight in Preview or More button. :P

This patch should take into account adding microweights to prevent elements with the same weight from being reordered (say sorting an array with all the weights = 0).

I think this patch is trying to abstract something which is easy enough. Adding a layer of abstraction actually obfuscates things in this case.

Cleaning up DrupalWTF list... Since this is a won't fix, removing tag...