NGSTACK-822 adding sorting option to netgen site search

[ljacmi]

No description provided.

[emodric]

This here looks like too much diff which is unrelated to feature itself, which makes it hard to review this properly. Please reduce diff only to relevant lines.

Also, do we need a configurable sorting, as opposed to just allowing to use every sort clause with the class name as sort key?

[ljacmi]

This here looks like too much diff which is unrelated to feature itself, which makes it hard to review this properly. Please reduce diff only to relevant lines.

Also, do we need a configurable sorting, as opposed to just allowing to use every sort clause with the class name as sort key?

@emodric

1. This diff was created automatically. I tried to avoid it, but every time I pressed save some automatic changes were made. I had to allow them to be able to commit.

Do you have a recommendation how to approach this problem? I see no solution to be honest.

2. As for the need for configurable sorting, I am not sure what you are suggesting. The configuration in yaml serves as the mapper between doGetQuery() parameters and SortClauses objects.

I also think this is what the task specified (as far as I can recall and read in Jira).

EDIT: I pushed a commit where I switched back the order of functions which cs fixer probably made. Also some other minor changes are made by cs-fixer, but they shold not be to distracting. Is this ok now?

[emodric]

This diff was created automatically. I tried to avoid it, but every time I pressed save some automatic changes were made. I had to allow them to be able to commit.

Disable code reformat in PHPStorm :)

As for the need for configurable sorting, I am not sure what you are suggesting. The configuration in yaml serves as the mapper between doGetQuery() parameters and SortClauses objects.

What I'm suggesting is that we do not use the mapper, but use the sort clause object name as the sort key, that way you don't need the mapping. So instead of ['sort' => ['published_date']], you would use ['sort' => ['DatePublished']] and then in code you build the sort clause object by concatenating the namespace with the object name. The alternative, which is maybe better, is to just have a hardcoded mapping of all sort types to sort clauses directly inside the class, so there is no need to have a config which enables or disables a specific sort. Just make them available all from the start.

[iherak]

Maybe even we can accept the FQN as sorting, so we don't have to do anything in our code here?
So it would be something like

['sort' => [DatePublished::class]]

[emodric]

Maybe even we can accept the FQN as sorting, so we don't have to do anything in our code here? So it would be something like

['sort' => [DatePublished::class]]

Even better! Although, my suggestion was more to simplify the configuration from the yaml files :)

[ljacmi]

Maybe even we can accept the FQN as sorting, so we don't have to do anything in our code here? So it would be something like

['sort' => [DatePublished::class]]

Even better! Although, my suggestion was more to simplify the configuration from the yaml files :)

A am having a hard time wrapping my head around some things...

So in the case of using FQN, the user is expected to know the whole FQN? I don't see how this is an improvement in user experience?

If I used the name of the sort object, it would still have to be mapped to the FQN somehow, wouldn't it? How?
Similiar problem if only QN is used.

Having hardcoded mapper in PHP is certainly an option.
I used yaml as a mapper, I think it makes no difference compared to hardcoded mapper in php, it is just a bit less code. Honestly I don't see a downside? If there is one please tell me.

EDIT: namespaces can potentially be different for different sort clauses, can't they? If I used namespace + name of the object, how would I know the correct namespace? This can also change through time and it feels to me that it would be a bit weird to hardcode them .

[iherak]

Maybe even we can accept the FQN as sorting, so we don't have to do anything in our code here? So it would be something like

['sort' => [DatePublished::class]]

Even better! Although, my suggestion was more to simplify the configuration from the yaml files :)

A am having a hard time wrapping my head around some things...

So in the case of using FQN, the user is expected to know the whole FQN? I don't see how this is an improvement in user experience?

That is a common usage, the user is a developer anyway, so it's a developer experience :)
When doing normal search (through Find service) user also needs to know the FQN, so I do not think this diminishes DX in any way to be honest.
The opposite actually, as it is much clearer to the user what is being used instead of having to read through the code to see what is supported, what is mapped to what etc.

[iherak]

@ljacmi where are we with this now?

[ljacmi]

@ljacmi where are we with this now?

@iherak In this last commit before your comment I made the changes requested. As far as I recall, this should be done and functional.

[emodric]

Why do we need $descendingOrder variable? Can't you just use $parameters['order'] direcly, and make their values correspond to Query::SORT_DESC and Query::SORT_ASC, so below you can just use new $name($parameters['order'])?

[ljacmi]

Fixed!

[emodric]

No need for trim and we should use Query::SORT_* constants for easier handling later on.

You can also just say in_array($order, [Query::SORT_ASC, Query::SORT_DESC], true).

Also why $searchText? :D

[ljacmi]

Fixed!

[emodric]

We should also check if the values are instances of Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause with: is_a($key, SortClause::class, true)

[ljacmi]

Fixed!

[emodric]

We can simplify to new $name($parameters['order']) if we change order values to Query::SORT_* constants.

[ljacmi]

Fixed!

[emodric]

We don't need this variable. It is only used once in foreach loop.

[ljacmi]

Fixed!

[emodric]

Please do not remove empty lines like this to maintain readability of the code.

[ljacmi]

Fixed!

[emodric]

Shouldn't setAllowedValues accept the array of allowed values, instead of a boolean?

From Syfmony docs:

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefault('transport', 'sendmail');
        $resolver->setAllowedValues('transport', ['sendmail', 'mail', 'smtp']);
    }

[emodric]

Ah, disregard, it also accepts a closure with bool return type.

[emodric]

We do not need this @return PHPDoc since it's already typehinted in method return type.

[ljacmi]

Fixed!

[emodric]

Can we not use $keys nomenclature here and instead use e.g. $classNames since this is what those are (or rather, should be)?

Same goes for method name. It should be named something similar to validateSortConfig or something similar.

[ljacmi]

@emodric I find sortKey terminology much clearer because we know what sorting key is immediately. On the other hand, validateSortConfig(array $classNames) is ambiguous.

Perhaps sortingKeyClassName and checkSortingKeyClassNamesAllowed() (or validateSortingKeyClassNames) would be my choices at this point.

Nevertheless, I changed it as you instructed!

[emodric]

Thanks @ljacmi !