Right now, tablesort_sql() allows only for additional sorting before tablesorting. Also, it omits the additional sorting as well, if the table doesn't get sorted.

The additional sorting functionality should be used on most sortable tables, as it ensures that within the tablesorted column the display order is not random but defaults to a sensible ordering. For example in the nodes or comments admin form, even if the admin sorts by the creator's uid (a discrete variable), within each uid we still want the items be sorted by 'changed' date (a quasi-continuous variable).

As long as the default sorting order is not reliable, we can't use it and have to use workarounds. Also, in most cases we obviously need additional sorting after tablesorting, so the former doesn't break the latter.
That's why the existing $before parameter has not at all been used in core.

I therefore completely refactored tablesort_sql() to allow for additional sorting after tablesorting as well. Using an imploded array of sortings ensures the "ORDER BY" clause is returned if any sorting needs to take place.

As the $before parameter has not yet been used in core, this small API change doesn't break any functionality.
This patch has undergone basic testing. Still, some reviews and further testing would be great to make sure this can be committed early!

Note that Make admin/content/node sortable requires this patch to be committed.

CommentFileSizeAuthor
tablesort_after.patch1.86 KBpancho

Comments

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

These 'before' and 'after' parameters seems hacky to me, but maybe it is just me.

I do not like the idea, that the actual sort is not associated with a user readable text, like the column header title.

sidenote:
I would like to see a general solution in drupal 7 for tablesorting.
Selectable sort: Sort by column which is not displayed in the table, but it is part of the query. (add invisible column headers, with translatable text)
Multi sort: sort by multiple columns. (max 3 columns, with a nice (popup?) selection box)
There are lot of possibilities on improvement.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

I don't think, the "before" and "after" parameters are hacky - of course we could call them more sophisticated "pre" and "post" or "presort" and "postsort", but this wouldn't change anything. IMHO this is just the right way to do this.

If you don't like the idea, that the actual sort is not associated with the column header title, you probably like the idea even less to have them randomly ordered the way the database spits it out. There must be a default way to present items from the database, and the default way should be given with the "post" parameter and not left to the database's mood. The "after" sort order is absolutely transparent.

In the contrary we could talk about the "before" sort order. I left this one in, as I didn't want to remove functionality. But if something's intransparent, then a sorting that is prepended before the tablesorting, because it breaks the sorting chosen by the user.
I'd be fine with removing that, as it is not even used in core. But the "after" parameter is really needed and it is neither hackish nor intransparent.

Surely, a general tablesorting solution would be great, as it could greatly reduce redundant code. I absolutely support moves towards consistancy and convention. But let's face reality: If noone comes up with this general approach, we will probably neither have this nor that in D7 before feature freeze.

Please support this change, as we really need it (wasn't it you who demanded sorting by changed date on #213151?) and we need it right now. If at a later stage a better solution, some kind of general approach is being taken, even better.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

I know we need it, but I don't understand the

we need it right now

argument. The original issue #132714: Add sortable header to form at admin/content/node is 10 months old now, and there was no hurry to get it in. And this feature request is for drupal 7, we have plenty of time to think about it. I would like to see and test improvements with drupal 7, not just an easy bug fix.

My quick assumption is, if the 'before' parameter is not used, then something must be wrong with it. What was the reason behind introducing this parameter? Who is using it? Are you sure there is nothing wrong with the 'after' parameter? Why don't we introduce only one general parameter as an array for these two (before an after), and maybe we can add more things to it?

Just quick questions, but as you can see, I am not sure. I would like to think and design before development. If there will be no other solutions, I will gladly help you to get this fix in before D7 feature freeze, because the missing tablesort in content management is really embarrassing.

If you are trying to get this for drupal6 maintenance release, then you must do it without api change. It is not impossible. The patch in original issue #132714, has default sorting by node.changed column, but I will not try to convince anybody that it is an acceptable solution..

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

You are absolutely right that we don't definitely need it right now. I went a bit too far with that statement. I just want to get the "obvious little fixes" done early to move on to the "big topics" for D7. Also, I prefer a gradual process to "all or nothing" strategies.

But I guess, we should leave this open for now to get some more input, and then we will see how to proceed best.

Also, I won't try to get sortable headers for admin/content/nodes into a drupal6 maintenance release, as it already has been rejected and moved to D7.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

another sorting idea:
remembered sort: remember the previous sort and use it after actual

Meaning, if the previous sort was "n.changed desc" and clicking on n.title, that would be "ORDER BY n.title ASC, n.changed DESC".
The count of columns for "order by" clause should not be more than 3. The first is dropped when the fourth comes in (FIFO).

robin monks’s picture

robin monks commented
Status: Needs review » Reviewed & tested by the community

Patch applies correctly, and the Database SimpleTest ran without issues.

Robin

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

It would be great if we could make the PHPdoc a bit more explanatory. I couldn't figure out how to use the $after parameter without actually looking at the code. I recommend that we make it slightly more verbose, and maybe go as far as including an example. That will also help reviewers understand the use case. Thanks!

lilou’s picture

lilou commented

See also #242962: Add tableselect form element

newbuntu’s picture

newbuntu commented

This seems to break forum.module in drupal 6.12.
Here is part of the error output:
"WHERE n.status = 1 AND r.tid = 1 ORDER BY n.sticky DESC,, l.last_comment_timestamp DESC, n.created DESC LIMIT 0, 25". You can see there are two commas ,, right after n.sticky. DESC.

Here is the source in forum.module that is causing the problem:

  $sql .= tablesort_sql($forum_topic_list_header, 'n.sticky DESC,');
  $sql .= ', n.created DESC';  // Always add a secondary sort order so that the news forum topics are on top.

I had to hack forum.module. I removed all the commas, and used the "$after" parameter

  $sql .= tablesort_sql($forum_topic_list_header, 'n.sticky DESC', ' n.created DESC'); // hack
  //$sql .= ', n.created DESC';  // Always add a secondary sort order so that the news forum topics are on top.

I don't like to hack. What would be the correct way to fix it?

thx!

drewish’s picture

drewish commented

we've got the TableSort extender now. i wonder if that helps this.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Fixed

Yes it absolutely does. You can have as many sorts before & after as you want. :)

Status: Fixed » Closed (fixed)

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