Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Folder sort breaks existing group sorts (eg Fluent Localisation sorts) #820

Closed
2 tasks done
blueo opened this issue Aug 13, 2018 · 3 comments
Closed
2 tasks done

Comments

@blueo
Copy link
Contributor

blueo commented Aug 13, 2018

Currently the FolderTypeCreator sorts assets by folder first (line 163), in doing so it first saves the current order-by list then clears all other sorts to add a group sort by folder. However, if there is an existing grouped sort, such as as one set by the fluent module (tractorcow-farm/silverstripe-fluent#208), then this will get the same index (eg both sorts get _SortColumn0 as their identifier and as a result, only the folder sort remains.

To reproduce:
install the fluent module and add the FluentVersionedExtension to File, make sure that 'Name' is in the localised fields. This will mean the default sort on Name will become a group sort. This leads to unstable sorting as the 'Name' sort is removed when the folder sort is applied.

PRs

@tractorcow
Copy link
Contributor

If we change folder connector sort to FQN "File"."ClassName" instead of just "ClassName" maybe that'll fix it? Fluent is sensitive about FQN column.

@blueo
Copy link
Contributor Author

blueo commented Aug 28, 2018

Unless it is changed from a group filter I think the problem will still exist. The issue is that all filters are removed by the sort call and re-added. If a group query is included it is moved into the column select and given an index (SQLSelect.php line 360). So removing and adding messes up the index count and you get two _SortColumn0 criteria.

@lukereative lukereative self-assigned this Sep 23, 2018
@lukereative lukereative removed their assignment Oct 1, 2018
@chillu chillu added this to the Recipe 4.3.0 milestone Oct 3, 2018
@robbieaverill robbieaverill self-assigned this Oct 3, 2018
@robbieaverill
Copy link
Contributor

I've reviewed #844 which is great and fixed this bug. silverstripe/silverstripe-assets#166 seems related but not coupled to this issue, so I'm going to close this issue now - I've left some comments on that PR which can be discussed separately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants