i believe this has never really worked correctly. basically, the issue is that the tablesorting code was only built to handle one table. attached patch adds an optional 'id' arg to both the tablesort_sql and theme_table functions. this allows for properly identifying sorting parameters per table. you can even sort mutliple tables in different ways, and the sorting parameters for each table will be preserved!
i was torn on whether to use an internal id for the table, or to allow the dev to specify. i chose allowing the dev to specify, but it probably wouldn't be too hard to change it to the other method if that's what we prefer.
to properly id a table, you need to specify the id arg in both of the above mentioned functions. i really didn't see a better way to do it, since the sql generation and theming generation are pretty separate.
note that this does not break any existing table code--if an id isn't specified, then things work the way they always have.
this is probably a borderline critical issue. i suppose it could be deemed an API change, or not critical b/c the functionality has never worked correctly. somebody knock me down if i'm out of line... :)
| Comment | File | Size | Author |
|---|---|---|---|
| tablesort.patch | 5.6 KB | hunmonk |
Comments
Comment #1
merlinofchaos commentedLooks like a reasonable solution to me.
I don't, however, feel this is really 'critical'.
Comment #2
chx commentedThis is definitely not critical. Also, this changes API... though the usual entrypoints are indeed BC.
Comment #3
chx commentedComment #4
drummCommitted to HEAD.
Comment #5
hunmonk commentedfor anyone interested, i added a short example on how to use this in the module how-to section of the handbook:
http://drupal.org/node/98097
Comment #6
dries commentedThis is not a bug report but a new feature. I also am not particularly fond of the implementation.
Comment #7
dries commentedI'm rolling this back. It's a new feature and not a bug report. This has been an intentional limitation for many years. Having multiple sortable tables on one page is only useful for mega-pages which should be avoided to begin with. Not being able to put two sortable pages on one page is a _good_ restriction. Sorry but I consider this patch to be cruft.
Either way it is an API change and a feature request, not a bug report and far from critical.
Comment #8
hunmonk commenteddries: i'm happy to rework the implementation to suit your needs--the patch was a first attempt at solving a usability issue. i do, however, think it's debatable that it's a good restriction. in my view there are certainly use cases where having more than one table on a page is perfectly valid, and leaving tablesorting broken for that case is, IMHO, a worse solution than the initial one i proposed... ;)
since i'd like to see this fixed in drupal 6, i'm moving it out of the postponed category. if anybody has suggestions on a better approach than the one i initially posted, please chime in with thoughts, and i'll try to work up another patch when the dev branch opens for 6.0
Comment #9
BioALIEN commentedI believe this is a valid fix to a problem/limitation present in the existing limitation of the tablesorting.
Recently I was required a stack of 10+ tables on a page, each with their individual sorting methods . I used a JS implementation in the end but it would have been nice to have a server-side fall over for it via drupal tablesort.
Comment #10
hunmonk commentedi still think this should be properly fixed, but i'm not going to champion it anymore. hopefully somebody else will pick it up.
Comment #11
moshe weitzman commentedWhen a developer goes out of his way to add multiple tables to a page, Drupal should not reject him with a red " i know this is bad for usability" card. I'd like to see this fix go in. The framework part of Drupal needs this sort of thing.
Comment #12
BioALIEN commentedI couldn't agree more. Anyway, now that FAPI3 is in place, should this fix be done at the API level? Would love to have this in D6, but not sure where it stands with the feature freeze.
Comment #13
merlinofchaos commentedBioALIEN: No, tables are only occasionally used for forms. This isn't really related to FAPI.
Comment #14
pasqualleif someone can make it without api change, than go on, but otherwise 7.x
Comment #15
Jaza commentedAFAIK this is still an issue, still waiting for someone to pick it up and give it some TLC again. Maybe 8.x, eh?
Comment #16
timmillwoodI have just ran into this issue when trying to add a 'content' and 'comments' table blocks to a content creator dashboard.
Comment #17
jaypanThis is a feature I would like to see implemented. I have created two tables for the Force Password Change module. One showing a list of users with a pending password change, the other showing a list of users without the pending password change. As the table headers are the same, both tables are affected when choosing to sort by any column in either of them.
pager_query.inc has a method for identifying multiple tables on a page, I feel that tablesort.inc should also contain a method for this.
Comment #18
dawehner.
Comment #19
dawehnerComment #32
daffie commentedComment #33
daffie commented