Tablesort cleanup

This also seems to be a problem in 4.7, unless I'm misunderstanding something. Currently, sorting of tables on my 4.7-based site breaks as soon as the column titles are translated.

These tables are generated by Views module, so just figuring out a way to apply t() to the column titles required a bit of work... and now sorting seems to break.

The following code from tablesort_header seems relevant:

$cell['data'] = l($cell['data'] . $image, $_GET['q'], array('title' => $title), 'sort='. $ts['sort'] .'&order='. urlencode($cell['data']) . $ts['query_string'], NULL, FALSE, TRUE); 

As a workaround, I may have to incorporate this code -- with slight modifications that will generate the link based on the field name rather than the translated label -- into a theme override.

Anyone with more in-depth understanding of the whole system have thoughts?

I'm not sure how others feel, but I'm not always too keen on letting my database field names be viewed by the public, as its a potentially (although pretty small) security risk (IMOH).

I may be too paranoid here, & in most situations it may be ok to use, but I can see circumstances where it could be an issue & this is possibly why the title was chosen as the sort parameter id? (just a guess)

Perhaps it could default to using either the 'field' (as Arancaytar suggests) or the 'data' (current default behaviour) info by default, and add an optional argument along the lines of 'sort_var' that could be used to force a specific value to use in the 'order=' clause.

Hi!

This is still open in the 6.2 release. Maybe the API documentation needs a change?

I spent some time walking through the source code to find out why theme ('table', ...) does not work as expected.

Regarding the comments about sort_var and exposing internal database column names:

Is the "field" attribute of the table's $header array used otherwise than being the sort variable for theme_table()?

According to the api docs for theme_table (for D5 & D6):

$header An array containing the table headers. Each element of the array can be either a localized string or an associative array with the following keys:

* "data": The localized title of the table column.
* "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").
* Any HTML attributes, such as "colspan", to apply to the column header cell.

But if you look at tablesort_header(), we see:

  $cell['data'] = l($cell['data'] . $image, $_GET['q'], array('title' => $title), 'sort='. $ts['sort'] .'&order='. urlencode($cell['data']) . $ts['query_string'], NULL, FALSE, TRUE);

The problem is: urlencode($cell['data']). According to the API docs, that should be: urlencode($cell['field'])- atleast, as far as I can understand it. This seems true for D6 as well.

Supplied patch is for D5.

And here's a patch for D6 head. I haven't tried it out, but should be fine.

I assume the bug is also in Drupal 7.x, so it must be fixed there.

Also:

• Please learn how to do a proper patch (unified diff): http://drupal.org/patch/create
• We also need to modify tablesort_get_order() in order for it to understand the new meaning of the "order" argument
• While we are at it, some cleanup of tablesort.inc will be rather welcome, as the code is really ugly (for example: $ts['sql'] is not SQL, it is expected to be a field name, but tablesort_sql() sanitize it anyway, etc.).

@Arancaytar: I agree completely.

Please do now. :)

First pass.

And this time including all changes.

oh boy, someone should have told me how horrible Forum module really is...

This one will pass.

Anyone up for reviewing and RTBC'ing?

I don't have much experience with the tablesort API, so I'd like somebody else to sign off on this, but here are some comments.

+    if (!empty($ts['field'])) {
+      // Remove table prefix from sorting field.
       // Based on code from db_escape_table(), but this can also contain a dot.
-      $field = preg_replace('/[^A-Za-z0-9_.]+/', '', $ts['sql']);
+      $field = preg_replace('/[^A-Za-z0-9_.]+/', '', $ts['field']);

I don't understand the added comment. AFAICT table prefixes are not stripped.

+    if (!is_array($column) || !isset($column['field'])) {
+      continue;
+    }
+    // Use the header column matching the URL parameter.
+    if ($order == $column['field']) {
+      return array('name' => $column['data'], 'field' => $column['field']);
+    }
+    // In case no header column will match the URL parameter, and this column
+    // defines 'sort', it is supposed to be the default sorting column.
+    if (isset($column['sort']) && ($column['sort'] == 'asc' || $column['sort'] == 'desc')) {
+      $default_column = array('name' => $column['data'], 'field' => $column['field']);
+    }

Doesn't this overwrite $default_column in each iteration, so it ends up referring to the last column with a valid sort attribute? If yes, is this intended? The first column would be a better default.

Do we need to check for isset($column['data']) as well?

+  // If we end up here, then a table header did not define any valid tablesort
+  // data.
+  throw new Exception(t('Invalid TableSort data; header needs to define at least one sorting field.'));
+  return array('name' => '', 'field' => '');

The return is never reached. Though I personally like that Drupal complains loudly in case it detects a developer mistake, it seems that the current convention is not silently ignore cases like these.

+        $ts = tablesort_init($header);

It's a terrible variable name (I know you didn't introduce it).

-  foreach ($headers as $header) {
...
+  foreach ($header as $column) {

I think the variable name $column describes the variable better than $header, but if you change it, I think you should change $headers to $columns. AFAICT $headers is an array of $header variables, so a simple plural form seems like the obvious choice.

Some other clean-up ideas (possibly outside the scope of this issue):
How about renaming tablesort_init() to e.g. tablesort_get_parameters() - the function simply returns stuff and does not initialize anything. Likewise for TableSort::init().

How about killing tablesort_get_sort(), tablesort_get_order() as separate functions and instead just move them inside tablesort_init()? It seems that you rarely need these values alone without the rest of the array returned by tablesort_init(). Likewise for their counterpart methods in TableSort, getSort()/order()/init().

tablesort_header() and tablesort_cell() take an array as argument (by value), modifies it and returns it. It seems more in line with Drupal standards to accept the argument by reference and just modify the original array, i.e. change tablesort_foo($array) to tablesort_foo(&$array).

I don't understand the added comment. AFAICT table prefixes are not stripped.

That was contained before already. I'm not sure whether it will work without that, but let's see. :)

Doesn't this overwrite $default_column in each iteration, so it ends up referring to the last column with a valid sort attribute? If yes, is this intended? The first column would be a better default.

Do we need to check for isset($column['data']) as well?

1) No. As the comment explains, and as explained elsewhere in API docs, only the default sorting column in the table's $header should define 'sort'. All other columns do not specify a sort.

2) Since we explicitly check that the column in $header is an array, 'data' must be set. @see theme_table()

The return is never reached. Though I personally like that Drupal complains loudly in case it detects a developer mistake, it seems that the current convention is not silently ignore cases like these.

I agree on both, and with the new exceptions, we should start to think about DX. Removed the return.

I think the variable name $column describes the variable better than $header, but if you change it, I think you should change $headers to $columns. AFAICT $headers is an array of $header variables, so a simple plural form seems like the obvious choice.

Nope, $header is documented in theme_table() and follows the structure documented in there. It is the same $header, so we keep that variable name.

The other clean-ups you mentioned we should defer to another issue. This patch is large enough. :)

I don't understand the added comment. AFAICT table prefixes are not stripped.

That was contained before already. I'm not sure whether it will work without that, but let's see. :)

I was only referring to the additional comment you added (// Remove table prefix from sorting field.), not the code itself. Now that you removed the preg_replace() call, it seems that the user-provided string ends up unescaped in the SQL query.

2) Since we explicitly check that the column in $header is an array, 'data' must be set. @see theme_table()

I'm not sure I follow you. The documentation for theme_table() says this about $header:

 * @param $header
 *   An array containing the table headers. Each element of the array can be
 *   either a localized string or an associative array with the following keys:
 *   - "data": The localized title of the table column.
 *   - "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").
 *   - Any HTML attributes, such as "colspan", to apply to the column header cell.

I'm not sure whether this means that all the mentioned keys are required when passing an array. It doesn't say so very explicitly. At least the code seems to check for "field" and "sort" using isset(), but not for "data".

Nope, $header is documented in theme_table() and follows the structure documented in there. It is the same $header, so we keep that variable name.

I still think it's a bad name and I think we should change theme_table() :-) But fair enough, we can deal with that later. It's just an internal variable name, so we can change it without breaking any APIs.

This patch was at 99% and only hold off by nitpicks. :(

Two other issues made me revisit this issue:

#615822: Benchmarking / profiling of D7
#602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable

Due to the latter patch, I suspect that this patch will no longer apply, but let's see what the bot thinks.

subscribing

subscribing

For reference #664042: TableSort order error when no sort set in header affects this issue if it gets in this patch may have to be rerolled.

CNR for bot run of #25. Is this still on the table for D7?

Re-rolled against HEAD. My main interest is to get includes/tablesort.inc out of the regular full bootstrap request path. It's needlessly loaded, on each and every single page.

#30: drupal.tablesort.30.patch queued for re-testing.

Not sure why the locale test is failing... Let's ask the bot one more time.

Huge +1 for this. I've been looking at the tablesort code while implementing #885014: Add pager and tablesort to EntityFieldQuery, and noticed the problems described...
Reviewed the patch, looks excellent.

#30: drupal.tablesort.30.patch queued for re-testing.

Gave this another review, the fixes are awesome and really needed.
I don't like the fact that we need to require_once tablesort every time we plan on using it, but I guess that's unavoidable (on the other hand, including it on every page even when not used is completely stupid)

+++ modules/forum/forum.module	25 Sep 2010 14:33:52 -0000
@@ -1058,20 +1044,20 @@ function template_preprocess_forum_list(
+  // Render the table header; this is required, because the forum topic list
+  // table does not use theme_table().
+  require_once DRUPAL_ROOT . '/includes/tablesort.inc';

Note that it's only Forum module that needs to load tablesort.inc manually. As the comment tries to explain, Forum module does not use theme_table(), and on top of that, it tries to output a sortable table also for a forum container, even when there are no topics (since otherwise forum_get_topics() would have been invoked, in which the select query is extended with TableSort, which in turn autoloads tablesort.inc), and therefore Forum module has to load tablesort.inc manually.

That's a total edge-case, and should be fixed in a completely different way: Update Forum module to make sense.

Powered by Dreditor.

Thanks for the clarification. Let's get this in!

#30: drupal.tablesort.30.patch queued for re-testing.

Just because someone is upset doesn't mean we have to mark something as won't fix. Someone else is more than welcome to pick up work on a ticket.

Although badly needed, this sounds like D8 material to me.

Can we ask Drieschick first?
If there are parts that are D8, maybe they can be postponed, and the other chunks can get in.
It's a good cleanup and a simple fix to an annoying bug. I actually see nothing controversial about it.

I'm prepared to push this forward (even if it gets moved to D8 in the end).

And... This is definitely off the table :/

Referencing back to a case addressing 7.x minimal bugfixing for tablesort functions.
#295430: Default sort using tablesort_get_order() is broken.

#30: drupal.tablesort.30.patch queued for re-testing.

Here's a re-roll of #30 that applies cleanly, but now has failing unit tests that need to be updated to work with the cleaned up API.

Subscribing

It's really annoying.

The patch from comment #50 touches a lot of files. Attached is a tighter D7.9 fix isolated to tablesort.inc and entity.inc.

Changelog:

Use field to determine sort order: Rewrite tablesort.inc:tablesort_get_order()

• Compare header[field] to _GET[order] (instead of data)
• Remove ts[name] - not needed
• Rename ts[sql] to ts[field] - more internally consistent and not exposed to callers

Decorate column header with sort indicator and URL based on field: Rewrite tablesort.inc:tablesort_header()

• Compare cell[field] to ts[field] (instead of data)
• Use ts[field] instead of ts[data] to construct column click sort query params

Decorate cell based on column field: Rewrite tablesort.inc:tablesort_cell()

• Compare header[field] to ts[field] (instead of data)

Determine sort direction based on field: Rewrite tablesort.inc:tablesort_get_sort()

• Use header[field] and ts[field (instead of data)

Query database using field: Rewrite TableSort::orderByHeader()

• Use ts[field] instead of ts[sql]

Fix callers
entity.inc: EntityFieldQuery::tableSort()

• Rename $order to $ts for code consistency
• Compare header[field] to ts[field]

Please be gentle this is my first patch and I've only been coding in anger w/ Drupal for about 3 weeks.

The are a few issues I needed to handle in addition to using [field] in the sort URL:

1. Decouple the field ID and database query column
2. Allow sort order to be reversed (ORDER BY ASC appears with decending indicator)

I've added support for this with the attached D7.9 patch using two new header column attributes: query_field and query_reverse.

As an example of #1, consider the scenario where a item date is stored in a database datetime column, however you would like to show the date and time in two separate table columns. You might use the following $header definition

$header = array(
  array('data' =>'Date','field' => 'date', 'query_field' => 'post_datetime'),
  array('data' =>'Time','field' => 'time', 'query_field' => 'post_datetime'),
);

For these two columns, the URL will be ?order=date and ?order=time, however on the backend TableSort extension is adding an ORDER BY post_datetime clause to the SQL query.

#2 should be self explanatory, just add 'query_reverse' => TRUE

See attached patch.

Re-roll.

Fix whitespace.

Anyone care to revive this for D8? Still worthwhile.

reroll

Fixed some tests and docs.

Why can't the indexes in $header be used for identifying the columns? That way, we can actually allow sorting on expressions like "CAST(fieldwronglyimplementedastext AS UNSIGNED)", "fielda + fieldb" and other cool stuff.

User submitted field names is a potential security risk since it can expose the order for fields that aren't supposed to be in the query.

Closely related #1876718: Allow modules to inject columns into tables more easily

#60: 97293-60.patch queued for re-testing.

Needs reroll
https://drupal.org/patch/reroll

As I've said before, that patch (and the current behaviour) is a potential security risk.

Example from my elearning experience:

A list of students that shows who has turned in an assignment. If a student figures out the column name for the grades (for example by figuring out which module I use), he/she can manipulate the url to sort by grade!

Never mind, I was mistaken.

Re-rolled

I wonder if we should have a service that defines how to mangle URLs / queries for sorting (and paging also?)

Rerroll

This is serious BC break but it makes sense

Just stumbled on this again. I think it's really weird that Drupal introduces the field content into the URL - especially with localization. It can contain weird Chinese letters etc. and in your code you have to check for all those manually. This makes the Drupal sortable table render element far less useful. I was even more surprised to find out that you cannot even override the string that is actually used for the query manually. The code seems to be now in core/lib/Drupal/Core/Utility/TableSort.php. Link::createFromRoute has 'order' => $cell_content which causes the problem.