Bug 14906

Summary: Koha::Logger does not correctly handle instantiation errors
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED INVALID QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: jweaver, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14597
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14167    
Bug Blocks:    
Attachments: Bug 14906: Koha::Logger - handle permission issues
[SIGNED-OFF] Bug 14906: Koha::Logger - handle permission issues

Description Jonathan Druart 2015-09-28 09:08:30 UTC
Koha::Logger should log in the regular log files if the paths are not correctly set (does not exist, no permission to write, etc.).

Also, the _check_conf subroutine will fail (return undef) if at least 1 path is not correctly defined, which is not necessary:

154     foreach my $l (@lines) {
155         if ( $l =~ /(OPAC|INTRANET)\.filename\s*=\s*(.*)\s*$/i ) {
156 
157             # we only check the two default logfiles, skipping additional ones
158             return if !-w $2;
159             push @logs, $1 . ':' . $2;
160         }
161     }
Comment 1 Jonathan Druart 2015-09-28 09:22:07 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-09-28 13:42:24 UTC
Created attachment 42918 [details] [review]
[SIGNED-OFF] Bug 14906: Koha::Logger - handle permission issues

Koha::Logger should log in the regular log files if the paths are not
correctly set (does not exist, no permission to write, etc.).

Test plan:
Set a path without write permission for the intranet in the log4perl
configuration file. The one for opac should be correct.
Try to write 1 line in the OPAC file.
Without this patch, nothing is logged, and nothing is warned in the
regular Koha log file.
With this patch, the line in the OPAC log file will be appened and the
Koha log file will have a line with a permission issue.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Marcel de Rooy 2015-10-08 10:12:03 UTC
First chmod your opac logfile to 400.
Second, try this test please:

use Koha::Logger;
my $logger = Koha::Logger->get({ interface => 'opac' });
$logger->warn('L4');

Without this patch, you will have no warning since the log is not writable. With your patch, the program will crash (on the following call on line 132 after calling _check_conf):
    Log::Log4perl->init_once($conf);
The intention of the code in Logger.pm was just to prevent such a crash.
This patch reintroduces it.

Failed QA
Comment 4 Marcel de Rooy 2015-10-08 10:18:22 UTC
(In reply to Jonathan Druart from comment #0)
> Also, the _check_conf subroutine will fail (return undef) if at least 1 path
> is not correctly defined, which is not necessary:

Yes it is.
Comment 5 Jonathan Druart 2015-10-08 13:42:43 UTC
I get
Logger configuration error - Wrong permission for file /home/koha/var/log/opac-error.log at t.pl line 289.
Can't open /home/koha/var/log/opac-error.log (Permission denied) at /usr/share/perl5/Log/Log4perl/Appender/File.pm line 124.
Comment 6 Jonathan Druart 2015-10-08 13:50:39 UTC
Forget last comment.

Marcel, what do you suggest?
Comment 7 Marcel de Rooy 2015-10-16 11:21:19 UTC
(In reply to Jonathan Druart from comment #6)
> Forget last comment.
> 
> Marcel, what do you suggest?

If one of those two logfiles (opac, intranet) is not writable, log4perl will crash nicely. The goal was to prevent such a crash.
So I would suggest to add the warnings about the unwritable logfile and still return undef. So do not call log4perl and no additional log message. Note that this situation should be quite unusual. Probably both files are writable or both are not.
Comment 8 Jesse Weaver 2015-12-11 20:06:30 UTC
May I ask why this was resolved invalid? It's a rather surprising silent failure that I had to find with a debugger, and anything besides silence would be preferable. Yes, this is unlikely in production, but it would still be nice to fix.
Comment 9 Marcel de Rooy 2015-12-12 08:37:31 UTC
(In reply to Jesse Weaver from comment #8)
> May I ask why this was resolved invalid? It's a rather surprising silent
> failure that I had to find with a debugger, and anything besides silence
> would be preferable. Yes, this is unlikely in production, but it would still
> be nice to fix.

Please check the earlier comments. The solution proposed was not adequate. On itself no reason to close the report, I agree.
If you have a better solution, please reopen.