[Koha-bugs] [Bug 14167] Add Koha::Logger based on Log4perl

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14167

Marcel de Rooy <m.de.rooy at rijksmuseum.nl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Failed QA

--- Comment #27 from Marcel de Rooy <m.de.rooy at rijksmuseum.nl> ---
QA Comment:
Nice feature. Economic implementation of logging.
Although the code looks good to me, I still have some concerns:

[1] For logging level and category handling, I made some tiny adjustments via
amending or follow-up.

[2] A bigger problem could be permissions on the logfile. 
If you do not have sufficient permissions, Log4perl will make a crash. So this
is a very fundamental issue. In Debian the koha instance user should have
permissions on the file, but I noticed that after a forced logrotate, the file
permissions for the new log file were changed to root and Koha crashed on that.
What about other distros btw?

[3] The BEGIN section of Koha::Logger assumes that it will have either a conf
file via $ENV{LOG4PERL_CONF} or the koha-conf.xml.
If you do not have either one, Koha will again crash on you (Configuration not
defined).
It is safer imo to go always via the koha-conf. If there is nothing in
koha-conf, set some flag, warn once and do nothing or perhaps fall back to
warn. (Note that you should trap all calls to warn, trace etc. in that case.) 
Note that a new installation would be fine. But an upgrade does not guarantee
the presence of a config file.
Also note that you can pass some default configuration string to init() too. 

By conclusion, we could argue that a Koha administrator should just take care
of the perl module and the log4perl config file and push this as-is. But we
also know that this will result in problems at various locations when
upgrading. What would be the best way to go here?

I will be happy to follow through on this report, but not in the next two
weeks. (But another QAer is welcome too.) 

Changing status to reflect need for changes or clarification..

-- 
You are receiving this mail because:
You are watching all bug changes.