Problem/Motivation

template_preprocess_table() processes $variables['header']. This consists of a structured array for each column header. The 'sort' value supposedly allows to set a default sort order for this column. This would be very convenient - simply set this to 'desc', and from now on, a click on that header will first sort the entries in descending order. This saves an extra page load in fields that are routinely sorted in descending order. However, instead of actually changing the default sorting direction for that field, it seems to set the current sort order to that field and that direction - as indicated by the little triangle next to the column header.

If this is intentional, the API page should probably be rewritten to make this clear. In that case, this is just a feature request for a default sorting order as described above.

Proposed resolution

Add support for initial_click_sort in the table render array.

Remaining tasks

User interface changes

None

API changes

Add support for initial_click_sort in the table render array.

Data model changes

None

Release notes snippet

Table click sorting can now be set to a descending sort initially.

CommentFileSizeAuthor
#107 109493-107.patch8.29 KBgnuget
#107 109493-103--tablesort_initial_sort--combined_strtolower--php7-107-interdiff.txt2.76 KBgnuget
#104 tables.tar_.gz1.01 KBgnuget
#4 tablesort.first_sort.patch2.13 KBdrunken monkey
#5 tablesort.first_sort.patch2.18 KBdrunken monkey
#6 tablesort.first_sort.patch2.31 KBdrunken monkey
#9 tablesort.first_sort.patch5.95 KBdrunken monkey
#16 tablesort_descending.patch6.68 KBrobertDouglass
#17 tablesort_descending.patch6.68 KBrobertDouglass
#18 tablesort_descending.patch6.63 KBrobertDouglass
#19 tablesort_descending.patch6.56 KBdrunken monkey
#23 tablesort_descending.patch8.61 KBdrunken monkey
#27 tablesort_descending.patch8.36 KBdrunken monkey
#29 tablesort_descending.patch8.37 KBdrunken monkey
#33 tablesort_descending.patch8.5 KBdrunken monkey
#34 109493-34.patch8.5 KBjhodgdon
#44 tablesort-descending-44.patch7.63 KBdrunken monkey
#55 109493-55.patch7.78 KBjibran
#56 tablesort-descending-56-d7.patch7.51 KBchaby
#71 109493-71.patch5.98 KBgnuget
#72 109493-71-72-interdiff.patch1.09 KBgnuget
#72 109493-72.patch6 KBgnuget
#78 109493-72-78-interdiff.txt8.64 KBgnuget
#78 109493-78.patch8.48 KBgnuget
#80 109493-80--tablesort_initial_sort--interdiff.txt2.26 KBdrunken monkey
#80 109493-80--tablesort_initial_sort.patch8.49 KBdrunken monkey
#86 109493-86--tablesort_initial_sort.patch7.77 KBaleevas
#87 109493-87--tablesort_initial_sort--interdiff.txt7.6 KBdrunken monkey
#87 109493-87--tablesort_initial_sort.patch11.13 KBdrunken monkey
#90 109493-90--tablesort_initial_sort--interdiff.txt3.28 KBdrunken monkey
#90 109493-90--tablesort_initial_sort--interdiff_to_86.txt4.32 KBdrunken monkey
#90 109493-90--tablesort_initial_sort.patch7.85 KBdrunken monkey
#100 interdiff_90_100.txt3.48 KBdhirendra.mishra
#100 109493-100--tablesort_initial_sort.patch7.87 KBdhirendra.mishra
#103 109493-103--tablesort_initial_sort--combined_strtolower--php7--interdiff_to_90.txt3.6 KBdrunken monkey
#103 109493-103--tablesort_initial_sort--combined_strtolower--php7--interdiff.txt1018 bytesdrunken monkey
#103 109493-103--tablesort_initial_sort--combined_strtolower--php7.patch7.76 KBdrunken monkey
#103 109493-103--tablesort_initial_sort--combined_strtolower--interdiff_to_90.txt3.67 KBdrunken monkey
#103 109493-103--tablesort_initial_sort--combined_strtolower--interdiff.txt1.35 KBdrunken monkey
#103 109493-103--tablesort_initial_sort--combined_strtolower.patch7.83 KBdrunken monkey
#103 109493-103--tablesort_initial_sort--interdiff.txt2.32 KBdrunken monkey
#103 109493-103--tablesort_initial_sort.patch7.85 KBdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cburschka’s picture

Title: theme_table uses default sorting order wrongly » Allow default sort direction in theme_table
Version: 5.x-dev » 7.x-dev
Category: bug » feature

I know now that this is by design, and I think it's also been documented somewhere. However, what I'm still missing is a way to reverse the default direction on a certain field.

Take a table that ranks popularity of a list of items. When someone clicks the Popularity column, they obviously expect the items to be sorted in descending order, because most would want to look at the popular end of the list first (also, in any given one-sided distribution, the maximum is a lot more narrow than the minimum, where you will likely end up with several thousands of 0s). This currently takes two clicks, as the table will first be sorted in ascending order. It's been a pet peeve for me for the past two years, and the subject of uncountable user complaints as well, on each of my sites that exposes sortable tables.

rupat’s picture

I have a little solution for this, should work in all drupal versions:

you have to add simply one line to /includes/tablesort.inc in function "tablesort_header"

change this

    else {
      // If the user clicks a different header, we want to sort ascending initially.
      $ts['sort'] = 'asc';
      $image = '';
    }

to this:

    else {
      // If the user clicks a different header, we want to sort ascending initially.
      //$ts['sort'] = 'asc';
      $ts['sort'] = (strpos($cell['data'],'<!--DESC-->') === FALSE) ? 'asc' : 'desc';
      $image = '';
    }

now drupal checks if the string "

" is included in the data field of the header of table.
so you have simply add a header like this to get it initially sorted descent

	// initial order desc
	$header[] = array('data'=>t('vote count<!--DESC-->'),'field'=>'votecount');
	// initial order asc
	$header[] = array('data'=>t('something else<!--DESC-->'),'field'=>'something');
effulgentsia’s picture

Title: Allow default sort direction in theme_table » tablesort should allow rendered tables to have columns that default to DESC when their header is clicked
Component: theme system » database system

I agree that the lack of support for this is unfortunate. Probably too late for feature requests in D7, but before changing version to D8, setting component to "database system" in case anyone monitoring that issue queue feels inspired to roll a patch. Clearly, this issue hasn't had much luck as part of the "theme system" queue.

drunken monkey’s picture

Component: database system » theme system
Status: Active » Needs review
Issue tags: +Needs documentation
FileSize
2.13 KB

I've tried to come up with something, since it's really not complicated (I hope). New doc comment:
- "first sort": The default sort order for this column, by which the column will be sorted after the first click.
- "sort": For one column, the order by which the table will be sorted by default (i.e., before the user explicitly sorts the table).
Only thing bothering me is the terminology, since "first sort" doesn't really sound right – but I think, "default sort" would be pretty confusing with the current "sort". Swapping those fields would be clearest, I'd say, but of course this would break about every table header definition out there.
So this patch just adds the requested possibility without breaking existing code.

Changing back to "theme system" as this doesn't concern the database at all (TableSort extender just uses whatever it finds in $_GET).

(Not sure about the tag, btw – is there any other documentation on this than the theme_table() doc comment?)

drunken monkey’s picture

FileSize
2.18 KB

Forgot part of the documentation comment (as always).
- "first sort": The default sort order for this column, by which the column will be sorted after the first click ("asc" or "desc"). Defaults to "asc".
- "sort": For one column, the order by which the table will be sorted by default (i.e., before the user explicitly sorts the table).

drunken monkey’s picture

FileSize
2.31 KB

I just realized an error – I forgot to unset the "first sort" key after processing, so the resulting tables contained invalid HTML. Which brings me to the conclusion that there should maybe be some tests for valid HTML.

In any case, here is a revised patch which is now hopefully completely correct.

moshe weitzman’s picture

Nice little improvement. Do we have any tables in core that can benefit from this? Would be good to add at least one so we demonstrate usage.

drunken monkey’s picture

There are a bunch. I think grep -r "'sort' => 'desc'" . gives a pretty good overview over some of them.
For example, nearly all tables in admin/reports are initially sorted descending by some field (date, timestamp, number of hits).
Of course, there may also be some fields which are not used for the default sort and which should be sorted descending when clicked – but these would be harder to find.

drunken monkey’s picture

FileSize
5.95 KB

Attached is a new patch, with use cases in the statistics module.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Thanks for the implementation in stats module

robertDouglass’s picture

I've wanted this feature for years. Huge UX improvement, especially if we can identify more tables.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ includes/theme.inc	17 Jul 2010 11:51:22 -0000
@@ -1548,7 +1548,11 @@ function theme_breadcrumb($variables) {
+ *     - "first sort": The default sort order for this column, by which the
+ *       column will be sorted after the first click ("asc" or "desc"). Defaults
+ *       to "asc".
+ *     - "sort": For one column, the order by which the table will be sorted by
+ *       default (i.e., before the user explicitly sorts the table).

This not really intuitive, IMO. I think we should try to come up with different names and better descriptions.

drunken monkey’s picture

I agree, these names don't really fit. But I don't think that "sort" can reasonably be changed (neither in name, nor in functionality), since this would mean that all sorted tables in core and contrib would have to be adapted to that. And with this premise, "first sort" was the best I could come up with for the other option, as "default sort" would maybe fit better, but be too confusing regarding the "sort" meaning.

I also can't come up with better descriptions, I find them pretty clear already. Maybe just 'The order by which the column will be sorted after the first click ("asc" or "desc"). Defaults to "asc".' for "first sort"?
Is there a tag for attracting the attention of people who are good at terminology and clarity of documentation?

robertDouglass’s picture

I'm rewriting the descriptions. Maybe "initial sort" might be a better key, but in the end, it's the descriptions that are going to make the difference. I agree we can't change "sort" at this point.

robertDouglass’s picture

One option for the "initial sort" is to make it a "descending" => TRUE attribute. Since the default sort order is ascending, you'd then add "descending" => TRUE any time you want to change the default behavior. This would have the benefits of a) not having to document the default behavior, and b) not wasting people's time writing "first sort" => "asc".

Here is some suggested documentation for the comments (retaining "first sort"):

* - "first sort": "asc" or "desc"; Determines the order of a column when its
* header is clicked for the first time. This attribute can be applied to
* multiple columns in a table. The default is "asc".
* - "sort": "asc" or "desc"; Determines the column and order of the table's
* sort when it is rendered for the first time. This attribute should only
* be assigned to one column in a table.

robertDouglass’s picture

Status: Needs work » Needs review
FileSize
6.68 KB

Implemented this with the "descending" => TRUE pattern and found two more columns in the statistics module where it made sense to use.

robertDouglass’s picture

FileSize
6.68 KB

Doesn't change anything from the last patch except the order of the code comments.

robertDouglass’s picture

FileSize
6.63 KB

Corrected and simplified code comments.

drunken monkey’s picture

FileSize
6.56 KB

Looks good to me. Using "descending" is a great idea, and the descriptions are also very clear (at least I think so).
Everything seems to work with this patch. I just altered a few minor things in comments and one code expression.
(Lowercase after semicolons, emphasis on "descending" only applying at first click (in code comment), empty() instead of isset() && $.)

Also, your patches have two "no status" files at the top, tablesort.first_sort_2.patch and sites/d7head.localhost.

robertDouglass’s picture

Status: Needs review » Reviewed & tested by the community

Let's have a committer take a look.

drunken monkey’s picture

Note: This would also fix #839634: Fix up documentation for table sorting, which should therefore be marked as "fixed" if this patch gets committed.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This patch looks like a good idea, but shouldn't this new behavior (first click is descending) have a test?

drunken monkey’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
8.61 KB

On closer inspection, sort link generation wasn't tested at all before. But now that there's a test (which hopefully also passes the test bot), can we please commit this?

Status: Needs review » Needs work

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

drunken monkey’s picture

That's what I get for incorporating a dangling whitespace issue …
New patch is created, but I think I'll wait until the end of Core Developer Summit. Right now the test bot has got enough to do as it is.

jhodgdon’s picture

If you have a new patch, please post it...

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
8.36 KB

OK.

jhodgdon’s picture

Hmmm.

-    array('data' => t('Timestamp'), 'field' => 'a.timestamp', 'sort' => 'desc'),
+    array('data' => t('Timestamp'), 'field' => 'a.timestamp', 'descending' => TRUE, 'sort' => 'desc'),

How does it make sense to set the descending flag on a particular cell as well as the sort flag?

Because it looks like the first time tablesort_header() is called, the descending flag goes away?

-    unset($cell['field'], $cell['sort']);
+    unset($cell['field'], $cell['sort'], $cell['descending']);

Something about this isn't making sense to me in the code. ???

And what's this "first sort" thing?

-    array('data' => t('Hits'), 'field' => 'hits', 'sort' => 'desc'),
+    array('data' => t('Hits'), 'field' => 'hits', 'first sort' => 'desc', 'sort' => 'desc'),
drunken monkey’s picture

FileSize
8.37 KB

> Something about this isn't making sense to me in the code. ???

As far as I can tell, tablesort_header is called only once per request, so the unsetting at the function's end has no influence on the effect of "descending" and "sort". And really, not specifying both when 'sort' => 'desc' is set doesn't usually make sense. At the moment it's like this:
First request => ORDER BY time DESC
Click on column "foo" => ORDER BY foo ASC
Click on column "time" => ORDER BY time ASC
With the patch, sorting on "time" after first changing the sort to something else would again give a descending sort – what is wanted and expected by the user most of the time, I think.

> And what's this "first sort" thing?

'first sort' => 'desc' was the first proposed way of doing 'descending' => TRUE – it being still in there is of course wrong, thanks for pointing it out.

Attached is a new patch fixing that last thing.

jhodgdon’s picture

That makes sense about setting both sort and descending...

But if tablesort_header() is only called once, what is the purpose of unsetting those array elements?

drunken monkey’s picture

After tablesort_header() is called, the array is used to build the table cell, with all remaining array elements (other than "data") being used as attributes. So if we wouldn't unset it, we'd have something like &lt;td field="time" sort="desc" descending="1"&gt;.

jhodgdon’s picture

Ah, that makes sense... Maybe a comment in the code explaining that would be helpful? :)

drunken monkey’s picture

OK.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.5 KB

I found one spot where coding standards weren't being followed:
array(
array(
item [not followed by comma]
)
)

So I edited the patch to put that in. With that added comma, I think it's all good, well-documented code and a needed feature. Let's see if we can get this in...

drunken monkey’s picture

Thanks for catching that!

sun’s picture

Issue tags: -Needs documentation

oh, awesome. Is this the cause for d.o sorting issues backwards by default?

sun’s picture

#34: 109493-34.patch queued for re-testing.

moshe weitzman’s picture

Yeah, code looks good to me too.

sun’s picture

Version: 7.x-dev » 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

drunken monkey’s picture

Version: 8.x-dev » 7.x-dev

I'm not sure, it may be. But since your comment lacks any real argumentation, I'm setting it back for now, waiting for other opinions. I don't think this would hurt that badly for D7.

robertDouglass’s picture

Rules or no, there have been other "nice to have" patches going into D7. Forcing core maintainers to do "X", whatever "X" is (ie stick to rules) is not fruitful business. Until Dries says that only criticals are going in, leave it D7. We can bump it to D8 in due time.

Dries’s picture

Version: 7.x-dev » 8.x-dev

I'm going to postpone this to Drupal 8. We survived Drupal 7 without this, I think we'll survive another release cycle without this feature. It's a feature request.

mlncn’s picture

Subscribing. Could this be one of those non-API-changing additions that goes into 8, and then goes into 7?

drunken monkey’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.63 KB

Seems to still apply, maybe we can now finally get this in.

jox’s picture

I think this is a great usability enhancement with not much overhead and no api consequences. I would love to see added to Drupal 7. Otherwise I (and others) will always have to patch it in on each core update, which is a pain (especially if those core patches sum up)...

drunken monkey’s picture

Hm, I'm not sure whether you can really say that it has „no API consequences“ … After all, it does add another key to an array in an API function.
Bug fixes like #838096: Problem with the "active" class of tablesort seem more pressing to me, with much the same argument of having to patch this manually.

Getting any of this in would be brilliant, though. These are the things that really suck in core work.

jox’s picture

Ok, it has consequences. But these are intentional and optional. I meant consequences for already existing code.

If you add functionality without changing the way a function is called and operates otherwise, then there is no "api change" in this sense. In other words, this will not possibly affect any code that uses that function in the way it is designated.

Besides that, to be precise, it's not quite true that it "does add another key to an array in an API function". It is considering an additional key of a passed array.

KingMoore’s picture

I really hope there is some way this can be added to 7.

Just spent a couple of hours trying to figure out why when my header field was set to sort=>DESC it sorts by ASC when clicked. IMO the current extend('TableSort')->orderByHeader($header_row) documentation as is is very confusing, and leads me to believe I should have the functionality of above patch, but in reality I do not.

As commenters above noted, there are lots of cases where we do not want two clicks required to get a DESC sort order. This is a big enough deal for me to consider abandoning extend('TableSort')->orderByHeader($header_row) in favor of a 3rd party table script. However, I would much rather use built in Drupal functionality.

jox’s picture

You'll survive it.

(Sorry, couldn't resist...)

arnoldbird’s picture

I'm a little surprised that this was marked a feature request and not a bug. The documentation and some parts of the code create the expectation that this feature already exists and will work, so it seems like more of a bug to me.

See the docs here: http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_table/7
I'm not sure what the 'sort' key in the header array would be for, if not for setting a default sort direction.

KingMoore’s picture

^ +1

drunken monkey’s picture

I'm not sure what the 'sort' key in the header array would be for, if not for setting a default sort direction.

It defines the one column by which the table will be sorted by default. But yes, the documentation is very misleading. See #839634: Fix up documentation for table sorting.

jibran’s picture

#44: tablesort-descending-44.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, tablesort-descending-44.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
7.78 KB

Re-roll of #44

chaby’s picture

Re-roll of #55 and reviving this issue !
However, regarding to the last comment date, I fear that it won't never be applied to the d7 branch... ?

Status: Needs review » Needs work

The last submitted patch, tablesort-descending-56-d7.patch, failed testing.

chaby’s picture

Version: 8.x-dev » 7.x-dev
chaby’s picture

Status: Needs work » Needs review

#56: tablesort-descending-56-d7.patch queued for re-testing.

drunken monkey’s picture

Version: 7.x-dev » 8.x-dev

As per #42, this should be added to D8, and D7 is probably really out of the question. I wouldn't get my hopes up for D8 either, though.

drunken monkey’s picture

#55: 109493-55.patch queued for re-testing.

pounard’s picture

#60 I would leave the choice of commiting to Drupal 7 this to official Drupal 7 maintener instead, even thought it's a feature, it's a useful feature, not extensive and with no BC break. It would actually be a plus to have it in both Drupal 8 and Drupal 7.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 56: tablesort-descending-56-d7.patch, failed testing.

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes

Moving to 8.1.x because this feature didn't get in to 8.0.x

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gnuget’s picture

Status: Needs work » Needs review
FileSize
5.98 KB

I needed this today and it seems that there is not a D8 version of the patch so I made it.

Patch attached and I added a few tests.

David.

gnuget’s picture

I found a typo in my last patch.

New version attached.

The last submitted patch, 72: 109493-71-72-interdiff.patch, failed testing. View results

The last submitted patch, 71: 109493-71.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

This works! A tiny addition to give us long-requested functionality that confounds a developer every once in a while by not being there. A bit late for this issue's ten-year anniversary, but it'll be nice to have it in.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record
+++ b/core/includes/tablesort.inc
@@ -135,13 +137,21 @@ function tablesort_get_sort($headers) {
+  // if not use "asc".

+++ b/core/includes/theme.inc
@@ -822,6 +822,9 @@ function template_preprocess_image(&$variables) {
+ *     - "descending": Set to TRUE if the column should sort descending when its

none of the other params have ". Also I find the param name confusing. It's not helped by the fact that sort is only supposed to appear once. The better solution would be to allow sort to appear multiple times and introduce a new key of default_sort which identifies the column to use as the default sort. I think it might be possible to make it work this way.

But if we can't see a way to make it work like that then changing this to something better that might hint that it is about what happs on the first time you click would be good. I also think that rather than a bool it should be 'asc' and 'desc' so it's more descriptive as to what it going on. Maybe initial_click_sort or first_click_sort.

Also we should have change record to tell people about the new feature.

gnuget’s picture

none of the other params have ". Also I find the param name confusing. It's not helped by the fact that sort is only supposed to appear once. The better solution would be to allow sort to appear multiple times and introduce a new key of default_sort which identifies the column to use as the default sort. I think it might be possible to make it work this way.

I've been thinking about this approach and while I agree that this would be the best this might broke the backward compatibility and this was already discussed on #13 so I will continue with your second suggestion and I will use first_click_sort.

If you think that we can take the risk and change the current behaviour of sort let me know and I can provide a patch of your first suggestion

In my next comment a patch.

--
David

gnuget’s picture

New patch attached, the parameter now accepts asc and desc and it is named initial_click_sort

I added two extra tests as well.

gnuget’s picture

I created the change record.

https://www.drupal.org/node/2981313

drunken monkey’s picture

Thanks a lot, great job! Maybe we can finally get this committed …
Patch looks pretty good, I'd suggest just a few minor changes, mostly code style. Plus, I think if we have a column match in tablesort_get_sort(), there's no point in continuing the loop? So, added a break there.
I've tested the patch manually, and it worked as expected. This would be RTBC for me.

The change record also looks great, thanks!

gnuget’s picture

Status: Needs review » Reviewed & tested by the community

The changes looks great, thanks!

Marking it as RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 80: 109493-80--tablesort_initial_sort.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community

Seems like an unrelated random failure so moving back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Unfortunately 8.7.x has had #3001201: Convert tablesort.inc functions to a static class committed. So this needs a pretty major re-roll.

aleevas’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.77 KB

I re-did the patch #80 according to the new changes in #3001201: Convert tablesort.inc functions to a static class

drunken monkey’s picture

Thanks a lot for keeping this going!
I just found two small code style problems, fixed in the attached patch.
Also, I figured we definitely want to default to descending sort for clicks on those columns that are already set as descending by default? (I.e., when first going to table X, it is sorted by column A descending. When you now click-sort on column B, the default for a click on A should again be “descending”.) Therefore, I searched the code base for 'sort' => 'desc' and added 'initial_click_sort' => 'desc' to all instances outside of tests. (Can be easily reverted if we decide this needs further deliberation in a separate issue.)

alexpott’s picture

Also, I figured we definitely want to default to descending sort for clicks on those columns that are already set as descending by default? (I.e., when first going to table X, it is sorted by column A descending. When you now click-sort on column B, the default for a click on A should again be “descending”.)

Couldn't we make this behaviour the default behaviour without having to add initial_click_sort - ie. have the code add it for us.

alexpott’s picture

Plus the scenario of clicking back on a default desc sorted column ought to be tests. That said all of the returning to a default sorted column could be a followup as this behaviour is not changed by this patch. And the fact that it could be a follow-up means it probably should be.

drunken monkey’s picture

alexpott’s picture

Issue tags: +Needs followup

Thanks @drunken monkey - let's file a follow-up to address #87

drunken monkey’s picture

Issue tags: -Needs followup

Posted #3023617: Add "initial_click_sort" parameter to sortable tables.
(Side note: Just noticed, when linking to it, that this issue’s ID is actually six-digit! High time it got resolved …)

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DanChadwick’s picture

Interested in this feature, which is difficult to achieve otherwise.

drunken monkey’s picture

Interested in this feature, which is difficult to achieve otherwise.

Then help us get there, please!
Does the patch work for you? Does the code look good to you? If yes to both, please set to RTBC and let’s finally get this in!

gnuget’s picture

Status: Needs review » Reviewed & tested by the community

I ran the tests last week and all passed and reviewed again the patch and looks great.

Thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Utility/TableSort.php
    @@ -73,9 +73,16 @@ public static function header(&$cell_content, array &$cell_attributes, array $he
    +        // or 'desc'.
    +        if (isset($cell_attributes['initial_click_sort']) && in_array(strtolower($cell_attributes['initial_click_sort']), [self::ASC, self::DESC])) {
    +          $context['sort'] = strtolower($cell_attributes['initial_click_sort']);
    +        }
    

    We could do the strtolower() once here. I had a read a couple of times to see why we were strtolower() the same thing.

    if (isset($cell_attributes['initial_click_sort'])) {
      $initial_click_sort = strtolower($cell_attributes['initial_click_sort']);
      if (in_array($initial_click_sort, [self::ASC, self::DESC]) {
        $context['sort'] = $initial_click_sort;
      }
    }
    else {
    
  2. (these two could be fixed on commit, but spotted while reviewing)

    +++ b/core/tests/Drupal/KernelTests/Core/Render/Element/TableSortExtenderTest.php
    @@ -136,7 +136,103 @@ public function testTableSortInit() {
    +    // parameter is defined as well the sort parameter should be has precedence.
    ...
    

    nit: should have precedence. could maybe have commas before 'and the', and 'the sort'.

+++ b/core/tests/Drupal/KernelTests/Core/Render/Element/TableSortExtenderTest.php
@@ -136,7 +136,103 @@ public function testTableSortInit() {
+    // Test that if the initial_click_sort parameter  is defined and the value

Nit: double space here.

DanChadwick’s picture

Holy optimizing the leap year interrupt. On a 12 year old issue, no less.

dhirendra.mishra’s picture

Assigned: Unassigned » dhirendra.mishra

Let me fix the issue reported above.

dhirendra.mishra’s picture

Assigned: dhirendra.mishra » Unassigned
Status: Needs work » Needs review
FileSize
3.48 KB
7.87 KB

Here is the updated patch.

Status: Needs review » Needs work

The last submitted patch, 100: 109493-100--tablesort_initial_sort.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DanChadwick’s picture

#100 is incorrect because it access a potentially nonexistent array element without first checking to see if it exists. See the proposed fix in #97 or omit this change as the original code is functionally correct.

Increasing the code size and complexity to avoid a redundant call is strtolower() is, in my opinion, unwise. We are literally talking about one call to a fast function called with a short string in a non-typical case, per table. The new code is longer and I'm not sure that the opcodes are efficient. If indeed the change from #97 is actually more efficient, it could never, ever be measured, even with the best instrumentation, under any real-world server.

I would not make this change. The code in #90 has already been reviewed and found correct. Volunteer time is a resource to be conserved. I suggest we simply make the code comments changes and try to get this committed before Drupal 25 comes out.

drunken monkey’s picture

I agree with #102, but really don’t care anymore. At this point, I just want to get the patch in and be finally done with this issue.
So, attached are three suggested patches which should all work well. Whichever Core committer next comes this way can just pick the one they prefer:

  1. The first (tablesort_initial_sort – interdiff compared to #90) just fixes the comments.
  2. The second (tablesort_initial_sort--combined_strtolower – interdiff compared to the first) implements the suggested strtolower() refactoring.
  3. The third (tablesort_initial_sort--combined_strtolower--php7 – interdiff compared to the second) uses the PHP 7 ?? operator (allowed at this point, I believe?) to make this check more elegant and avoid the double nesting.

I’ve also included *--interdiff_to_90.txt interdiffs that compare to #90 instead of the previous patch variant, to give you more options for reviewing the differences (depending on the patch you prefer).

Would be great if someone could (after the test bot has assented) give all of these a whirl, confirm they still all work for them (and the code now looks good) and then set to RTBC again.

gnuget’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.01 KB

All the 3 patches are working great!

This was a bit more complicated to test than expected so I wrote a small module to make it easier.

Basically it is just necessary to install the module and visit /table and that's it.

Module attached.

Thanks for all the work on this!

(Drupal changed the extension for security, the original extension is tar.gz)

alexpott’s picture

+++ b/core/lib/Drupal/Core/Utility/TableSort.php
@@ -73,9 +73,15 @@ public static function header(&$cell_content, array &$cell_attributes, array $he
         $context['sort'] = self::ASC;
+        $initial_click_sort = strtolower($cell_attributes['initial_click_sort'] ?? '');
+        if (in_array($initial_click_sort, [self::ASC, self::DESC])) {
+          $context['sort'] = $initial_click_sort;
+        }

Given that this is not user input are we sure we need the validation and manipulation here? This could be
$context['sort'] = $cell_attributes['initial_click_sort'] ?? self::ASC; and the sort value is later checked to be valid when the link is clicked on. In fact this kinda of manipulation could result in hiding things from developers. If we did the above code and we used some funny_value at least this would appear on the link. And then when you click on the link the following code would result

    $query = $request->query;
    if ($query->has('sort')) {
      return (strtolower($query->get('sort')) == self::DESC) ? self::DESC : self::ASC;
    }

in an ascending sort and we should never assume the $query->get('sort') is a valid value because a user can manipulate that.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Utility/TableSort.php
    @@ -73,9 +73,15 @@ public static function header(&$cell_content, array &$cell_attributes, array $he
             $context['sort'] = self::ASC;
    +        $initial_click_sort = strtolower($cell_attributes['initial_click_sort'] ?? '');
    +        if (in_array($initial_click_sort, [self::ASC, self::DESC])) {
    +          $context['sort'] = $initial_click_sort;
    +        }
    

    So what I'm saying is that this can be

    $context['sort'] = $cell_attributes['initial_click_sort'] ?? self::ASC;
    
  2. +++ b/core/lib/Drupal/Core/Utility/TableSort.php
    @@ -150,8 +156,13 @@ public static function getSort(array $headers, Request $request) {
    +        if (isset($header['initial_click_sort'])) {
    +          return (strtolower($header['initial_click_sort']) == self::DESC) ? self::DESC : self::ASC;
    +        }
    

    This can be

    if (isset($header['initial_click_sort'])) {
      return $header['initial_click_sort'];
    }
    
  3. Note if you make the above changes core/tests/Drupal/KernelTests/Core/Render/Element/TableSortExtenderTest.php does not fail - so if the strtolower and default value stuff is required then it is untested.
gnuget’s picture

Thanks for your review and suggestions Alexpott.

I addressed 1 and 2 and added a test to make explicit that if a non-expected value is passed to the initial_click_sort it is respected and passed to the sort param in the URL. I'm not sure if I should also leave a comment letting the user know that the even if the sort value is unexpected later is changed to asc when getSort() is executed.

If you think that I should add that to the comment let me know.

Patch attached.

drunken monkey’s picture

Status: Needs review » Reviewed & tested by the community

Alright, makes sense and the change looks good to me. RTBC once again from me!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c0881f9 and pushed to 8.8.x. Thanks!

As #7/#8 point out there are core tables that might benefit from this or maybe views should be able to leverage this so let's create some follow-ups for that.

Credited @DanChadwick. @alexpott, @catch, @Dries for their code reviews.

diff --git a/core/tests/Drupal/KernelTests/Core/Render/Element/TableSortExtenderTest.php b/core/tests/Drupal/KernelTests/Core/Render/Element/TableSortExtenderTest.php
index 08b4b6271a..2a7ea0410c 100644
--- a/core/tests/Drupal/KernelTests/Core/Render/Element/TableSortExtenderTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Render/Element/TableSortExtenderTest.php
@@ -252,4 +252,5 @@ public function testTableSortInit() {
     $this->verbose(strtr('$ts: <pre>!ts</pre>', ['!ts' => Html::escape(var_export($ts, TRUE))]));
     $this->assertEquals($expected_ts, $ts, 'Complex table headers with the initial_click_sort set as foo are sorted correctly.');
   }
+
 }

Fixed coding standards on commit.

  • alexpott committed c0881f9 on 8.8.x
    Issue #109493 by drunken monkey, gnuget, robertDouglass, dhirendra....
alexpott’s picture

Issue summary: View changes
Issue tags: +8.8.0 release notes
alexpott’s picture

Issue summary: View changes
xjm’s picture

Status: Fixed » Needs work

The release note and change record here seem to describe a feature addition, not a disruptive change. If there are disruptive impacts from this issue, the release note and change record should be updated to describe those disruptive impacts.

Otherwise, the issue should be tagged for the 8.8.0 highlights instead. Thanks!

alexpott’s picture

Status: Needs work » Fixed
Issue tags: -8.8.0 release notes +8.8.0 highlights

Yep this is totally a feature addition. There's no BC implication.

Status: Fixed » Closed (fixed)

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