yiisoft / data

Data providers
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
45 stars 18 forks source link

Sort and function getCriteria #75

Closed pawelkeska closed 2 years ago

pawelkeska commented 3 years ago

I have a question about Sort because when i create object Sort as below:

$sort = Sort::only([
  'title' => [
      'asc' => ['id' => 'asc', 'title' => 'desc'],
      'desc' => ['first_name' => 'desc', 'title' => 'asc'],
      'default' => 'desc'
  ],
  'descscription' => [
      'asc' => ['id_2' => 'asc', 'title_2' => 'desc'],
      'desc' => ['first_name_2' => 'desc', 'title_2' => 'asc'],
      'default' => 'desc'
  ],
])->withOrder(['descscription' => 'asc']);

Function getCriteria return: Array ( [id_2] => asc [title_2] => desc [first_name] => desc [title] => asc )

Why criteria return rest sorts by default option ? I think when i need only description asc it should return only this sort. In function getCriteria i found this:

foreach ($config as $field => $fieldConfig) {
    $criteria += $fieldConfig[$fieldConfig['default']];
}

Should it work in that way ? I've checked how it works in yii2 and sort doesn't return rest sorting options by default value. In yii-cycle it's used getOrder instead of getCriteria. Could someone explain me how can i create order query in my Doctrine Reader by using this class Sort. Perhaps in this package is another aproach ?

samdark commented 3 years ago

Should it work in that way ?

Yes. Currently it's by design:

  1. Read explicitly specified order: withOrder(['descscription' => 'asc'])
  2. Appy it (['id_2' => 'asc', 'title_2' => 'desc']).
  3. Apply defaults for fields not specified. In our case it's title and default is desc: ['first_name' => 'desc', 'title' => 'asc'].

The reasoning is that these interfaces are usually used for grids and not specifying sorting for a field in SQL gives non-predictable sorting (according to how it's stored in DB) but in general we want the sorting to be predictable.

I've checked how it works in yii2 and sort doesn't return rest sorting options by default value.

Yes, in Yii 2 it was different.

In yii-cycle it's used getOrder instead of getCriteria.

@roxblnfk do you remember why?

Could someone explain me how can i create order query in my Doctrine Reader by using this class Sort.

We haven't done any Doctrine integration yet. Maybe someone from Yii community.

roxblnfk commented 3 years ago

In yii-cycle it's used getOrder instead of getCriteria.

@roxblnfk do you remember why?

https://github.com/yiisoft/yii-cycle/issues/98

pawelkeska commented 3 years ago

Ok, thanks for the explanation. But when we have a sort object with 15 options to sorted data and have to sort by all fileds i think it's not efficient, especially when we make this operation on big data table. Is there any chance to add option which could turn it off ? i think i some cases it coluld be useful.

samdark commented 3 years ago

Could be useful, indeed.

samdark commented 3 years ago

An option like withoutDefaultSorting() should be alright.

pawelkeska commented 3 years ago

Yes. In my opinion it's alright. But we could add option to filed sorted like together/join/with and when config is being built in constructor, default option set to false. In this case we can still use all sorts together. Perhaps it's a bit magical but more flexaible.

'title' => [
  'asc' => ['title' => 'asc', 'content' => 'desc'],
  'desc' => ['id' => 'desc', 'title' => 'asc'],
  'default' => 'desc',
  'together/join/with' => true
]
samdark commented 3 years ago

Something like apply with values APPLY_ALWAYS, APPLY_IF_SPECIFIED?

pawelkeska commented 3 years ago

Yes, something like that. It's good idea.

pawelkeska commented 3 years ago

In constructor Sort class maybe add this option like false if we need that it should be added always, change option together/join/with to true (like in example in prev post).

/** @psalm-var TConfig $fieldConfig */
$normalizedConfig[$fieldName] = array_merge([
  'asc' => [$fieldName => SORT_ASC],
  'desc' => [$fieldName => SORT_DESC],
  'default' => 'asc',
  'together/join/with' => true
], $fieldConfig);

and in getCriteria function check it. If this option is to true add to criteria (default always will be false).


foreach ($config as $field => $fieldConfig) {
 if($fieldConfig['together/join/with'])
    $criteria += $fieldConfig[$fieldConfig['default']];
}
pawelkeska commented 3 years ago

i don't know is that correctly solution but i pushed it to my fork.

https://github.com/pawelkeska/data/commit/dee88c98f1da0e416b27e5fdd1b263ce19bdd3a9

samdark commented 3 years ago

I've left some comments.