When no sort is specified in a table's header a notice is thrown:
Notice: Undefined offset: 0 in TableSort->order() (line 131 of /home/ben/projects/d7/head/includes/tablesort.inc).
because the default ordering off the first column assumes a numeric key,
You can see this in core if you remove the sort on 'member_for' from user_admin_account()
Attached patch uses array_keys() to get the first header key.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 664042-tablesort-error-14.patch | 4.13 KB | effulgentsia |
| #12 | 664042-tablesort-error-12-test.test | 3.06 KB | coltrane |
| #12 | 664042-tablesort-test.png | 32.12 KB | coltrane |
| #11 | 664042-tablesort-error-11.patch | 1.06 KB | coltrane |
| tablesort-error-no-default-sort.patch | 1.13 KB | coltrane |
Comments
Comment #4
coltraneSetting back to CNR
Comment #7
effulgentsia commentedtablesort-error-no-default-sort.patch queued for re-testing.
Comment #8
effulgentsia commentedI'm not so sure about the use of array_pop(), but I agree that if there's a situation where a '0' index can't be assumed, then we need to fix it to be more robust. Can you please add a test that demonstrates the bug by failing with HEAD and that passes with the patch? Thanks. Also, I'm moving this to the "database system" since I think people watching those issues should see this too.
Comment #9
coltraneA test is a good idea. Instead of array_pop() what do you propose?
Comment #10
effulgentsia commentedHow about emulating the code above it that uses a local variable $header by doing:
Comment #11
coltraneRight on, that looks good. Here's an updated patch. I'll work on a test soon.
Comment #12
coltraneThe attached file is for testing that the TableSort error does not happen when no sort is set. It should fail without the patch in #11 applied but it does not, even though there are notices thrown in the testing results that show the error. I'm using the assertion
assertNoTextbut I'm obviously doing something wrong. The attached PNG shows the testing output.Comment #13
coltrane(Removing tag)
Comment #14
effulgentsia commentedCombines #11 with the test in #12. I removed the assertion from the test, because the PHP notices themselves cause a simpletest exception, which is all we need. I confirmed that the test does fail in HEAD and pass with the patch, and the code in the patch looks good to me.
@coltrane: once bot goes green, please review the patch to make sure I didn't mess anything up in the re-roll, and if it looks good to you, please RTBC.
Comment #16
effulgentsia commented#14: 664042-tablesort-error-14.patch queued for re-testing.
Comment #17
coltraneLooks good!
Comment #18
realityloop commented#14: 664042-tablesort-error-14.patch queued for re-testing.
Comment #19
berdir#14: 664042-tablesort-error-14.patch queued for re-testing.
Comment #20
marcingy commented#14: 664042-tablesort-error-14.patch queued for re-testing.
Comment #21
coltrane#14 still applies with offset (bump)
Comment #22
webchickCommitted to HEAD. Thanks!