Steps to Reproduce

Create an array of column headers as described in theme_table() with one sort parameter to indicate the default sort column and order and then call a sort function that depends on tablesort_get_order() such as tablesort_sql().

I found the problem while trying to figure out why the Workspace module did not have a default sort order. It turned out it does, but the core drupal sort function was not working as anticipated. Here's a code snippet from that module to help show usage.

  ...
  $header = array(
    array('data' => t('Type'), 'field' => 'type'),
    array('data' => t('Title'), 'field' => 'title'),
    array('data' => t('Owner'), 'field' => 'uid'),
    array('data' => t('Published'), 'field' => 'status'),
    array('data' => t('Modified'), 'field' => 'changed', 'sort' => 'desc'),
    $comments_enabled ? array('data' => t('Replies'), 'field' => 'comment_count') : array('data' => ''),
    array('data' => t('Operations'), 'colspan' => 2)
  );

  $result = pager_query($sql . <strong>tablesort_sql($header)</strong>, $maxnodes, 0, $count_sql);
  ...

Expected Behaviour

Based on the passed array I would expect the return of the tablesort_sql() call to be "ORDER BY changed DESC".

Actual Results

The tablesort_sql() call results in an empty string.

My Suspect

I traced the problem back to the tablesort_get_order function. From my testing it almost appeared as if a scope issue. It was definitely detecting the default order and setting the $default variable, however, the variable would vanish outside of the loop. I've never run into this kind of behavior before and it looks like it should work but just wasn't for me.

Version Information

I created a little test script and ran it on a couple of other computers to make sure it wasn't behavior specific to my version of PHP but got the same results. Here are the systems I've tried it on:

Dreamhost (PHP 4.4.8 and Apache).
Ubuntu Gutsy (PHP 5.2.4-2ubuntu5.2 and Apache 2)
Mac OS X 10.5.3 (PHP 5.2.5 (CLI) and Apache 2.2.8)

I'm running the official Drupal 5.9 release. I just took a peek at the 5.10 source and the function has not changed.

A Possible Solution

I made a small modification that fixes the problem and I haven't noticed any side affects yet. I have attached the patch for review. I'm not exactly sure why a return wasn't originally used, perhaps there is a special case that I'm not aware of?

CommentFileSizeAuthor
#13 drupal_tablesort_order-295430-13.patch4.31 KBjrchamp
#11 drupal_295430_11_tablesort.patch3.66 KBmiro_dietiker
#8 295430-8-tablesort-order-broken.patch1.94 KBcarlos8f
#6 drupal_295430_6_tablesort.patch1.73 KBmiro_dietiker
tablesort_default_order_fix.patch1.13 KBjmstacey

Comments

mariusilie’s picture

mariusilie commented

I don't know why but I found the same problem in drupal 6. The funny thing is that this only happend with "sort" => "desc". When I set "sort" => "asc", everything goes well.

This is weird...

[LATER EDIT]

I found out that this only happens when at least one of the table header data is an empty string

$headers = array(
array("data" => ""),
array("data" => "Title", "field" => "n.title", "sort" => "asc")
);
drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.9 » 6.x-dev
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Priority: Normal » Major

This also affects Drupal 7.

The source of the issue is that the tablesort_init calls both tablesort_get_order and tablesort_get_sort.
http://api.drupal.org/api/drupal/includes--tablesort.inc/function/tables...

Here: tablesort_get_sort returns the FIRST match in the header while tablesort_get_order returns the LAST match.
I consider this as a fatal broken UI API for both present 7.x and 6.x.

To make sure those two functions correspond to each other, we should make them acually build on identical logical checks / structure.

Status: Needs review » Needs work

The last submitted patch, tablesort_default_order_fix.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Version: 6.x-dev » 7.x-dev

Moving this to D7 queue as we should fix issues there first.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new drupal_295430_6_tablesort.patch1.73 KB

Submitting a first try for the issue from my side.

I think this is much more consistent, although we should rework the API. However it's too late for 7.x.

bojanz’s picture

bojanz commented
Status: Needs review » Needs work

There's a good cleanup patch in #97293: Tablesort cleanup.
Sadly, it didn't get in 7.x.

The change makes sense.

I don't like this comment though:

+  // User has not specified an order. Use first if specified; (same as sort).

A comment shouldn't make the code followed by it less understandable ;)
Either lose it (don't see a particular need for it), or reword the "Use first if specified; (same as sort)." part (maybe to something like "Use the first 'sort' specified.")

carlos8f’s picture

carlos8f commented
Status: Needs work » Needs review
StatusFileSize
new 295430-8-tablesort-order-broken.patch1.94 KB

Didn't test the patch, but rerolled to improve comments:

  1. Determine if the column to order by is in $_GET.
  2. If the 'sort' key is defined in the header, respect the first instance.
  3. Otherwise, default to ordering by the first column.
bojanz’s picture

bojanz commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Great! The comments are now awesome.

Forgot to mention that we should probably put in a few tests, the fact that we are fixing a bug as stupid as this proves that we need them.

jrchamp’s picture

jrchamp commented

#839556: Fix isset regression in tablesort, add tests, and cleanup theme_process_registry() fixes the sorting, with tests. This is probably duplicate. Please test with that patch to see if it resolves your issue.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new drupal_295430_11_tablesort.patch3.66 KB

I tried to produce a reliable sample. Pity i don't have time now to produce a test.

This is what i setup as header in simplenews module to test:

  $header = array(
    'mail' => array('data' => t('Email'), 'field' => 'sn.mail', 'sort' => 'asc'),
    'username' => array('data' => t('Username'), 'field' => 'u.name'),
    'status' => array('data' => t('Status'), 'field' => 'sn.activated', 'sort' => 'desc'),
    'language' => array('data' => t('Language'), 'field' => 'ss.language'),
    'operations' => array('data' => t('Operations')),
  );

As of my understanding, if no sort provided, the first column providing a sort should be considered with the right sort order.

Original core takes the SECOND (status) column, ordered by the FIRST (asc) sort order.
Patch #839556 still takes the SECOND (status) column, ordered by the SECOND (desc) sort order. This is better already.

Our patch changes to the first column with the right order.
To my surprise all this code is existing at two locations in the file and i needed to adjust it at a second location to make my sample work.

Update attached.

Status: Needs review » Needs work

The last submitted patch, drupal_295430_11_tablesort.patch, failed testing.

jrchamp’s picture

jrchamp commented
Status: Needs work » Needs review
StatusFileSize
new drupal_tablesort_order-295430-13.patch4.31 KB

#11 fails when the sort column is status because it picks mail. I have attached a patch which contains the patch from #985836: tablesort.inc contains a load of duplicated code to remove duplicated code from tablesort and the tablesort portions from #839556: Fix isset regression in tablesort, add tests, and cleanup theme_process_registry(). Once #839556 goes in, #985836 can go and this patch will simply be reduced to a cleanup/bugfix. The problem here is the default is being overridden. Returning immediately is not the correct solution because it skips over the possibility that another header has been chosen to sort by.

Setting to needs review so that the testbot can play.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Right. I did miss that...
Looks fine to me, untested.

jrchamp’s picture

jrchamp commented
Status: Needs review » Fixed
Issue tags: -Needs tests

@miro_dietiker: Patch #839556 went in over the weekend with the changes from this issue included (as the patches on both issues were related to the sorting brokenness and adding tests). Please let me know if this issue is now resolved for you in git (both 7.x and 8.x head).

Status: Fixed » Closed (fixed)

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

Blooniverse’s picture

Blooniverse commented
Status: Closed (fixed) » Needs work

... default sorting tables still doesn't work as supposed. Please see #1833746: Undefined index: form_build_id in ajax_get_form()

Blooniverse’s picture

Blooniverse commented
Status: Needs work » Closed (won't fix)

... changing status due to #97293: Tablesort cleanup

David_Rothstein’s picture

David_Rothstein commented
Status: Closed (won't fix) » Closed (fixed)

The original issue here is actually fixed, per #15. Reopen with details if that's not the case.