🐛 (cms interface) applies default sort order to CMS ordering #325
Conversation
This allows secondary sort ordering to be applied when it exists and when the objects have not been sorted manually
src/GridFieldOrderableRows.php
Outdated
| @@ -359,7 +360,15 @@ public function getManipulatedData(GridField $grid, SS_List $list) | |||
| // Fix bug in 3.1.3+ where ArrayList doesn't account for quotes | |||
| $sortterm .= $this->getSortTable($list).'.'.$this->getSortField(); | |||
| } else { | |||
| // If not an ArrayList it's definitely a DataList right? | |||
There was a problem hiding this comment.
Nope. The param says SS_List (which is an interface) and there are more classes implementing it than just ArrayList and DataList. It's also extended to Sortable which may be implemented by yet another set of classes, so we can't assume the ->dataClass() method is available without further checks.
There was a problem hiding this comment.
You're right! I missed the sub-interfaces of SS_List.
Happy to alter the PR. I'm thinking of just directly checking method_exists($list, 'dataClass') or do you think $list instanceof DataList would be cleaner?
There was a problem hiding this comment.
I would check for the DataList and only apply the default sort as the secondary ordering there
There was a problem hiding this comment.
Checking for a DataList now.
src/GridFieldOrderableRows.php
Outdated
| if (Config::inst()->get($list->dataClass(), 'default_sort')) { | ||
| // Append the default sort to the end of the sort string | ||
| // This may result in redundancy... but it seems to work | ||
| $sortterm .= ', ' . Config::inst()->get($classname, 'default_sort'); |
There was a problem hiding this comment.
$sorttterm can be an empty string here, so we can't start with the comma here, will need further logic.
There was a problem hiding this comment.
Adjusted to only add a comma when the string is not empty.
Co-authored-by: Michal Kleiner <[email protected]>
|
I've created a follow up unit-test PR for this here #329 |
maxime-rainville
added
type/bug
affects/v4
complexity/low
impact/critical
and removed
type/bug
affects/v4
complexity/low
impact/critical
labels
This allows secondary sort ordering to be applied when it exists and when the objects have not been sorted manually.
This attempts to address #324. It could use some sanity checks and testing, but it is working as needed in my application. It was an open question to me whether this was the best way to address it or whether additional sort fields should be applied via a similar mechanism as what exists for
$extraSortFields. My preference was for the component to automatically pick up on default sorts, but applying sort through another method would make sense to me too.