Allow local sort definitions in the "Sort By" menu

[ekaterinazinchenko]

These aliases can be configured in the LocalSortDefinitions section of the search configuration file (searches.ini).
Ex.:
[LocalSortDefinitions]
locMunich[field] = "max(1,product(termfreq('owner','Munich'),20.0))"
locMunich[order] = desc

This is my first pull request and please let me know if i have to do something differently.
Do we need any configuration examples? Or something else?

cf. https://sourceforge.net/p/vufind/mailman/message/59295052/

Thanks for supporting!

[demiankatz]

Thanks, @ekaterinazinchenko! I would suggest a few adjustments:

1.) As you suggest, it would be useful to have a commented-out section in the config file to show examples of how to use the new setting.

2.) I would suggest putting this in searches.ini rather than config.ini; config.ini is for global settings that apply to all search backends, but this setting is backend-specific. We use searches.ini for Solr-related settings, but this would enable us to include equivalent settings in other backend configuration files like EDS.ini or Summon.ini as needed. You just need to use getSearchIni() instead of getMainIni() to retrieve the appropriate file.

3.) I would create a protected array $sortDefinitions property at the top of the class rather than embedding it in normalizeSort. I would load the configuration values in the constructor rather than creating the getLocalSortDefinitions method. This way, the configuration will be loaded only once and can be looked up by normalizeSort. Otherwise, the configuration will have to be looked up for every normalization action. (If you feel that normalizeSort is called so infrequently that it's actually more efficient not to do the up-front configuration loading, I could probably be persuaded, but there might still be strategies we could use to improve the efficiency of the code in cases where multiple values are normalized).

I hope this is helpful, and please let me know if you need any more help from my side!

[ekaterinazinchenko]

Thank you , @demiankatz !

I hope I understand you correctly.

Here is an example of the configuration part in searches.ini:

[LocalSortDefinitions]
locMunich[field] = "max(1,product(termfreq('owner','Munich'),20.0))"
locMunich[order] = desc

It is also possible to overwrite the default configuration, like
title[field] = "ANY_SORT_FUNCTION"
title[order] = desc

Thank you!

[demiankatz]

Thanks for the refactoring, @ekaterinazinchenko, this is coming along very well! See below for a couple more comments. I have also allowed the automatic checks to run (they require admin permission for first-time contributors, but once this is merged, you'll be on record as a contributor, and future PRs will get checks automatically run). If they fail, it may indicate that you need to run composer fix to automatically correct some common style issues. (It's also possible that one of my suggestions below will be helpful).

Regarding the searches.ini examples, what you have shared is helpful -- I'd suggest adding it (commented out) to the searches.ini file as part of this PR.

Thanks again!

[demiankatz]

@ekaterinazinchenko, it also looks like some tests are failing because their expectations were not set up to match the new configuration loading behavior. Once we have worked through the other parts of the review, I will be happy to help figure out what is wrong and come up with a solution. We might as well refine the code first before we fix the tests, though, so I will wait until the rest of the review process is complete.

[ekaterinazinchenko]

Is something wrong with configuration loading? Please let me know. Thank you!

[demiankatz]

Thanks for all the improvements, @ekaterinazinchenko. I haven't tested this hands-on yet, but it looks good to me. See below for some suggestions, mostly relating to project style conventions. I hope you don't mind all the nitpicks! :-)

Once this is done, let me know if you want me to look into any remaining test failures.

[ekaterinazinchenko]

Great, @demiankatz! So I could understand better how Vufind works, please do not hesitate to do further improvements. Could you also please take a look at tests - it works for me locally. Thank you!

[demiankatz]

Thanks, @ekaterinazinchenko! This looks pretty good to me, though I think there may still be a couple of stylistic things that composer fix should automatically correct for you. I'm about out of time for today, but I'll see if I can help with the tests tomorrow.

[ekaterinazinchenko]

I ran 'composer fix' and a couple of small changes were made.
Unfortunately, I have no idea about the test failure. Thank you for taking a look, @demiankatz !

[demiankatz]

Thanks, @ekaterinazinchenko. Fortunately, the tests were not hard to fix -- I just had to add some new mock methods to mock objects so that the call to getSearchIni was simulated correctly.

I also fixed a couple of remaining style issues that the automatic tools reported but couldn't fix -- a couple of lines longer than the 120-character limit, and a missing blank line in a doc comment. Nothing serious, as you can tell!

The build is now passing here. I'll do a little hands-on testing, and if I don't find any problems there, I will approve and merge.

Thanks again for your contributions here; I'm sure others will find this useful, and it's really an overdue improvement to the code!

[ekaterinazinchenko]

Great! I will be happy if it can be useful for anybody else! Thank you for supporting!

[demiankatz]

This is working as expected and all tests are passing. Thanks again, @ekaterinazinchenko -- I will merge it now!