This issue concerns two things about using the TableSort extender when the associated GET parameters are not set (so, the default behaviour). One is a bug, the other maybe only a feature request, or an issue with the documentation.
When no GET parameters are present…
* TableSort::order()
will return the last specified sort field (i.e., a header field for which the "sort" field is set) and
* TableSort::getSort()
will return the default sort of the first specified sort field, meaning that
* the table will be sorted by the last sortable row, in the order specified for the first sortable row.
(I hope I got that right.)
This is clearly no sensible solution.
So the issues are:
* bug request: The default sort order should in any case be the order specified for the field that is also used for sorting.
* feature request: Allow users to specify a "default search field" instead of just using the last one, which is probably only in a few cases what the user will want. Could be either specified in the array as a "default" => TRUE
entry set for the default field, or (my favorite) as an optional additional parameter to TableSort::orderByHeader()
.
* documentation request: As far as I can see, none of this behaviour is documented anywhere – neither on drupal.org, nor in the code.
Comment | File | Size | Author |
---|---|---|---|
#27 | tablesort_doc_cleanup-839634-27.patch | 945 bytes | aaronott |
#16 | noextras-839634-16.patch | 844 bytes | drupal_was_my_past |
#14 | noextras-839634-14.patch | 864 bytes | drupal_was_my_past |
#5 | 839634-noextras.patch | 974 bytes | jhodgdon |
#4 | 839634.patch | 2.62 KB | jhodgdon |
Comments
Comment #1
BerdirConfirmed by looking at the code, but....
'sort' is global default. It is used to define the column which is sorted by default. *Not* the default direction of that field. See #109493: tablesort should allow rendered tables to have columns that default to DESC when their header is clicked for a feature request, which nobody has written a patch for yet.
So, you are not supposed to define multiple 'sort' keys, as that results in undefined behavior.
Looking at the api documentation, tablesort_sql() doesn't contain this information, orderbyHeader() doesn't either and theme_table() actually has it wrong:
So, reclassifying this issue as a documentation problem, follow up in the linked issue above regarding your feature request. Implementing the feature also needs to take care of the current "bug" (which is not an actual bug since you're passing "wrong" arguments)
Comment #2
jhodgdonAltering title
Comment #3
jhodgdonI took a closer look at the code for theme_table().
theme_table() says you can define a header array, and for each header cell, you can have
* - "field": The database field represented in the table column (required
* if user is to be able to sort on this column).
* - "sort": A default sort order for this column ("asc" or "desc").
Inside theme_table(), it calls some functions that eventually lead to tablesort_get_order(), which is in fact doing:
So this looks to me as though theme_table() is documented correctly -- it is looking for the field and sort entries in the array, and using them to set the default sort order for the table sort.
Comment #4
jhodgdonAh, I see what you are saying. It is true: if you specify more than one sort field, the last one it finds is the one that will be used as the default. We should document in theme_table() that you should only specify one default sort, as only one column can be sorted at a time (it doesn't support "sort by this column, then if there are ties, by this column' sorting).
Here's a patch that documents this fact.
Comment #5
jhodgdonOops. That patch had some other changes in it. Try this one.
Comment #6
drunken monkeyThis is already being fixed in the course of another issue, see #109493-19: tablesort should allow rendered tables to have columns that default to DESC when their header is clicked. The description there seems to me to be the more complete one, although yours would still be an improvement compared to the status quo.
But I don't think that fixing this twice (and possibly having to re-roll the other patch) would be sensible.
Comment #7
jhodgdonAgreed. Let's postpone this issue and see what happens on that other issue. If that one gets in, mark this one as a duplicate.
Comment #8
jhodgdonThat other issue was postponed to D8 as a feature request. So we need to make sure the doc for table sorting is fixed up to match the current behavior. The patch at #5 needs a review -- is it correct?
Comment #9
jhodgdon#5: 839634-noextras.patch queued for re-testing.
Comment #10
jhodgdon#5: 839634-noextras.patch queued for re-testing.
Comment #11
jhodgdonAnother year (almost) has past, and still needs a review...
Comment #13
jhodgdonand it apparently also needs a reroll.
Comment #14
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedRe-rolled #5.
Comment #16
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedOops. Made the patch against 8.x by accident. Here's the patch against 7.x.
Comment #17
jhodgdonThanks for the reroll! Since I wrote the patch in #5, and this is a reroll, someone else needs to review the content of the patch.
Comment #18
jhodgdon#16: noextras-839634-16.patch queued for re-testing.
Comment #19
jhodgdon#16: noextras-839634-16.patch queued for re-testing.
Comment #20
jhodgdonStill needs someone other than me to review this patch... Actually, we need to patch D8 docs as well as D7 at this point, maybe, depending on what's happening on
#109493: tablesort should allow rendered tables to have columns that default to DESC when their header is clicked
Comment #21
jhodgdonI'm marking this postponed until #109493: tablesort should allow rendered tables to have columns that default to DESC when their header is clicked is resolved.
Comment #22
scuba_flyCan't you just change the documentation now and change it back when this very old feature request / bug is finally implemented / fixed ?
Just spend hours trying to figure out what was wrong with my code and why it wouldn't sort by 'desc' by default while this was at least a 2010 issue...
My suggestion:
Just change:
"sort": A default sort order for this column ("asc" or "desc").
to:
"sort": The default sort column for this table ( only sorts "asc" ).
And change it back later or also put in that a feature request is being worked on.
Comment #23
jhodgdonYes... that other issue isn't going anywhere very fast. Here is what we could do:
a) Someone could make a documentation patch for Drupal 8.x (based on #16, which is a reroll of #5).
b) Someone other than me could review the patch (I wrote the patch in #5 so I cannot review it).
c) I could commit that patch to both Drupal 8 and Drupal 7.
d) As a courtesy, someone would also need to reroll the patch that is being worked on on #109493: tablesort should allow rendered tables to have columns that default to DESC when their header is clicked and add a comment explaining what we did here and why.
If someone wants to make a commitment to do all of this, I would be happy to have this issue reopened. I can take care of (c).
Comment #24
scuba_fly#5: 839634-noextras.patch queued for re-testing.
Is this what needed to be done? I'm willing to put time in this but don't know were to start.
I do know how to make a patch but don't know how to submit it or review it.
Comment #25
jhodgdonThe steps are outlined in comment #23. The first one is to make a patch that will apply to Drupal 8.x.
Comment #26
jhodgdonComment #27
aaronott CreditAttribution: aaronott commentedHere is a patch for d8. This should take care of #23 a).
Comment #28
aaronott CreditAttribution: aaronott commentedComment #29
jhodgdonSomeone besides me needs to review the patch in #27, as the next step. Thanks!
Comment #30
drunken monkeySeems fine to me.
Comment #31
alexpottComment #32
jhodgdonThanks all!
I've committed the patch in #27 to Drupal 8.x, and the patch in #16 to 7.x (they were the same patch but with 7.x/8.x context).
So, now the documentation for both versions at least matches what the code is currently doing. Watch this other issue for fixing the code to actually do something worthwhile:
#109493: tablesort should allow rendered tables to have columns that default to DESC when their header is clicked
And if someone wants to go over there and reroll its patch that might be helpful - it probably will not apply after this commit.