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.
Comments
Comment #1
cburschkaI 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.
Comment #2
rupat CreditAttribution: rupat commentedI 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
to this:
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
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #4
drunken monkeyI'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?)
Comment #5
drunken monkeyForgot 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).
Comment #6
drunken monkeyI 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.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedNice 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.
Comment #8
drunken monkeyThere 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.
Comment #9
drunken monkeyAttached is a new patch, with use cases in the statistics module.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good to me. Thanks for the implementation in stats module
Comment #11
robertDouglass CreditAttribution: robertDouglass commentedI've wanted this feature for years. Huge UX improvement, especially if we can identify more tables.
Comment #12
Dries CreditAttribution: Dries commentedThis not really intuitive, IMO. I think we should try to come up with different names and better descriptions.
Comment #13
drunken monkeyI 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?
Comment #14
robertDouglass CreditAttribution: robertDouglass commentedI'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.
Comment #15
robertDouglass CreditAttribution: robertDouglass commentedOne 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"):
Comment #16
robertDouglass CreditAttribution: robertDouglass commentedImplemented this with the "descending" => TRUE pattern and found two more columns in the statistics module where it made sense to use.
Comment #17
robertDouglass CreditAttribution: robertDouglass commentedDoesn't change anything from the last patch except the order of the code comments.
Comment #18
robertDouglass CreditAttribution: robertDouglass commentedCorrected and simplified code comments.
Comment #19
drunken monkeyLooks 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 ofisset() && $
.)Also, your patches have two "no status" files at the top,
tablesort.first_sort_2.patch
andsites/d7head.localhost
.Comment #20
robertDouglass CreditAttribution: robertDouglass commentedLet's have a committer take a look.
Comment #21
drunken monkeyNote: This would also fix #839634: Fix up documentation for table sorting, which should therefore be marked as "fixed" if this patch gets committed.
Comment #22
jhodgdonThis patch looks like a good idea, but shouldn't this new behavior (first click is descending) have a test?
Comment #23
drunken monkeyOn 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?
Comment #25
drunken monkeyThat'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.
Comment #26
jhodgdonIf you have a new patch, please post it...
Comment #27
drunken monkeyOK.
Comment #28
jhodgdonHmmm.
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?
Something about this isn't making sense to me in the code. ???
And what's this "first sort" thing?
Comment #29
drunken monkey> 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.
Comment #30
jhodgdonThat 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?
Comment #31
drunken monkeyAfter
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<td field="time" sort="desc" descending="1">
.Comment #32
jhodgdonAh, that makes sense... Maybe a comment in the code explaining that would be helpful? :)
Comment #33
drunken monkeyOK.
Comment #34
jhodgdonI 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...
Comment #35
drunken monkeyThanks for catching that!
Comment #36
sunoh, awesome. Is this the cause for d.o sorting issues backwards by default?
Comment #37
sun#34: 109493-34.patch queued for re-testing.
Comment #38
moshe weitzman CreditAttribution: moshe weitzman commentedYeah, code looks good to me too.
Comment #39
sunAlthough 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).
Comment #40
drunken monkeyI'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.
Comment #41
robertDouglass CreditAttribution: robertDouglass commentedRules 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.
Comment #42
Dries CreditAttribution: Dries commentedI'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.
Comment #43
mlncn CreditAttribution: mlncn commentedSubscribing. Could this be one of those non-API-changing additions that goes into 8, and then goes into 7?
Comment #44
drunken monkeySeems to still apply, maybe we can now finally get this in.
Comment #45
jox CreditAttribution: jox commentedI 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)...
Comment #46
drunken monkeyHm, 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.
Comment #47
jox CreditAttribution: jox commentedOk, 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.
Comment #48
KingMoore CreditAttribution: KingMoore commentedI 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.
Comment #49
jox CreditAttribution: jox commentedYou'll survive it.
(Sorry, couldn't resist...)
Comment #50
arnoldbird CreditAttribution: arnoldbird commentedI'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.
Comment #51
KingMoore CreditAttribution: KingMoore commented^ +1
Comment #52
drunken monkeyIt 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.
Comment #53
jibran#44: tablesort-descending-44.patch queued for re-testing.
Comment #55
jibranRe-roll of #44
Comment #56
chaby CreditAttribution: chaby commentedRe-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... ?
Comment #58
chaby CreditAttribution: chaby commentedComment #59
chaby CreditAttribution: chaby commented#56: tablesort-descending-56-d7.patch queued for re-testing.
Comment #60
drunken monkeyAs 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.
Comment #61
drunken monkey#55: 109493-55.patch queued for re-testing.
Comment #62
pounard#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.
Comment #63
jibran56: tablesort-descending-56-d7.patch queued for re-testing.
Comment #65
joelpittetMoving to 8.1.x because this feature didn't get in to 8.0.x
Comment #71
gnugetI 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.
Comment #72
gnugetI found a typo in my last patch.
New version attached.
Comment #75
mlncn CreditAttribution: mlncn at Agaric for Drutopia, Teachers with GUTS commentedThis 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.
Comment #76
alexpottnone 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
orfirst_click_sort
.Also we should have change record to tell people about the new feature.
Comment #77
gnugetI'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 suggestionIn my next comment a patch.
--
David
Comment #78
gnugetNew patch attached, the parameter now accepts
asc
anddesc
and it is namedinitial_click_sort
I added two extra tests as well.
Comment #79
gnugetI created the change record.
https://www.drupal.org/node/2981313
Comment #80
drunken monkeyThanks 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 abreak
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!
Comment #81
gnugetThe changes looks great, thanks!
Marking it as RTBC.
Comment #84
lauriiiSeems like an unrelated random failure so moving back to RTBC.
Comment #85
alexpottUnfortunately 8.7.x has had #3001201: Convert tablesort.inc functions to a static class committed. So this needs a pretty major re-roll.
Comment #86
aleevasI re-did the patch #80 according to the new changes in #3001201: Convert tablesort.inc functions to a static class
Comment #87
drunken monkeyThanks 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.)Comment #88
alexpottCouldn't we make this behaviour the default behaviour without having to add
initial_click_sort
- ie. have the code add it for us.Comment #89
alexpottPlus 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.
Comment #90
drunken monkeyAlright, makes sense.
Comment #91
alexpottThanks @drunken monkey - let's file a follow-up to address #87
Comment #92
drunken monkeyPosted #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 …)
Comment #94
DanChadwick CreditAttribution: DanChadwick commentedInterested in this feature, which is difficult to achieve otherwise.
Comment #95
drunken monkeyThen 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!
Comment #96
gnugetI ran the tests last week and all passed and reviewed again the patch and looks great.
Thanks!
Comment #97
catchWe could do the strtolower() once here. I had a read a couple of times to see why we were strtolower() the same thing.
(these two could be fixed on commit, but spotted while reviewing)
nit: should have precedence. could maybe have commas before 'and the', and 'the sort'.
Nit: double space here.
Comment #98
DanChadwick CreditAttribution: DanChadwick commentedHoly optimizing the leap year interrupt. On a 12 year old issue, no less.
Comment #99
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedLet me fix the issue reported above.
Comment #100
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedHere is the updated patch.
Comment #102
DanChadwick CreditAttribution: DanChadwick commented#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.
Comment #103
drunken monkeyI 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:
strtolower()
refactoring.??
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.
Comment #104
gnugetAll 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
)Comment #105
alexpottGiven 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 somefunny_value
at least this would appear on the link. And then when you click on the link the following code would resultin an ascending sort and we should never assume the
$query->get('sort')
is a valid value because a user can manipulate that.Comment #106
alexpottSo what I'm saying is that this can be
This can be
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.Comment #107
gnugetThanks 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 thesort
param in the URL. I'm not sure if I should also leave a comment letting the user know that the even if thesort
value is unexpected later is changed toasc
whengetSort()
is executed.If you think that I should add that to the comment let me know.
Patch attached.
Comment #108
drunken monkeyAlright, makes sense and the change looks good to me. RTBC once again from me!
Comment #109
alexpottCommitted 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.
Fixed coding standards on commit.
Comment #111
alexpottComment #112
alexpottComment #113
xjmThe 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!
Comment #114
alexpottYep this is totally a feature addition. There's no BC implication.