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

fix mode 1 sorting by flag for multiple fields #1536

Closed
wants to merge 2 commits into from

Conversation

@patrickjDE
Copy link
Contributor

commented May 31, 2018

WIth the following dca configuration

    ...
    'list' => [
        'sorting' => [
            'mode' => 1,
            'fields' => ['date_start', 'date_stop'],
            'flag' => 8, // sort descending by month
        ],
        ...

I'd expect the records to be sorted first descending by date_start then descending by date_stop, instead they are sorted ascending by date_start and descending by date_stop.

This is caused by https://github.com/contao/core-bundle/blob/4.4/src/Resources/contao/drivers/DC_Table.php#L4664 where the SQL for the sorting direction is only added once at the end, instead of being added to every field.

This PR fixes this issue by setting the sorting for each field according to the chosen flag, except where it was set explicitly (e.g. 'fields' => ['date_start', 'date_stop ASC'],).

It also fixes an SQL syntax error when neither sorting fields nor sorting flag are given (DESC was added to the query string, although there was no ORDER BY ... part).
Futher it fixes another SQL syntax error, when an even flag or no flag at all is given, and the last field has its sorting set explicitly (DESC was added twice in this case).

@leofeyer leofeyer added the defect label Jun 11, 2018

@leofeyer leofeyer added this to the 4.4.19 milestone Jun 11, 2018

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

You are right that the current logic is incorrect. However, your fix does not seem to fix it.

Contao considers two flags, the list.sorting.flag and the fields.<field>.flag. If you want to sort by both fields descending, you have to make sure that both date_start and date_stop have 'flag' => 8 set. Then the query will be:

… ORDER BY date_start DESC, date_stop DESC

So now the question remains why there is an additional list.sorting.flag in sorting mode 1? I don't recall. @contao/developers Do you?

@leofeyer leofeyer added up for discussion and removed defect labels Jun 13, 2018

@leofeyer leofeyer removed this from the 4.4.19 milestone Jun 13, 2018

Patrick Josupeit
@patrickjDE

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2018

You're right, as my test cases were dates (and thus had fields.<field>.flag set for formatting reasons), I didn't notice the value wasn't applied if the field has no date sorting flag. I also noticed another problem with my fix (with no flag at all would sort DESC).

I've updated the PR to also take fields.<field>.flag into account and apply sorting with the following priority:

  1. explicit in list.sorting.fields (e.g. 'fields' => ['date_start', 'date_stop ASC'],).
  2. fields.<field>.flag
  3. list.sorting.flag (can simply be removed, if the flag is deemed superfluous)

Additionally, thinking a bit about it, I'm not sure why this is limited to mode 1, shouldn't mode 2 (and mode 3, but that is broken beyond repair, afair) also respect the sorting directions defined in the dca?

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

shouldn't mode 2 also respect the sorting directions defined in the dca?

Probably yes.

Regarding the priority in your implementation: Shouldn't the list.sorting.flag override the fields.<field>.flag? So you can override the default sorting that is e.g. used in the filter menus with a custom sorting order?

@contao/developers /cc

@patrickjDE

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

Regarding the priority in your implementation: Shouldn't the list.sorting.flag override the fields..flag? So you can override the default sorting that is e.g. used in the filter menus with a custom sorting order?

I'm not sure. In https://github.com/contao/core-bundle/blob/4.4/src/Resources/contao/drivers/DC_Table.php#L4367 and https://github.com/contao/core-bundle/blob/4.4/src/Resources/contao/drivers/DC_Table.php#L4855 fields.<field>.flag also takes precedence over list.sorting.flag.
Also it feels just natural to me, that a setting that applies to a single field (fields.<field>.flag) overrides the setting that applies to all fields (list.sorting.flag).

@leofeyer

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

@patrickjDE We have tried to discuss this in Mumble on January 27th, however we do not really see a valid use-case. Can you elaborate on your use-case please?

@patrickjDE

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

Well, the actual use-case is simple: Sort a mode 1 dca by two fields, both descending. IMO a valid use-case, especially for dates.

I do realize that it is currently possible to achieve the desired result with explicitly defining the sorting SQL, but both ways to get there are counter-intuitive and IMO semantically wrong:

  1. Set DESC for every field, but the last:
    'fields' => ['date_start DESC', 'date_stop'],
  2. Set DESC for both fields, but also add list.sorting.flag with an odd value (which means sort ascending):
    'fields' => ['date_start DESC', 'date_stop DESC'], 'flag' => 7,
@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

are we gonna merge/fix this at all or close the PR?

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@aschempp That depends on whether or not you (or rather we) consider Patrick's use case to be valid or not. 🤓

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Set DESC for every field, but the last:
'fields' => ['date_start DESC', 'date_stop'],

As discussed in Mumble on June 6th, this is a bug. We have to add an isset($GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['flag']) check here:

if ($GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['mode'] == 1 && ($GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['flag'] % 2) == 0)
{
$query .= " DESC";
}

@leofeyer leofeyer added defect and removed up for discussion labels Jun 6, 2019

@leofeyer leofeyer added this to the 4.4 milestone Jun 6, 2019

@patrickjDE

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

That would fix the sql syntax bugs, but setting ['list']['sorting']['flag'] would still only apply to the last item, instead of all, or am I missing something?

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

You are right. We have to move the whole if ($GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['mode'] == 1 && ($GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['flag'] % 2) == 0) condition into the foreach loop, so the sorting flag is added to every field. But we still have to add the isset() check.

leofeyer added a commit to contao/contao that referenced this pull request Jul 12, 2019
@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Eventually fixed in contao/contao@fc9a686. Thank you @patrickjDE.

@leofeyer leofeyer closed this Jul 12, 2019

leofeyer added a commit to contao/comments-bundle that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.