Posted by cburschka on November 13, 2006 at 9:48am
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | API clean-up, Framework Initiative |
Issue Summary
I noticed that, even though the API supports a "field" element for table columns, the URL will contain the "data" element as a sorting parameter.
As an example, if a header column is defined in the following way:
$header[]=array('data'=>t('Number'),'field'=>'id','sort'=>'asc');
The column head in the table is titled 'Number'. The query string in the link is "?sort=desc&order=Number".
"Number" is not supposed to be an identifying title of the field, that's what "id" is. In fact, if the site is localized to other languages, "Number" will change to other, unpredictable values.
From the documentation of the table API, it seems that it would make more sense if the query string contained "?sort=desc&order=id".
Comments
#1
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?
#2
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.
#3
Admittedly, my argument about localization doesn't really apply - if the page internally uses t("Number") to compare the get parameter to, that will work. Well, unless the site is localized to two different languages and one user tries to send someone with the other language setting a URL.
Perhaps sort_var really is the way to go.
#4
The sort_var addition still strikes me as a good idea. However, this would be an API change, so it will probably have to be delayed to 7.x.
------------
In a related issue, I have problems differentiating between "order" and "sort" every time I use this API. Intuitively, I think that you "sort" by the name in descending "order", but in fact "order" contains the field name and "sort" the direction. Since we're already in 7.x territory, perhaps the parameters could be renamed into something more clear - like "order_by" instead of "order", and possibly "d" for direction instead of "sort".
#5
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()?
#6
According to the api docs for theme_table (for D5 & D6):
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.
#7
And here's a patch for D6 head. I haven't tried it out, but should be fine.
#8
I assume the bug is also in Drupal 7.x, so it must be fixed there.
Also:
tablesort_get_order()in order for it to understand the new meaning of the "order" argument#9
Yes, indeed. And while we're at it, we could take the opportunity and improve the API in the following ways:
1.) Require an associative key in the
$headersarray. This key will be used as the URL parameter, which solves the problem by neither introducing localized text into the URL nor exposing the database schema.2.) Make
orderdetermine only the default direction for that field (for example desc for numbers), instead of marking that column as the default sorted field. Instead, the#sortproperty in the headers array will contain the key of the field sorted by by default.Does this sound good?
#10
@Arancaytar: I agree completely.
#11
#12
Please do now. :)
#13
First pass.
#14
The last submitted patch failed testing.
#15
And this time including all changes.
oh boy, someone should have told me how horrible Forum module really is...
#16
The last submitted patch failed testing.
#17
This one will pass.
Anyone up for reviewing and RTBC'ing?
#18
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']);
+ 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']);
+ }
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' => '');
+ $ts = tablesort_init($header);- foreach ($headers as $header) {...
+ foreach ($header as $column) {
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)totablesort_foo(&$array).#19
That was contained before already. I'm not sure whether it will work without that, but let's see. :)
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()
I agree on both, and with the new exceptions, we should start to think about DX. Removed the return.
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. :)
#20
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.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 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.
#21
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.
#22
The last submitted patch failed testing.
#23
subscribing
#24
subscribing
#25
This is definitely not done. This is simply an attempt to reroll the previous patch since alot has changed in the last month and a half. I'd work some more on it now, but I have to step away for a bit.
#26
Anyone that wants to play with it, feel free. I won't be able to for a while.
#27
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.
#28
CNR for bot run of #25. Is this still on the table for D7?
#29
The last submitted patch, 97293.patch, failed testing.
#30
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.
#31
The last submitted patch, drupal.tablesort.30.patch, failed testing.
#32
#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.
#33
The last submitted patch, drupal.tablesort.30.patch, failed testing.
#34
#30: drupal.tablesort.30.patch queued for re-testing.
#35
The last submitted patch, drupal.tablesort.30.patch, failed testing.
#36
#37
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)
#38
+++ 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.
#39
Thanks for the clarification. Let's get this in!
#40
#30: drupal.tablesort.30.patch queued for re-testing.
#41
#42
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.
#43
Although badly needed, this sounds like D8 material to me.
#44
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).
#45
And... This is definitely off the table :/
#46
Referencing back to a case addressing 7.x minimal bugfixing for tablesort functions.
#295430: Default sort using tablesort_get_order() is broken.
#47
#30: drupal.tablesort.30.patch queued for re-testing.
#48
The last submitted patch, drupal.tablesort.30.patch, failed testing.
#49
#50
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.
#51
Subscribing
It's really annoying.
#52
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()
Decorate column header with sort indicator and URL based on field: Rewrite tablesort.inc:tablesort_header()
Decorate cell based on column field: Rewrite tablesort.inc:tablesort_cell()
Determine sort direction based on field: Rewrite tablesort.inc:tablesort_get_sort()
Query database using field: Rewrite TableSort::orderByHeader()
Fix callers
entity.inc: EntityFieldQuery::tableSort()
Please be gentle this is my first patch and I've only been coding in anger w/ Drupal for about 3 weeks.
#53
The are a few issues I needed to handle in addition to using [field] in the sort URL:
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
<?php$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.