This is terribly wrong as we are not aware the logger did not initiated correctly and we are going to lose logs. We should indeed make sure the error will be blocking and propagated to the interface.
Created attachment 103088 [details] [review] Bug 25172: Make Koha::Logger explode if init went wrong We do not want to fail silently for the logger.
Created attachment 103089 [details] [review] Bug 25172: Remove _recheck_logfile I don't understand how that could be useful. We do not want to test if the logfile is writable every time we log something!
Created attachment 103090 [details] [review] Bug 25172: Adjust tests
Created attachment 103091 [details] [review] Bug 25172: Identify and display possible problems on the about page
Applying: Bug 25172: Make Koha::Logger explode if init went wrong error: sha1 information is lacking or useless (Koha/Logger.pm). error: could not build fake ancestor Patch failed at 0001 Bug 25172: Make Koha::Logger explode if init went wrong I think this patch set needs a rebase.
Created attachment 103300 [details] [review] Bug 25172: Make Koha::Logger explode if init went wrong We do not want to fail silently for the logger.
Created attachment 103301 [details] [review] Bug 25172: Remove _recheck_logfile I don't understand how that could be useful. We do not want to test if the logfile is writable every time we log something!
Created attachment 103302 [details] [review] Bug 25172: Adjust tests
Created attachment 103303 [details] [review] Bug 25172: Identify and display possible problems on the about page
Patches apply now, taking a look.
Created attachment 103304 [details] [review] Bug 25172: Make Koha::Logger explode if init went wrong We do not want to fail silently for the logger. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 103305 [details] [review] Bug 25172: Remove _recheck_logfile I don't understand how that could be useful. We do not want to test if the logfile is writable every time we log something! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 103306 [details] [review] Bug 25172: Adjust tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 103307 [details] [review] Bug 25172: Identify and display possible problems on the about page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This all appears to work as expected for me.. and offers a genuine improvement.. I didn't like that we just carried on regardless either.. I'm sure Kyle has also done similar changes in a SIP bug at some point?
(In reply to Martin Renvoize from comment #15) > This all appears to work as expected for me.. and offers a genuine > improvement.. I didn't like that we just carried on regardless either.. I'm > sure Kyle has also done similar changes in a SIP bug at some point? The SIP logging bug uses Koha::Logger so its' behavior should be fixed by this as well.
I love everything about this patch set, but I'm getting one unit test failure: root@6b3a1a3db7b8:koha(bug25172-qa)$ prove t/Logger.t t/Logger.t .. # Failed test 'Logger did not init correctly without permission' # at t/Logger.t line 52. # Looks like you failed 1 test of 6. t/Logger.t .. 1/1 # Failed test 'Test01 -- Simple tests for Koha::Logger' # at t/Logger.t line 63. # Looks like you failed 1 test of 1. t/Logger.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/1 subtests Test Summary Report ------------------- t/Logger.t (Wstat: 256 Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 1 Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.36 cusr 0.06 csys = 0.44 CPU) Result: FAIL Jonathan, does this test fail for you as well?
Created attachment 103310 [details] [review] Bug 25172: Add missing filter
(In reply to Kyle M Hall from comment #17) > I love everything about this patch set, but I'm getting one unit test > failure: > > root@6b3a1a3db7b8:koha(bug25172-qa)$ prove t/Logger.t > t/Logger.t .. > # Failed test 'Logger did not init correctly without permission' > # at t/Logger.t line 52. > # Looks like you failed 1 test of 6. > t/Logger.t .. 1/1 > # Failed test 'Test01 -- Simple tests for Koha::Logger' > # at t/Logger.t line 63. > # Looks like you failed 1 test of 1. > t/Logger.t .. Dubious, test returned 1 (wstat 256, 0x100) > Failed 1/1 subtests > > Test Summary Report > ------------------- > t/Logger.t (Wstat: 256 Tests: 1 Failed: 1) > Failed test: 1 > Non-zero exit status: 1 > Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.36 cusr 0.06 > csys = 0.44 CPU) > Result: FAIL > > > Jonathan, does this test fail for you as well? Thanks for testing Kyle. No, it passes for me. But I have a weird behavior with this permission test, I had to move it up before the other test (when the log file is 700).
Hmm, I can't get the test to fail either.
(In reply to Martin Renvoize from comment #20) > Hmm, I can't get the test to fail either. Thanks for the confirmations. It's probably something odd with my setup.
Created attachment 103391 [details] [review] Bug 25172: Make Koha::Logger explode if init went wrong We do not want to fail silently for the logger. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 103392 [details] [review] Bug 25172: Remove _recheck_logfile I don't understand how that could be useful. We do not want to test if the logfile is writable every time we log something! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 103393 [details] [review] Bug 25172: Adjust tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 103394 [details] [review] Bug 25172: Identify and display possible problems on the about page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 103395 [details] [review] Bug 25172: Add missing filter Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
Created attachment 103485 [details] [review] Bug 25172: Fix Auth_with_ldap.t # Failed test 'checkpw_ldap tests' # at t/db_dependent/Auth_with_ldap.t line 324. {UNKNOWN}: Configuration not defined at /usr/share/perl5/Log/Log4perl/Config.pm line 579. at /kohadevbox/koha/Koha/Patron.pm line 395
Could bug 25284 be related to this?
(In reply to Katrin Fischer from comment #30) > Could bug 25284 be related to this? Testing strongly suggest it does... linking.
not backported to 19.11.x