Sorting on forms of type tableselect may not give the desired order if '#weight' is not explicitly set. The unexpected ordering may be seen at 'admin/content' or 'admin/people' when sorting the tables via the table headers.
#517318: Tableselect needs to be able to give options weights added weight sorting to tableselect forms but the two examples above, node_admin_nodes and user_admin_account do not use '#weight'. Since '#weight' is not required, we can check to see if the array contains a '#weight' and if not, skip the sorting by weight (if the '#weight' key exists, I think it's safe to assume the #options array has been constructed to use weight consistently) .
The attached patch checks to see if the '#weight' key exists and only sorts by weight if so, rather than adding weights to the two forms that weren't sorting as expected.
Comment | File | Size | Author |
---|---|---|---|
#58 | 700380-tableselect-weight.patch | 3.25 KB | mfb |
#51 | 700380-weight.patch | 3.05 KB | mfb |
#46 | 700380-weight.patch | 1.6 KB | mfb |
#44 | tableselect-700380-44.patch | 1.57 KB | mitchmac |
#38 | drupal-form-700380-38.patch | 1.34 KB | helmo |
Comments
Comment #1
mitchmac CreditAttribution: mitchmac commentedComment #2
Ryan Palmer CreditAttribution: Ryan Palmer commentedPatch applies well and fixes the tablesort issue for me.
Comment #3
Ryan Palmer CreditAttribution: Ryan Palmer commentedTablesort is broken on admin/content and admin/people without this fix.
Comment #4
sun.core CreditAttribution: sun.core commentedNot critical. Please read and understand Priority levels of Issues, thanks.
Comment #5
mitchmac CreditAttribution: mitchmac commentedRe-confirmed and re-rolled to HEAD. Should this have some test coverage as well?
Comment #6
mitchmac CreditAttribution: mitchmac commented+ tag Usability
Comment #7
Dave ReidThis 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.
Comment #8
Dave ReidAhh, 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.
Comment #9
mfb#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
Comment #10
Ryan Palmer CreditAttribution: Ryan Palmer commentedAlmost 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.
Comment #12
mfbThank u, that looks much nicer :D rerolled so it should apply.
Comment #13
Ryan Palmer CreditAttribution: Ryan Palmer commentedHmm. 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.
Comment #14
aspilicious CreditAttribution: aspilicious commentedYou need to run git on the latest DEV checkout, not alpha (maybe you downloaded alpha).
Comment #15
helmo CreditAttribution: helmo commentedI had the same problem witt the devel/variable page.
The patch from #12 worked nicely on the latest CVS HEAD.
Comment #16
aspilicious CreditAttribution: aspilicious commentedOk....
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)
Comment #17
aspilicious CreditAttribution: aspilicious commentedComment #18
Dries CreditAttribution: Dries commentedMmm, 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?
Comment #19
Dave ReidThe 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:
Output:
Maybe we need to fix this in element_sort()?
Comment #20
Dave ReidOh! 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.
Comment #22
mfbI 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?
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commenteduasort()
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:
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()).
Comment #24
mfbPersonally 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.
Comment #25
Dave ReidThe patch in #12 is still going to change around things if they don't have weight:
This array:
Becomes:
Comment #26
clemens.tolboomI 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.
Comment #27
clemens.tolboomAs 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);
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease fix coding style: avoid assignments in conditions, add a space before the '='.
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.
Code style: add spaces before and after the equality operator.
Comment #29
clemens.tolboomI 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.
Comment #30
clemens.tolboomSo we need to discuss what is consistent. And this needs review :)
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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.Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedAlso, there is one remaining style issue:
should be:
Comment #33
clemens.tolboomSorry about that coding style ... my coder install is crap (timeout) atm ... drush fails with coder too
Comment #34
clemens.tolboomI 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?
Comment #35
heather CreditAttribution: heather commentedIs 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
Comment #36
andypost#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 dependentComment #37
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks good, except the comment:
which is not accurate anymore. Marked as needs work, but please RTBC on my behalf once fixed.
Comment #38
helmo CreditAttribution: helmo commentedOK, I just updated the comment.
Lets get this in!
Comment #39
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #40
rfayTook a quick look in regard to #761482: Incorrect sorting in admin/content/node and that issue is resolved with the patch in #38.
Comment #41
Dries CreditAttribution: Dries commentedLet'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.
Comment #42
clemens.tolboomI 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
Comment #43
helmo CreditAttribution: helmo commented> - $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.
Comment #44
mitchmac CreditAttribution: mitchmac commentedI've attempted to beef up the comment to describe what is going on with the sorting. Code still seems fine.
Comment #45
helmo CreditAttribution: helmo commentedThe spaces on the empty line conflict with the code style guidelines.
It looks good otherwise.
Powered by Dreditor.
Comment #46
mfbFixed the spaces on empty line as well as some extra parentheses
if (($element_count) > 0)
Comment #47
corbacho CreditAttribution: corbacho commentedI 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.
Comment #48
clemens.tolboomLet's call it a RTBC now :)
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedoh, nice. i just posted a patch over at #906442: table sort broken at 'admin/content'. perhaps all we need is some tests?
Comment #50
rfayThis 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.
Comment #51
mfbIncorporated the test by justinrandell from #906442: table sort broken at 'admin/content' with some minor tweaks, which fails without this patch.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedawesome, lets get this in then.
Comment #53
BenK CreditAttribution: BenK commentedTracking this as I reported a duplicate of this issue... hoping we can get this in soon, too.
Maybe upgrade this to "major" priority?
Comment #54
rfayJust 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.
Comment #55
sunThis 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.
Comment #56
mfbform_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.
Comment #57
sunIs 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.
Comment #58
mfbOK here's an alternate patch, to build the table from the already-sorted element_children() rather than #options.
Comment #59
sunMakes a lot more sense now. Would love to see someone from this issue to manually test and confirm/RTBC this patch.
Comment #60
helmo CreditAttribution: helmo commentedI've tested the patch from #58 on both alpha7 and today's HEAD and it solves the problem in both instances.
Comment #61
corbacho CreditAttribution: corbacho commentedSorting 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.
Comment #62
webchickFixes 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.