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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

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)

jhodgdon’s picture

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

Altering title

jhodgdon’s picture

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.

jhodgdon’s picture

Status: Active » Needs review
FileSize
2.62 KB

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.

jhodgdon’s picture

FileSize
974 bytes

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

drunken monkey’s picture

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.

jhodgdon’s picture

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.

jhodgdon’s picture

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?

jhodgdon’s picture

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

jhodgdon’s picture

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

jhodgdon’s picture

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

Status: Needs review » Needs work

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

jhodgdon’s picture

and it apparently also needs a reroll.

drupal_was_my_past’s picture

Assigned: Unassigned » drupal_was_my_past
Status: Needs work » Needs review
FileSize
864 bytes

Re-rolled #5.

Status: Needs review » Needs work

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

drupal_was_my_past’s picture

Status: Needs work » Needs review
FileSize
844 bytes

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

jhodgdon’s picture

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.

jhodgdon’s picture

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

jhodgdon’s picture

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

jhodgdon’s picture

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

jhodgdon’s picture

Status: Needs review » Postponed
scuba_fly’s picture

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.

jhodgdon’s picture

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

scuba_fly’s picture

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.

jhodgdon’s picture

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.

jhodgdon’s picture

Status: Needs review » Patch (to be ported)
aaronott’s picture

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

aaronott’s picture

Status: Patch (to be ported) » Needs review
jhodgdon’s picture

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

drunken monkey’s picture

Status: Needs review » Reviewed & tested by the community

Seems fine to me.

alexpott’s picture

jhodgdon’s picture

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.