Bug 9883

Summary: Koha::Plugins::* should not require koha-conf to load
Product: Koha Reporter: Jared Camins-Esakov <jcamins>
Component: ToolsAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact:
Severity: blocker    
Priority: P5 - low CC: katrin.fischer, kyle
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9890
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 9883 - Koha::Plugins::Base should not require koha-conf to load
Bug 9883 - Koha::Plugins::Base should not require koha-conf to load
[PASSED QA] Bug 9883 - Koha::Plugins::Base should not require koha-conf to load
Bug 9883 - Koha::Plugins::* should not require koha-conf to load - Followup - Load C4::Auth at run time
Bug 9883 - Add missing parameter to koha-conf.xml causing Plugins.t to fail
Bug 9883 - Koha::Plugins::* should not require koha-conf to load - Followup - Load C4::Auth at run time
Bug 9883 - Create plugins dir in Makefile
Bug 9883 - Koha::Plugins::Base should not require koha-conf to load
[PASSED QA] Bug 9883 - Add missing parameter to koha-conf.xml causing Plugins.t to fail
[PASSED QA] Bug 9883 - Koha::Plugins::* should not require koha-conf to load - Followup - Load C4::Auth at run time

Description Jared Camins-Esakov 2013-03-21 02:48:36 UTC
This line:
    push @INC, C4::Context->config("pluginsdir");

means that Koha::Plugins::Base cannot be loaded without a koha-conf.xml file present and pointed to by the KOHA_CONF environment variable.
Comment 1 Kyle M Hall 2013-03-21 08:36:46 UTC Comment hidden (obsolete)
Comment 2 Magnus Enger 2013-03-21 10:50:37 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-03-21 11:29:52 UTC
Some notes about my testing:

After applying the patch the tests still failed, after talking to Kyle I realized I should make sure that my conf is current. So I am rerunning Makefile.pl doing an upgrade.

3 new dependencies are listed:
- Archive::Zip - could be installed with apt-get libarchive-zip-perl
- Archive::Extract - no package?
- Module::Bundled::Files - no package?

Module::Bundled::Files seems to make the test fail, but according to the output of koha_perl_deps.pl -m it should not be required.

Archive::Extract is not listed by the script at all, but only by Makefile.PL.
Comment 4 Jared Camins-Esakov 2013-03-21 14:09:02 UTC
It turns out I misunderstood the error message. The problem is not the modification to @INC (though that should definitely be removed if it is unnecessary) but rather the inclusion of C4::Auth. C4::Auth should be loaded with require, at runtime.
Comment 5 Katrin Fischer 2013-03-21 14:18:20 UTC
Created attachment 16640 [details] [review]
[PASSED QA] Bug 9883 - Koha::Plugins::Base should not require koha-conf to load

Test Plan:
1) Run t/db_dependent/Plugins.t
2) Apply patch
3) Re-run t/db_dependent/Plugins.t

Signed-off-by: Magnus Enger <magnus@enger.priv.no>
The tests pass both before and after the patch. I can see no
ill effects on the functionality (tried uploading and
accessing the Kitchen Sink plugin).

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
After installing libmodule-bundled-files-perl from the
debian.koha-community.org I ran through Makefile.PL
updating my conf file successfully. Also all tests
and QA script pass now.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 6 Jared Camins-Esakov 2013-03-21 14:23:17 UTC
Sorry, I should have been more clear in my last update. I would *also* like a follow-up adjusting the use C4::Auth to a require. The patch currently on the bug will go in as-is, but rather than adding yet another bug to Bugzilla, I figure it makes more sense to change the title and consolidate.
Comment 7 Kyle M Hall 2013-03-22 08:32:35 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-03-22 08:33:44 UTC
(In reply to comment #6)
> Sorry, I should have been more clear in my last update. I would *also* like
> a follow-up adjusting the use C4::Auth to a require. The patch currently on
> the bug will go in as-is, but rather than adding yet another bug to
> Bugzilla, I figure it makes more sense to change the title and consolidate.

I'm having a bit of difficulty following your line of though. Is this followup what you were looking for?
Comment 9 Kyle M Hall 2013-03-22 09:08:59 UTC Comment hidden (obsolete)
Comment 10 Jared Camins-Esakov 2013-03-22 11:26:42 UTC
Anything in a BEGIN block gets run at compile time. I was thinking of using "require C4::Auth" right before C4::Auth routines are required.
Comment 11 Kyle M Hall 2013-03-22 11:32:59 UTC
(In reply to comment #10)
> Anything in a BEGIN block gets run at compile time. I was thinking of using
> "require C4::Auth" right before C4::Auth routines are required.

Thanks! I'll replace that followup with a more sensible one.
Comment 12 Kyle M Hall 2013-03-22 11:40:54 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-03-22 13:30:56 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2013-03-22 13:47:19 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2013-03-22 13:50:04 UTC
Created attachment 16787 [details] [review]
[PASSED QA] Bug 9883 - Add missing parameter to koha-conf.xml causing Plugins.t to fail

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 16 Katrin Fischer 2013-03-22 13:50:23 UTC
Created attachment 16788 [details] [review]
[PASSED QA] Bug 9883 - Koha::Plugins::* should not require koha-conf to load - Followup - Load C4::Auth at run time

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Unset KOHA_CONF variable and made sure t/00_load.t passed
Also ran Makefile.PL doing an update on my existing installation
and checked koha-conf.xml:

 <pluginsdir>/home/katrin/koha-dev/var/lib/plugins</pluginsdir>
 <enable_plugins>0</enable_plugins>
Comment 17 Jared Camins-Esakov 2013-03-22 13:52:56 UTC
This patch has been pushed to master.