Regression: Tableselect sorting is altered if '#weight' is not set

Patch applies well and fixes the tablesort issue for me.

Tablesort is broken on admin/content and admin/people without this fix.

Not critical. Please read and understand Priority levels of Issues, thanks.

This would definitely be a candidate for a 'major' priority status as all table selects are broken. Have we tested this patch if people do have #weights set for individual elements in the table sort? Because I think it will probably fail for this case.

The key is that at this point when the form process runs, we haven't yet hit the part in form_builder() that assigns a default #weight value to elements to preserve the array ordering. After that is when the element_sort should be performed.

Ahh, its so simple. We should not be calling element_sort() ourselves. The Form API should be doing it for us. I removed the call to uasort/element_sort in the tableselect process function and by golly, they worked.

#8 fixes the bug but meanwhile #517318: Tableselect needs to be able to give options weights implies these lines are needed elsewhere (update.manager.inc)?

In which case we need something like this logic to do the uasort() if any of the options define a #weight

Almost certainly still an issue, but I'm unable to reproduce under the original conditions.

If patch on #9 is the one, re-rolled as a few less lines of code.

Thank u, that looks much nicer :D rerolled so it should apply.

Hmm. Any idea what was wrong with my patch? I rolled it using git diff > tableselect-weight-700380-10.patch. I just followed http://drupal.org/node/707484.

You need to run git on the latest DEV checkout, not alpha (maybe you downloaded alpha).

I had the same problem witt the devel/variable page.
The patch from #12 worked nicely on the latest CVS HEAD.

Ok....

Let's review this.

Apparantly we only can do uasort() if any of the options define a #weight.
So we loop through the options, if we find one with a weight ==> uasort() the array and break the loop.

This looks good to me + helmo says it is working nicely + bot gives green light + it still applies
==> RTBC

(I also grepped drupal core to double check if we can use the break statement in core)

Mmm, I don't understand this patch. uasort() is supposed to iterate over the array, just like the proposed foreach-loop does, and if #weight is not set, element_sort() should set the weight to 0. Any idea what I'm missing?

The problem we're encountering is if none of $element['#options'] has any weights, the element_sort() function should *not* rearrange them at all, but it actually does.

Demo:

$elements['#options'] = array();
$elements['#options']['first'] = array('#title' => 'First');
$elements['#options']['second'] = array('#title' => 'Second');
$elements['#options']['third'] = array('#title' => 'Third');
uasort($elements['#options'], 'element_sort');
var_export($elements);

Output:

array (
  '#options' => 
  array (
    'third' => 
    array (
      '#title' => 'Third',
    ),
    'second' => 
    array (
      '#title' => 'Second',
    ),
    'first' => 
    array (
      '#title' => 'First',
    ),
  ),
)

Maybe we need to fix this in element_sort()?

Oh! So we definitely need to fix element_sort and our other sort handlers! If values are equal, sort functions should never return 0 as it reverses the order of those elements in the array.

I had already tried the approach of modifying element_sort() and wasn't able to get it to work, my guess was because the algorithm compares the elements in an unpredictable order. Does this actually work for you?

uasort() doesn't give any guarantee on the order of elements of an equal weight.

What we need here is to explicitly set this weight if we want the original order to be significant:

$count = 0;
$sort = FALSE;
foreach ($element['#options'] as $choice) {
  if (!isset($element['#weight'])) {
    $element['#weight'] = $count++/1000;
  }
  else {
    $sort = TRUE;
  }
}
if ($sort) {
  uasort($element['#options'], 'element_sort');
}

That's basically what the Form API does in form_builder(), and we have the same pattern in a few places in core (especially in drupal_get_css() and drupal_get_js()).

Personally I think the patch in #12 is fine -- it uses the existing order when #weight is not set for any of the options, and ignores the existing order when #weight is set for at least one of the options. But if we want to use the existing order when #weight is set for at least one of the options then we'll need the code above.

The patch in #12 is still going to change around things if they don't have weight:

This array:

$element['#options']['first'] = array('#title' => 'First');
$element['#options']['second'] = array('#title' => 'Second');
$element['#options']['low_weight'] = array('#title' => 'Low weight', '#weight' => -100);

Becomes:

$elements['#options'] = array(
  'low_weight' => ...
  'second' => ...
  'first' => ...
);

I agree with #23

1. no options have a weight -> skip sorting
2. some options have a weight -> fix other weights to keep these items sorted in relation with each other -> 3
3. all options have weight -> sort options

But we should handle weights==0 different. That is in the patch too.

I don't like the number 1000 ... so changed that to make sure we can sort longer #options.

As mentioned in #23 both drupal_add_css and drupal_add_js adds a weight to their elements.

So those sort are safe but their weight can be adjusted/deleted through drupal_alter('js', $javascript); and drupal_alter('css', $javascript);

-  if (count($element['#options']) > 0) {
+  if (($element_count= count($element['#options'])) > 0) {
     if (!isset($element['#default_value']) || $element['#default_value'] === 0) {
       $element['#default_value'] = array();
     }

Please fix coding style: avoid assignments in conditions, add a space before the '='.

+      // Make sure weightless or weight==0 options get some weight

Comments should end with a period.

I disagree about the special casing of weight == 0. We don't do that elsewhere, and it will make this implementation inconsistent. Please remove.

+      if (!isset($choice['#weight']) || $choice['#weight']==0) {
+        $element['#options'][$index]['#weight'] = $count++ / $element_count;
+      }

Code style: add spaces before and after the equality operator.

I fixed the style messages.

About the weight == 0 ... I'm not sure why that make this implementation inconsistent. What if I add two items to a long list of items with no weights defined
Item n+1 : weight = 0
Item n+2 : no weight defined

Without the weight == 0 test that item ends in first OR second place.
With the weight == 0 test it ends as the second last which makes more sense to me.

So we need to discuss what is consistent. And this needs review :)

I meant: it is inconsistent because none of the other implementations do that (form_builder(), drupal_get_css(), drupal_get_js()...).

I don't care either way, but we need to be consistent. Let's just drop that check and use the same as in form_builder() for now !isset($element['#weight']). You can open a separate issue if you want to change that.

Also, there is one remaining style issue:

$element_count= count($element['#options']);

should be:

$element_count = count($element['#options']);

Sorry about that coding style ... my coder install is crap (timeout) atm ... drush fails with coder too

I missed #31 ... so removed the weight == 0 condition.

I'm not sure whether people will run into the edge case. Adding elements to a table are probably done within a loop. Or is there a lot of sub tabling done?

Is this a duplicate of : #761482: Incorrect sorting in admin/content/node?

e.g., The sorting does not work for title, type or date:

http://skitch.com/heatherjames/dfn39/sorted-by-title
http://skitch.com/heatherjames/dfn4d/sorted-by-update
http://skitch.com/heatherjames/dfn4m/sorted-by-type

#34 fixes problem so +1 to RTBC

Duplicate issue with some details #761482: Incorrect sorting in admin/content/node

As mentioned early uasort() is platform dependent

Looks good, except the comment:

// Make sure weightless or weight==0 options get some weight.

which is not accurate anymore. Marked as needs work, but please RTBC on my behalf once fixed.

OK, I just updated the comment.

Lets get this in!

Took a quick look in regard to #761482: Incorrect sorting in admin/content/node and that issue is resolved with the patch in #38.

Let's add some code comments. There is a lot of knowledge captured in this issue so it would be good to summarize that in a few sentences.

I guess the comment should contain something like:

- $element_count makes sure the added weights are less then 1

- We add weight to the weight-less with values between [0,1) that is between 0 (included) and 1 (not included)
(We could discuss the inclusion of 0 : see #31)

- This means all weightless are grouped together and not aligned in between their weight-full elements

> - $element_count makes sure the added weights are less then 1

This seems less relevant, it's only how we solve a detail. The other two lines are more important.

How about this:
Sorting order:
#weight < 0
#weight not set
#weight > 0

All weightless items are grouped together in their original order and sorted in between items with a high or low weight.
To do this weights are added to the weight-less items with values between [0,1) that is between 0 (included) and 1 (not included)

I guess we should add this just above the "$count = 0;" line.

+++ includes/form.inc	31 Jul 2010 18:47:07 -0000
@@ -2823,13 +2823,29 @@
+    ¶

The spaces on the empty line conflict with the code style guidelines.
It looks good otherwise.

Powered by Dreditor.

Fixed the spaces on empty line as well as some extra parentheses if (($element_count) > 0)

I found this issue because "Variable Editor" of Devel 7x-dev was behaving strange ways (It uses tableselect form element)

I applied this last patch over last last Drupal 7.x-dev and it solved the sorting issues
Good work.

Let's call it a RTBC now :)

This patch should really go in. This is amazingly confusing for users, as all the user and content tables sort in incomprehensible ways. People expect the content to be sorted last to first, and it's sorted by other random criteria.

If one of you could write an issue summary it may help the committers to get this in sooner.

Incorporated the test by justinrandell from #906442: table sort broken at 'admin/content' with some minor tweaks, which fails without this patch.

Tracking this as I reported a duplicate of this issue... hoping we can get this in soon, too.

Maybe upgrade this to "major" priority?

Just reread the Priority level description for major, and I think this is Major. It is quite a WTF for nearly every user, since both the admin/user and admin/content screens are completely foobared, among other things. It's conceivable that this is critical, since it's such a fundamental usability issue.

+++ includes/form.inc	9 Sep 2010 16:26:22 -0000
@@ -2821,13 +2821,29 @@ function form_process_tableselect($eleme
-    // Sort the options by their #weight if they have a #weight.
-    uasort($element['#options'], 'element_sort');
+    $count = 0;
+    $sort = FALSE;
+    foreach ($element['#options'] as $index => $choice) {
+      // If #weight is not set, add a #weight value that maintains the order of
+      // the #options array elements that lack #weight. The range of the added
+      // #weight values is 0 to less than 1. The elements without #weight will
+      // be sorted as a group between others with #weight < 0 and #weight >= 1.
+      if (isset($choice['#weight'])) {
+        $sort = TRUE;
+      }
+      else {
+        $element['#options'][$index]['#weight'] = $count++ / $element_count;
+      }
+    }
+    if ($sort) {
+      uasort($element['#options'], 'element_sort');
+    }

This entire code means that we are facing an entirely different bug: this #process has to be an #after_build or #pre_render.

form_builder() already adds proper #weight properties to all elements in a form. They are not contained here, because this process callback is invoked before the sub-elements are processed. Therefore, if this is a problem, then the #process callback itself is wrong.

Powered by Dreditor.

form_builder() doesn't descend into the #options array and add #weight properties there. so moving the uasort($element['#options'], 'element_sort'); to #after_build or #pre_render doesn't help (removing it entirely does since that's where the bug is :p)

If we want to be able to use element_sort then we have to add the #weight properties for the #options somewhere.

Is it possible that tableselect is entirely wrong in being based on #options? Form API expands #options into sub-elements, and the sub-elements get properly ordered, just like any other elements are. The sub-elements are available after #process has been executed. So we circle back into #after_build or #pre_render.

OK here's an alternate patch, to build the table from the already-sorted element_children() rather than #options.

Makes a lot more sense now. Would love to see someone from this issue to manually test and confirm/RTBC this patch.

I've tested the patch from #58 on both alpha7 and today's HEAD and it solves the problem in both instances.

Sorting is broken still in beta1 and it's quite annoying. #58 fix it.

Manually applied to beta1 without problems. It works on the tables of pages devel/variable . I generated some random users and check also sorting in admin/people

Everything OK.

Fixes a nasty bug, adds a test so we never have to fix it again. What's not to love? Read over the comments and they looked good to me.

Committed to HEAD.