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.

Files: 
CommentFileSizeAuthor
#27 tablesort_doc_cleanup-839634-27.patch945 bytesaaronott
PASSED: [[SimpleTest]]: [MySQL] 54,450 pass(es).
[ View ]
#16 noextras-839634-16.patch844 bytesrocket_nova
PASSED: [[SimpleTest]]: [MySQL] 37,870 pass(es).
[ View ]
#14 noextras-839634-14.patch864 bytesrocket_nova
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch noextras-839634-14.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#5 839634-noextras.patch974 bytesjhodgdon
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 839634-noextras.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 839634.patch2.62 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 22,179 pass(es).
[ View ]

Comments

Component:database system» documentation

Confirmed 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:

"sort": A default sort order for this column ("asc" or "desc").

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)

Title:TableSort sorts arbitrarily when no "sort" and "order" GET parameters are presentFix up documentation for table sorting

Altering title

I 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:

   if (isset($header['sort']) && ($header['sort'] == 'asc' || $header['sort'] == 'desc')) {
      $default = array('name' => $header['data'], 'sql' => isset($header['field']) ? $header['field'] : '');
    }
  }

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.

Status:Active» Needs review
StatusFileSize
new2.62 KB
PASSED: [[SimpleTest]]: [MySQL] 22,179 pass(es).
[ View ]

Ah, 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.

StatusFileSize
new974 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 839634-noextras.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Oops. That patch had some other changes in it. Try this one.

This 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.

Status:Needs review» Postponed

Agreed. Let's postpone this issue and see what happens on that other issue. If that one gets in, mark this one as a duplicate.

Status:Postponed» Needs review

That 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?

#5: 839634-noextras.patch queued for re-testing.

#5: 839634-noextras.patch queued for re-testing.

Another year (almost) has past, and still needs a review...

Status:Needs review» Needs work

The last submitted patch, 839634-noextras.patch, failed testing.

and it apparently also needs a reroll.

Assigned:Unassigned» rocket_nova
Status:Needs work» Needs review
StatusFileSize
new864 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch noextras-839634-14.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Re-rolled #5.

Status:Needs review» Needs work

The last submitted patch, noextras-839634-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new844 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,870 pass(es).
[ View ]

Oops. Made the patch against 8.x by accident. Here's the patch against 7.x.

Thanks 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.

#16: noextras-839634-16.patch queued for re-testing.

#16: noextras-839634-16.patch queued for re-testing.

Still 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

Status:Needs review» Postponed

Can'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.

Yes... 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).

Status:Postponed» Needs review

#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.

Version:7.x-dev» 8.x-dev

The steps are outlined in comment #23. The first one is to make a patch that will apply to Drupal 8.x.

Status:Needs review» Patch (to be ported)

StatusFileSize
new945 bytes
PASSED: [[SimpleTest]]: [MySQL] 54,450 pass(es).
[ View ]

Here is a patch for d8. This should take care of #23 a).

Status:Patch (to be ported)» Needs review

Someone besides me needs to review the patch in #27, as the next step. Thanks!

Status:Needs review» Reviewed & tested by the community

Seems fine to me.

Assigned:rocket_nova» jhodgdon

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Fixed

Thanks 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.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.