Bug 15795 - C4/Members.pm is floody (Norwegian Patron DB)
Summary: C4/Members.pm is floody (Norwegian Patron DB)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Mark Tompsett
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 15151
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-11 02:27 UTC by Mark Tompsett
Modified: 2017-06-14 22:12 UTC (History)
9 users (show)

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


Attachments
Bug 15795: C4/Members.pm is floody (Norwegian Patron DB) (2.43 KB, patch)
2016-02-11 03:03 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15795: C4/Members.pm is floody (Norwegian Patron DB) (2.51 KB, patch)
2016-02-11 14:59 UTC, Blou
Details | Diff | Splinter Review
Bug 15795: C4/Members.pm is floody (Norwegian Patron DB) (2.59 KB, patch)
2016-02-12 07:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2016-02-11 02:27:52 UTC
A warn without a prepended $debug is causing floodiness when NorwegianPatronDB is not set.
Comment 1 Mark Tompsett 2016-02-11 02:51:56 UTC
Though I can't see how to trigger it, except by deleting ~/kohaclone/Koha/NorwegianPatronDB.pm, there probably should be a $debug prepended on the warn.
Comment 2 Mark Tompsett 2016-02-11 03:03:41 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2016-02-11 03:12:06 UTC
See the conversation on Nabble: http://koha.1045719.n5.nabble.com/Introduce-the-use-of-Grunt-or-Gulp-tp5873339p5873842.html

Sadly, it got inter-threaded. The dangers of just replying in order to create a new thread. :(
Comment 4 Magnus Enger 2016-02-11 08:59:55 UTC
How about getting rid of both the if and the warn? So from this:

 use Module::Load::Conditional qw( can_load );
 if ( ! can_load( modules => { 'Koha::NorwegianPatronDB' => undef } ) ) {
    warn "Unable to load Koha::NorwegianPatronDB";
 }

to this:

 use Module::Load::Conditional qw( can_load );
 can_load( modules => { 'Koha::NorwegianPatronDB' => undef } );
Comment 5 Mark Tompsett 2016-02-11 13:11:13 UTC
(In reply to Magnus Enger from comment #4)
> How about getting rid of both the if and the warn? So ... this:
> 
>  use Module::Load::Conditional qw( can_load );
>  can_load( modules => { 'Koha::NorwegianPatronDB' => undef } );

While functional, I think warns are a good thing. I would rather keep the potential to warn, hence my patch.
Comment 6 Blou 2016-02-11 14:59:17 UTC Comment hidden (obsolete)
Comment 7 Blou 2016-02-11 15:04:17 UTC
Although I would prefer Magnus' code snippet overall, because it's less ugly than the big "if" that gives too much importance to that feature IMO, adding $debug is following the standard and expectations.  

I like standards...
Comment 8 Mark Tompsett 2016-02-11 18:26:23 UTC
(In reply to Blou from comment #7)
> Although I would prefer Magnus' code snippet overall,
> because it's less ugly

-- totally agree.


> than the big "if" that gives too much importance to that feature IMO, adding
> $debug is following the standard and expectations.  
> 
> I like standards...

And the discussion on development mailing list may end up turning this functionality into a plug-in in the future, but until then, this suffices. +1 to future plugin. :)
Comment 9 Marcel de Rooy 2016-02-12 07:14:49 UTC
Created attachment 47948 [details] [review]
Bug 15795: C4/Members.pm is floody (Norwegian Patron DB)

From the mailing list:
"I'm working on 3.22.02 and my logs are FILLED with the following warnings:

Members.pm: Unable to load Koha::NorwegianPatronDB at /blurred/C4/Members.pm line 46., referer: http://blurred/cgi-bin/koha/acqui/booksellers.pl
admin-home.pl: Unable to load Koha::NorwegianPatronDB at /blurred/C4/Members.pm line 46., referer: http://blurred/cgi-bin/koha/admin/preferences.pl"

I agree. Looking at C4/Members.pm other warns are all prepended with $debug.

TEST PLAN
---------
1) rm Koha/NorwegianPatronDB.pm
   -- quickest and dirtiest way to get the load to fail.
2) echo > ~/koha-dev/var/log/koha-error_log
   -- so it will be easy to spot the log changes.
   -- back up the log file if you really want it.
3) log into the staff client and go to patrons.
   -- Check the log file. "Unable to load ..." will be there.
4) echo > ~/koha-dev/var/log/koha-error_log
5) sudo vi /etc/apache2/sites-enabled/{your koha site file}
   -- Find your staff client entry in your apache
      configuration file and add:
       SetEnv DEBUG 1
6) sudo service apache2 restart
7) apply the patch
8) refresh the patron page
   -- "Unable to load..." will still be there.
9) change the DEBUG value in your apache config to 0
10) sudo sevice apache2 restart
11) echo > ~/koha-dev/var/log/koha-error_log
12) refresh the patron page
    -- "Unable to load..." will NOT be there.
13) git checkout origin/master -- Koha/NorwegianPatronDB.pm
    -- to bring it back.
14) refresh the patron page
    -- "Unable to load..." will still not be there.
15) run koha qa test tools

Signed-off-by: Philippe Blouin <philippe.blouin@inlibro.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Brendan Gallagher 2016-02-24 03:10:14 UTC
Pushed to Master - Should be in the May 2016 release.  (Just a note one of the previous patches wasn't obsolete - I followed up on that).  Thanks!
Comment 11 Julian Maurice 2016-02-26 15:43:22 UTC
Patch pushed to 3.22.x, will be in 3.22.4
Comment 12 Frédéric Demians 2016-03-01 16:54:57 UTC
This patch has been pushed to 3.20.x, will be in 3.20.9.