Bug 7430 - ModZebra should not be in C4::Biblio
Summary: ModZebra should not be in C4::Biblio
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: Jared Camins-Esakov
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-09 20:22 UTC by Jared Camins-Esakov
Modified: 2014-12-07 20:03 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7430: Move ModZebra out of C4::Biblio (81.32 KB, patch)
2012-01-09 23:45 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7430: Move ModZebra out of C4::Biblio (66.94 KB, patch)
2012-01-13 19:19 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7430: Move ModZebra out of C4::Biblio (Rewrited to apply in current master) (66.02 KB, patch)
2012-03-21 10:39 UTC, Juan Romay Sieira
Details | Diff | Splinter Review
Bug 7430 - ModZebra should not be in C4::Biblio (67.09 KB, patch)
2012-03-21 11:25 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
signed off (67.12 KB, patch)
2012-03-21 17:20 UTC, laurenthdl
Details | Diff | Splinter 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 (65.10 KB, patch)
2012-04-18 13:33 UTC, Mason James
Details | Diff | Splinter Review
Bug 7430 - ModZebra should not be in C4::Biblio (65.11 KB, patch)
2012-06-02 17:28 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-01-09 20:22:40 UTC
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.
Comment 1 Jared Camins-Esakov 2012-01-09 23:45:05 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-01-13 19:19:12 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2012-01-19 11:23:24 UTC
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.
Comment 4 Jared Camins-Esakov 2012-01-19 14:04:26 UTC
(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.
Comment 5 Paul Poulain 2012-02-17 14:39:03 UTC
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
Comment 6 Juan Romay Sieira 2012-03-21 10:39:45 UTC Comment hidden (obsolete)
Comment 7 Jared Camins-Esakov 2012-03-21 11:25:38 UTC Comment hidden (obsolete)
Comment 8 laurenthdl 2012-03-21 17:20:11 UTC Comment hidden (obsolete)
Comment 9 Magnus Enger 2012-03-23 15:29:14 UTC
Looks like this can be set to signed off.
Comment 10 Mason James 2012-04-18 13:33:24 UTC Comment hidden (obsolete)
Comment 11 Mason James 2012-04-18 13:38:14 UTC
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
Comment 12 Chris Cormack 2012-04-18 22:51:11 UTC
Back to Signed Off, as a QA team member has not signed off on this yet.
Comment 13 Mason James 2012-04-27 01:55:10 UTC
(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
Comment 14 Paul Poulain 2012-05-10 16:46:04 UTC
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)
Comment 15 Jared Camins-Esakov 2012-06-02 17:20:25 UTC
> 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.
Comment 16 Jared Camins-Esakov 2012-06-02 17:28:40 UTC
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.
Comment 17 Jared Camins-Esakov 2012-06-02 17:30:35 UTC
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.
Comment 18 Marcel de Rooy 2012-07-02 06:39:46 UTC
(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..
Comment 19 Chris Cormack 2012-07-02 06:41:25 UTC
(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?
Comment 20 Julian Maurice 2012-07-02 07:01:33 UTC
I suggest something like Koha::MARC::Framework[::Biblio] for GetMarcFromKohaField and Koha::AuthorityTypes for GetAuthType.
Comment 21 Marcel de Rooy 2012-07-02 07:17:39 UTC
(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.
Comment 22 Chris Cormack 2012-07-02 07:47:43 UTC
(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 :)
Comment 23 Paul Poulain 2012-07-05 08:30:03 UTC
(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 ?
Comment 24 Jared Camins-Esakov 2012-09-02 12:37:24 UTC
Since I will not be rebasing this again, I am closing this bug. The problem still exists, of course.