Created attachment 98361 [details] [review] Bug 24573: Add missing dependencies to PerlDependencies.pm Catmandu::Store::ElasticSearch is required for search, and Catmandu::MARC is required for indexation The `max_ver` for Catmandu::Store::ElasticSearch is only informative and is not used anywhere. Koha is not compatible with Catmandu::Store::ElasticSearch 0.9_01 and later versions
Created attachment 98769 [details] [review] Bug 24573: Add missing dependencies to cpanfile Catmandu::Store::ElasticSearch is required for search, and Catmandu::MARC is required for indexation
Hi Julian, how test this patch?
(In reply to Michal Denar from comment #3) > Hi Julian, > how test this patch? Hi, I think it's enough to `git grep Catmandu::MARC` and `git grep Catmandu::Store::ElasticSearch` to see that they are used in Koha (thus they must appear in cpanfile) But you can go further and uninstall these modules, and see that search and reindexation don't work when SearchEngine = Elasticsearch You can try the latest version of Catmandu::Store::ElasticSearch and verify that search doesn't work with it. Downgrade to 0.512 and verify that search works.
Created attachment 99959 [details] [review] Bug 24573: Add missing dependencies to cpanfile Catmandu::Store::ElasticSearch is required for search, and Catmandu::MARC is required for indexation Signed-off-by: Michal Denar <black23@gmail.com>
(In reply to Julian Maurice from comment #4) > I think it's enough to `git grep Catmandu::MARC` and `git grep > Catmandu::Store::ElasticSearch` to see that they are used in Koha (thus they > must appear in cpanfile) git grep Catmandu::MARC cpanfile:requires 'Catmandu::MARC', '1.241'; Only used in the cpanfile ?
> Catmandu::Store::ElasticSearch is required for search, > and Catmandu::MARC is required for indexation No, we still have Zebra. I think it should be a recommend then?
(In reply to Marcel de Rooy from comment #6) > (In reply to Julian Maurice from comment #4) > > I think it's enough to `git grep Catmandu::MARC` and `git grep > > Catmandu::Store::ElasticSearch` to see that they are used in Koha (thus they > > must appear in cpanfile) > > git grep Catmandu::MARC > cpanfile:requires 'Catmandu::MARC', '1.241'; > > Only used in the cpanfile ? We use Catmandu::Importer::MARC, which is part of Catmandu::MARC distribution
(In reply to Marcel de Rooy from comment #7) > > Catmandu::Store::ElasticSearch is required for search, > > and Catmandu::MARC is required for indexation > > No, we still have Zebra. I think it should be a recommend then? There is no rule yet for deciding when a module should be required or recommended. I think it should be required because when you turn on the feature, Koha will die if these modules are not installed, and there is nothing preventing you to turn on the feature.
(In reply to Julian Maurice from comment #9) > (In reply to Marcel de Rooy from comment #7) > > > Catmandu::Store::ElasticSearch is required for search, > > > and Catmandu::MARC is required for indexation > > > > No, we still have Zebra. I think it should be a recommend then? > > There is no rule yet for deciding when a module should be required or > recommended. > I think it should be required because when you turn on the feature, Koha > will die if these modules are not installed, and there is nothing preventing > you to turn on the feature. It seems to be that they should not be required now, since we did not require them before in PerlDependencies. In one of the tests we are mocking the module for such a reason iirc. In Discussion? Please mail to the dev list? Or dev meeting agenda point?
(In reply to Marcel de Rooy from comment #10) > It seems to be that they should not be required now, since we did not > require them before in PerlDependencies. In one of the tests we are mocking > the module for such a reason iirc. They were totally missing from PerlDependencies.pm. I think it was more a mistake than a real willingness to make it optional. > In Discussion? Please mail to the dev list? Or dev meeting agenda point? If you think it's better to make it optional, that's fine by me. The important part of the patch is the version requirements. I will change requires to recommends
Created attachment 100226 [details] [review] Bug 24573: Add missing dependencies to cpanfile When using Elasticsearch, Catmandu::Store::ElasticSearch is required for search, and Catmandu::MARC is required for indexation Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 100227 [details] [review] Bug 24573: Add missing dependencies to cpanfile When using Elasticsearch, Catmandu::Store::ElasticSearch is required for search, and Catmandu::MARC is required for indexation Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nice work everyone! Pushed to master for 20.05
not backported due to dependencies