Repository navigation
KB admin: let the article list sort by ID - #1954
Conversation
The articles list had no ID column, so there was no way to order the
grid by primary key. Add one as the last column, alongside the existing
sortable title/state/access headers, and bump the pagination colspan.
No controller change is needed: displayTask() orders via
Relational::ordered('filter_order', 'filter_order_Dir'), which reads the
request variable directly and applies no column whitelist, and `id` is a
real column on the article table.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nkissebe
left a comment
nkissebe
left a comment
There was a problem hiding this comment.
Thanks, the ID column itself is fine. The description's reasoning is the part I'd push back on: "no controller change is needed because ordered() applies no column whitelist" is a pre-existing problem this PR leans on, so I'll fix it here rather than add another column on top of it.
Sort column and direction aren't validated. displayTask() hands filter_order/filter_order_Dir straight to Relational::ordered():
ordered()treats a dotted value asrelationship.field, sofilter_order=foo.barmakes the model call$this->foo()on request, and an unknown column is an SQL error.- Both values are written to session state, so a bad one keeps breaking the list for that admin until the session ends.
- The KB API already confines
sort/sort_Dirto a fixed list for exactly this reason (api/controllers/entriesv1_0.php).
Header and query read different state. The headers use the controller's com_kb.articles.sort state while ordered() keeps its own model-level state, so the arrow can point at one column while the rows follow another.
Proposed change, which I'll push to this branch: confine the sort to the columns the grid offers (title, state, access, id) and the direction to asc/desc, reset and re-store the state when it's anything else, and order with order() from those validated filters so the header and the rows always agree. Then I'll rebase-merge.
Minor, left as is: the ID is now read twice per row by screen readers (the checkbox's sr-only label and the new cell).
displayTask() handed filter_order and filter_order_Dir straight to Relational::ordered(), which reads a dotted value as relationship.field (calling the named model method) and stores whatever it is given in the session. An unknown column was an SQL error, and an invalid direction made the query builder throw, and either then broke the list for that admin until the session ended. The column headers also read the controller's sort state while ordered() kept its own, so they could disagree. Accept only the columns the grid sorts on (title, state, access, id) and ASC/DESC, re-store the default when anything else arrives, and order with order() from those validated filters so the headers and the rows use the same values.
The articles list had no ID column, so there was no way to order the grid by primary key. Add one as the last column, alongside the existing sortable title/state/access headers, and bump the pagination colspan.
No controller change is needed: displayTask() orders via Relational::ordered('filter_order', 'filter_order_Dir'), which reads the request variable directly and applies no column whitelist, and
idis a real column on the article table.