ModZebra is currently in C4::Biblio. This is not a logical place for it, since ModZebra is also used for modifying authorities. The logical place to have that functionality is in Koha::Search::Engine to help prepare us for the solr integration. I propose breaking the ModZebra code into separate parts for NoZebra and Zebra (in Koha::Search::Engine::NoZebra and Koha::Search::Engine::Zebra, respectively), and have Koha::Search::Engine::add_to_index_queue dispatch to the appropriate engine. C4::Biblio can remain as a wrapper around Koha::Search::Engine::add_to_index_queue for legacy scripts.
Created attachment 7100 [details] [review] Bug 7430: Move ModZebra out of C4::Biblio This proof-of-concept commit does the following: * Moves all the functionality from C4::Biblio::ModZebra into a new Koha::Search::Engine namespace, breaking it up into Zebra and NoZebra classes for the relevant sections. * Rather than calling ModZebra, callers should now use Koha::Search and call AddToIndexQueue() with the same arguments. * Creates a new Koha::Utils class with GetMarcFromKohaField and GetAuthType methods, in an attempt to begin the process of reducing circular dependencies * Adds a syspref SearchEngine to specify whether Zebra or NoZebra is to be used, based on the setting of NoZebra. This syspref is checked *only* in ModZebra replacement code. The NoZebra is still relied upon by *all other* search-related code. IMPORTANT NOTE: The syspref is added by the atomicupdate in installer/data/mysql/atomicupdate/bug_7430_add_searchengine_syspref IMPORTANT NOTE: NoZebra indexing is currently broken due to the lack of a get_auth_type_location() method that can be used by the Koha::Search::Engine::NoZebra class. IMPORTANT NOTE: This patch depends on the patches for bug 7284.
Created attachment 7144 [details] [review] Bug 7430: Move ModZebra out of C4::Biblio This proof-of-concept commit does the following: * Moves all the functionality from C4::Biblio::ModZebra into a new Koha::Search::Engine namespace, breaking it up into a Zebra class for the relevant section. * Rather than calling ModZebra, callers should now use Koha::Search and call AddToIndexQueue() with the same arguments. * Creates a new Koha::Utils class with GetMarcFromKohaField and GetAuthType methods, in an attempt to begin the process of reducing circular dependencies * Adds a syspref SearchEngine to specify which search engine should be used. At present the only option is Zebra. IMPORTANT NOTE: The syspref is added by the atomicupdate in installer/data/mysql/atomicupdate/bug_7430_add_searchengine_syspref IMPORTANT NOTE: NoZebra was deprecated for 3.4.0, and this commit entirely removes portions of the NoZebra code.
This is a great first step to permit Koha to use different search engines. I just want to add some remarks. First, why a new Koha::Utils ? Isn't C4::AuthoritiesMarc the good place for GetAuthType ? If moving from C4 to Koha namespace, I would prefer something like Koha::Authority::Type or Koha::AuthorityType for such a function. I have no suggestion for GetMarcFromKohaField but I think it must be placed anywhere but here. I believe we all agree that we don't want a new C4::Koha. It seems that Koha::Search::Engine::Zebra::get_indexes returns a arrayref, Koha::Search::Engine::get_indexes and Koha::Search::GetIndexes return directly this arrayref, and it's assigned to a hash. I think there is a problem here.
(In reply to comment #3) > This is a great first step to permit Koha to use different search engines. > > I just want to add some remarks. > > First, why a new Koha::Utils ? Isn't C4::AuthoritiesMarc the good place for > GetAuthType ? If moving from C4 to Koha namespace, I would prefer something > like Koha::Authority::Type or Koha::AuthorityType for such a function. > I have no suggestion for GetMarcFromKohaField but I think it must be placed > anywhere but here. I believe we all agree that we don't want a new C4::Koha. We don't want any circular dependencies, and this helps reduce them. The reasoning behind Koha::Utils is that it seemed like a reasonable name for a module that provided utility functions to access Koha state information. Counter-patches gratefully accepted. > It seems that Koha::Search::Engine::Zebra::get_indexes returns a arrayref, > Koha::Search::Engine::get_indexes and Koha::Search::GetIndexes return directly > this arrayref, and it's assigned to a hash. I think there is a problem here. I think you are right. ;) Good catch.
The script devel/misc/testbugzillapatches.pl has detected that this bug does not apply anymore, so i'm marking this bug as "does not apply". The script is supposed to handle conflicts related to updatedatabase, but no other cases of merge conflict. So the conflict may be trivial to fix. If it happens that the patch attached does apply cleanly, please send me an email, there's something wrong in the script. It's possible, as it's a recent script. If the patch applies does not apply as I think, please rebase and resubmit, thanks
Created attachment 8396 [details] [review] Bug 7430: Move ModZebra out of C4::Biblio (Rewrited to apply in current master) I submit the patch of Jared rewrited to apply in current master (it has no new changes). This is a nice job to start moving all Zebra code and adds the posibility to use other index engines like SolR
Created attachment 8403 [details] [review] Bug 7430 - ModZebra should not be in C4::Biblio This proof-of-concept commit does the following: * Moves all the functionality from C4::Biblio::ModZebra into a new Koha::Search::Engine namespace, breaking it up into a Zebra class for the relevant section. * Rather than calling ModZebra, callers should now use Koha::Search and call AddToIndexQueue() with the same arguments. * Creates a new Koha::Utils class with GetMarcFromKohaField and GetAuthType methods, in an attempt to begin the process of reducing circular dependencies * Adds a syspref SearchEngine to specify which search engine should be used. At present the only option is Zebra. IMPORTANT NOTE: The syspref is added by the atomicupdate in installer/data/mysql/atomicupdate/bug_7430_add_searchengine_syspref IMPORTANT NOTE: NoZebra was deprecated for 3.4.0, and this commit entirely removes portions of the NoZebra code. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signing off that the behavior is the same as in my original patch, except for a fix to the return type of Koha::Search::Engine::get_indexes and Koha::Search::GetIndexes. Another sign off is required.
Created attachment 8452 [details] [review] signed off
Looks like this can be set to signed off.
Created attachment 9240 [details] [review] From: Juan Romay Sieira <juan.sieira@xercode.es> Date: Wed, 21 Mar 2012 11:05:16 +0100 Subject: [PATCH] Bug 7430 - ModZebra should not be in C4::Biblio This proof-of-concept commit does the following: * Moves all the functionality from C4::Biblio::ModZebra into a new Koha::Search::Engine namespace, breaking it up into a Zebra class for the relevant section. * Rather than calling ModZebra, callers should now use Koha::Search and call AddToIndexQueue() with the same arguments. * Creates a new Koha::Utils class with GetMarcFromKohaField and GetAuthType methods, in an attempt to begin the process of reducing circular dependencies * Adds a syspref SearchEngine to specify which search engine should be used. At present the only option is Zebra. IMPORTANT NOTE: The syspref is added by the atomicupdate in installer/data/mysql/atomicupdate/bug_7430_add_searchengine_syspref IMPORTANT NOTE: NoZebra was deprecated for 3.4.0, and this commit entirely removes portions of the NoZebra code. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signing off that the behavior is the same as in my original patch, except for a fix to the return type of Koha::Search::Engine::get_indexes and Koha::Search::GetIndexes. Another sign off is required. Signed-off-by: Henri-Damien LAURENT <henridamien.laurent@biblibre.com>
setting status to 'passed QA' patch rebased against commit 012c6df0015b68dfa390d6ed4faedbc42c8af9ce Jared, i took the liberty of perltidying your newly added files (i just couldn't resist! :P) IMPORTANT NOTE: The syspref is added by the atomicupdate in installer/data/mysql/atomicupdate/bug_7430_add_searchengine_syspref
Back to Signed Off, as a QA team member has not signed off on this yet.
(In reply to comment #12) > Back to Signed Off, as a QA team member has not signed off on this yet. just verified with Chris.C... flicking back to 'passed QA', for inclusion in Koha-3.10
This patch still applies, with a tiny merge conflict that can easily be solved: <<<<<<< HEAD use C4::Search; ======= use Koha::Utils; use Koha::Search; >>>>>>> From: Juan Romay Sieira <juan.sieira@xercode.es> Date: Wed, 2 must be use C4::Search; use Koha::Utils; use Koha::Search; as use C4::Search has been reintroduced to avoid an error (see bug 7958) However, I won't push this patch yet, because I think Koha::Utils is a too generic term, that will quickly result in a place where there is everything and anything. I'll propose quickly a naming convention and organisation (and a follow-up patch). Let me a few days. Other than this, I like this patch (and I think we could get rid of NoZebra completely, as it does not work at all for now)
> However, I won't push this patch yet, because I think Koha::Utils is a too > generic term, that will quickly result in a place where there is everything > and anything. > I'll propose quickly a naming convention and organisation (and a follow-up > patch). Do you have a proposal for the naming convention? I have further work which is dependent on untying this stuff from C4, and moving SearchAuthorities into Koha::Search. > Let me a few days. > > Other than this, I like this patch (and I think we could get rid of NoZebra > completely, as it does not work at all for now) I think there's another bug for removing NoZebra somewhere in Bugzilla.
Created attachment 9903 [details] [review] Bug 7430 - ModZebra should not be in C4::Biblio This proof-of-concept commit does the following: * Moves all the functionality from C4::Biblio::ModZebra into a new Koha::Search::Engine namespace, breaking it up into a Zebra class for the relevant section. * Rather than calling ModZebra, callers should now use Koha::Search and call AddToIndexQueue() with the same arguments. * Creates a new Koha::Utils class with GetMarcFromKohaField and GetAuthType methods, in an attempt to begin the process of reducing circular dependencies * Adds a syspref SearchEngine to specify which search engine should be used. At present the only option is Zebra. IMPORTANT NOTE: The syspref is added by the atomicupdate in installer/data/mysql/atomicupdate/bug_7430_add_searchengine_syspref IMPORTANT NOTE: NoZebra was deprecated for 3.4.0, and this commit entirely removes portions of the NoZebra code. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signing off that the behavior is the same as in my original patch, except for a fix to the return type of Koha::Search::Engine::get_indexes and Koha::Search::GetIndexes. Another sign off is required. Signed-off-by: Henri-Damien LAURENT <henridamien.laurent@biblibre.com> Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased 2 June 2012.
I rebased the patch on latest master, and corrected an encoding issue that broke the patch subject. There are no changes in the latest version of the patch, so the status should remain at Passed QA.
(In reply to comment #14) > However, I won't push this patch yet, because I think Koha::Utils is a too > generic term, that will quickly result in a place where there is everything > and anything. > I'll propose quickly a naming convention and organisation (and a follow-up > patch). +1 for Koha::Utils being too generic..
(In reply to comment #18) > (In reply to comment #14) > > However, I won't push this patch yet, because I think Koha::Utils is a too > > generic term, that will quickly result in a place where there is everything > > and anything. > > I'll propose quickly a naming convention and organisation (and a follow-up > > patch). > > +1 for Koha::Utils being too generic.. I'm going to play curmudgeon, criticism without a valid counter suggestion is not useful :) So if not Koha::Utils what would you call it?
I suggest something like Koha::MARC::Framework[::Biblio] for GetMarcFromKohaField and Koha::AuthorityTypes for GetAuthType.
(In reply to comment #19) > I'm going to play curmudgeon, criticism without a valid counter suggestion > is not useful :) So if not Koha::Utils what would you call it? A crusty, ill-tempered, and usually old man? ;) I am not sure if that may not be useful [at times], btw.. But let me give it a try: High-level configuration functions like GetMarcFromKohaField and GetAuthType.. GetAuthType looks to me like a candidate for the originally called Koha::DataObject, since it just passes the fields for auth_types table. GetMarcFromKohaField should be moved somewhere within Koha::BusinessLogic or its shorter variant. There could be room there for MARC utilities? Let's say: Koha::BusinessLogic::MarcUtils::GetMarcTagForKohaField. If we do not like MarcUtils after all, another proposal: Koha::BusinessLogic::Administration::GetMarcTagForKohaField.
(In reply to comment #21) > (In reply to comment #19) > > I'm going to play curmudgeon, criticism without a valid counter suggestion > > is not useful :) So if not Koha::Utils what would you call it? > > A crusty, ill-tempered, and usually old man? ;) > I am not sure if that may not be useful [at times], btw.. But let me give it > a try: > High-level configuration functions like GetMarcFromKohaField and > GetAuthType.. > GetAuthType looks to me like a candidate for the originally called > Koha::DataObject, since it just passes the fields for auth_types table. > GetMarcFromKohaField should be moved somewhere within Koha::BusinessLogic or > its shorter variant. There could be room there for MARC utilities? > Let's say: Koha::BusinessLogic::MarcUtils::GetMarcTagForKohaField. > If we do not like MarcUtils after all, another proposal: > Koha::BusinessLogic::Administration::GetMarcTagForKohaField. Positive criticism, IE with useful suggestions on how to fix the problem is always useful, Negative criticism almost never is. Lest we become the linux kernel mailing list :-) My criticism here is those names are quite long, and I don't think these routines are business logic at all. Are we going to build an inheritance structure such that those are objects under inheriting from the classes above them, or is it just an arbitrary naming scheme. If it's arbitrary and there is no inheritance, then I like Koha::MARCUtils better. That would be my preference, but, I am not going to die in a ditch about it, I would accept something like Koha::Service::MARCUtils (Service is what BusinessLogic has moved to it seems). What I would like is this patch progressed and the remnants of no zebra to go away and stop causing us issues. And yes I am a grumpy old man :)
(In reply to comment #22) > My criticism here is those names are quite long, and I don't think these > routines are business logic at all. Are we going to build an inheritance > structure such that those are objects under inheriting from the classes > above them, or is it just an arbitrary naming scheme. agreed > If it's arbitrary and > there is no inheritance, then I like Koha::MARCUtils better. I suggested, for Logger.pm to have a Koha::Utils namespace. So Koha::Utils::Marc would easily take place here ! OTOH, it access the database, isn't it ? So will need access to Koha::DB::, and haven't we said something about business logic being the entry point for all db accesses ?
Since I will not be rebasing this again, I am closing this bug. The problem still exists, of course.