Bug 10639 - Circular dependencies cause inventory.pl to fail when returning items
Summary: Circular dependencies cause inventory.pl to fail when returning items
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-25 00:48 UTC by David Cook
Modified: 2014-12-07 20:02 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2013-07-25 00:48:35 UTC
Currently, when you're running an inventory, inventory.pl will die if any of the items need to be returned (i.e. Koha says they're checked out, but they were found on the shelf, so we do an "AddReturn").

After chatting to Chris Cormack and Jared Camins-Esakov on IRC, it appears the cause is due to "circular dependencies". That is, C4::Circulation::AddReturn is failing, because Perl thinks certain subs used in AddReturn from C4::Items actually belong to C4::Circulation (or rather it's just plain confused about where they're supposed to come from). This is because C4::Items and C4::Circulation depend on each other. C4::Circulation directly loads C4::Items, while C4::Items loads C4::Search which loads C4::Reserves which loads C4::Circulation.

So...when inventory.pl loads C4::Items and C4::Circulation...we run into problems calling a sub from C4::Items within C4::Circulation, because there are conflicts/collisions in the namespace for subs from C4::Items (i.e. they've been loaded twice, so Perl has a hissy fit).

Chris mentioned that the model for new Koha modules will be to have no circular dependencies, but for now...I'm thinking that we fully qualify calls to imported subs...
Comment 1 David Cook 2013-07-25 02:49:08 UTC
Nevermind...I thought that I had tested this in the master branch, but apparently I had not. Apparently, I had actually just eyeballed the master code that I expected to be different (in Circulation.pm) and seeing it was the same...decided that was enough.

In fact, Jared Camins-Esakov removed this particular circular dependency back in 3.08.01 with Bug 7847: OPAC search dies with Plackthis (commit c97e0c76739b0624359feb47ee0573ebc46b8409).

In order to do so, he removed the "use C4::Search" declaration at the top of C4::Items and instead used "require C4::Search" in the subs that needed it, and then used fully qualified sub names (since they are required when using "require" rather than "use").

Yay, Jared!
Comment 2 David Cook 2013-07-25 03:13:17 UTC
That said, I think this is something that developers should keep in mind going forward...

Chris Cormack said that the ideal method should be as follows:

1) Export subs using the EXPORT_OK array
2) Import subs using "use C4::Module qw (sub1 sub2)"

Jared Camins-Esakov also mentioned:

1) In addition to the above, ff there is going to be any confusion, fully qualify the name (e.g. Add the C4::Module:: notation to the front of your imported sub when you're calling it).

2) "A require and fully-qualified call will at least make it easy to recognize problems."

My two cents:

I wonder if we should decide on the best method and put it in the Coding Guidelines?

Should we always fully qualify subroutine names?