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

remove hardcoded sortBy to read its value either from ini or post var #1427

Closed

Conversation

carlosmauri
Copy link
Contributor

Hi,

I've removed the hardcoded sort by logic here:
extension/ezjscore/classes/ezjscserverfunctionsjs.php
to be able to read either ini setting or post variable.

Now we can either use an ini setting to define the sort order we want to use globally or a new POST variable through the ajax call.
If the post variable is set, it will override the ini value.

If none of the values are set or the ezfind extension is OFF, there's a fallback that sets the $sortBy variable with the same "hardcoded" value.

This PR needs to be combined with this one:
ezsystems/ezfind#220

Thanks!

@andrerom
Copy link
Contributor

andrerom commented Apr 2, 2019

We don't usually place ezfind logic in legacy kernel.
If the fix is not already there, consider opening it there instead: https://github.com/ezsystems/ezfind/blob/master/classes/ezfindservercallfunctions.php#L16

Feel free to reopen if you think this is still relevant for kernel.

@andrerom andrerom closed this Apr 2, 2019
@peterkeung
Copy link
Contributor

peterkeung commented Apr 2, 2019

The ezjscore search function already had eZ Find specific logic (see the "SortBy" param), and the kernel doesn't use that deprecated eZ Find search call: https://github.com/ezsystems/ezfind/blob/master/classes/ezfindservercallfunctions.php#L14

@andrerom
Copy link
Contributor

andrerom commented Apr 2, 2019

Indeed you are right.
WDYT @emodric / @gggeek ?

@gggeek
Copy link
Contributor

gggeek commented Apr 3, 2019

Not a fan of this PR tbh. ezfind "knows" about ejscore. circualr deps are bad. ergo ezjscore should not "know" about ezfind (read an ini value from it). Eg. if ezfind is not installed this PR will generate a warning on every search...
I'd rather have ezjscore pass "NULL sort" to the search engine, and have the search engine determine what the default sorting is, or instead add SearchSettings/sortBy to kernel settings.
I understand that there is already handling of ezfind-specific stuff in ezjscServerFunctionsJs, but I'd rather remove it than add more.

@gggeek
Copy link
Contributor

gggeek commented Apr 3, 2019

Also, I would be happy as well with only handling here the case 'if received sort_by param via Post, honour it', and leave it up to the code that displays the search-page template and search link how to manage a default value...

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

Successfully merging this pull request may close these issues.

4 participants