WIP: FIX Implement localisable order by clause to support translatable sorting #415
Conversation
robbieaverill
force-pushed
the
pulls/4.1/allow-sorting
branch
from
798f74d to
e140b32
Compare
|
I've implemented a rough looking fix to allow localisable sort conditions in the same way that localisable where conditions work at the moment. There's a test failure in @tractorcow would be good to get your eyes over this when you're back from your holiday =) |
robbieaverill
changed the title
|
Ok so the tests I added in the first commit broke the |
robbieaverill
changed the title
src/Extension/FluentExtension.php
Outdated
| $foundInConditionSearch = false; | ||
| foreach ($conditionSearch as $conditionSearchOption) { | ||
| if (preg_match( | ||
| '/^"(?<table>[\w\\\\]+)"\."(?<field>' . trim($column, '"') . ')"$/i', |
There was a problem hiding this comment.
If $column fails the above regexp, I think assuming that it's a basic column name is a bit risky. It could be a non-trivial sub-query, or it could be a composite sort. The else should really be another regexp that is similar but doesn't require the table prefix.
There was a problem hiding this comment.
Ok, do you have a couple of examples that I could add to the tests?
There was a problem hiding this comment.
like $list->sort('"Title", "Content"')
There was a problem hiding this comment.
Sweet. We can't really make subqueries fluent in this case though right...?
There was a problem hiding this comment.
Right so you should skip them.
There was a problem hiding this comment.
I've added a bunch more tests, the last commit contains some tests that I've got no idea how to write logic for to make them pass 😆
Also I'm a bit iffy about some of the tests in the "Add more sorting tests" commit because they're relying on the DB to return natural sort orders when the sorting isn't specific enough to always return a definite order - the fixtures have common values in them (deliberately) but maybe we either need to remove the common values to ensure that there's a predictable sort order at all times.
robbieaverill
changed the title
… the behaviour they actually test
tractorcow
force-pushed
the
pulls/4.1/allow-sorting
branch
from
a449e8b to
0be2919
Compare
|
Pushed up some fixes @robbieaverill |
|
Pushed up linting fixes @robbieaverill but there are still some small issues to fix. |
|
I'm late to the party on this one, but please accept my delayed: |
|
If you experience this issue with SS 3.x theres a composer installable patch https://github.com/lerni/silverstripe3-mysql57-fluent |
Resolves #208