Should not we replace C4::Debug (and all "$debug && warn" statements) with Koha::Logger->debug?
Great idea. Probably we could miss a lot of such statements too. Remove instead of replace where appropriate?
+1, I love this idea
(In reply to Jonathan Druart from comment #0) > Should not we replace C4::Debug (and all "$debug && warn" statements) with > Koha::Logger->debug? +1
Created attachment 122118 [details] [review] Bug 28572: Remove C4::Debug We are using Koha::Logger when it makes sense to keep the info, otherwise we simply remove it
Created attachment 122147 [details] [review] Bug 28572: Remove C4::Debug We are using Koha::Logger when it makes sense to keep the info, otherwise we simply remove it Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Great work.. signing off.
No regressions found.. I went through the patches inspecting what had been removed/replaced and I agree with all the decisions Jonathan made.. be great to get a second opinion from QA, but I don't think there's anything controversial here.. it'll be great to see this go and hopefully it'll encourage a little more use of Koha::Logger by hiding dropping this alternative.. would be great to see more of the bare warns updated too at some point.
Just mentioning one compile error: perl -c tools/viewlog.pl Global symbol "$debug" requires explicit package name (did you forget to declare "my $debug"?) at /usr/share/koha/C4/Context.pm line 788. BEGIN not safe after errors--compilation aborted at /usr/share/koha/C4/Context.pm line 853. Compilation failed in require at /usr/share/koha/C4/Auth.pm line 30. BEGIN failed--compilation aborted at /usr/share/koha/C4/Auth.pm line 30. Compilation failed in require at tools/viewlog.pl line 23. BEGIN failed--compilation aborted at tools/viewlog.pl line 23.
sub get_shelves_userenv { my $active; unless ($active = $context->{userenv}->{$context->{activeuser}}) { $debug and warn "get_shelves_userenv cannot retrieve context->{userenv}->{context->{activeuser}}"; return;
- $debug and warn "Error ExportFramework $@\n"; + Koha::Logger->get->warn("Error ExportFramework $@"); Warn instead of debug ?
More cases of the former too
-BEGIN { - $DEBUG = ($ENV{DEBUG}) ? 1 : 0; -} We still have other cases of using $ENV{DEBUG} ? What about them?
git grep "C4::Debug" misc/admin/koha-preferences:use C4::Debug; misc/admin/koha-preferences: print STDERR $message . "\n" if ( $C4::Debug::debug );
(In reply to Marcel de Rooy from comment #9) > Just mentioning one compile error: > > perl -c tools/viewlog.pl Oops, there was a missing dependency on bug 28590. (In reply to Marcel de Rooy from comment #11) > - $debug and warn "Error ExportFramework $@\n"; > + Koha::Logger->get->warn("Error ExportFramework $@"); > > Warn instead of debug ? Yes, those are warnings (export errors). We can lower them later if it's too noisy. (In reply to Marcel de Rooy from comment #13) > -BEGIN { > - $DEBUG = ($ENV{DEBUG}) ? 1 : 0; > -} > > We still have other cases of using $ENV{DEBUG} ? What about them? This report was about C4::Debug only, we could extend it to DEBUG. (In reply to Marcel de Rooy from comment #14) > git grep "C4::Debug" > misc/admin/koha-preferences:use C4::Debug; > misc/admin/koha-preferences: print STDERR $message . "\n" if ( > $C4::Debug::debug ); Oops, my 'git grep' alias is not picking files without extension it seems!
Created attachment 122216 [details] [review] Bug 28572: Replace missing occurrence in misc/admin/koha-preferences
(In reply to Jonathan Druart from comment #15) > (In reply to Marcel de Rooy from comment #9) > > -BEGIN { > > - $DEBUG = ($ENV{DEBUG}) ? 1 : 0; > > -} > > > > We still have other cases of using $ENV{DEBUG} ? What about them? > > This report was about C4::Debug only, we could extend it to DEBUG. Pfiou, that's painful. I would prefer to do that on a separate bug report, unless someone else is volunteering.
Created attachment 122217 [details] [review] Bug 28572: Remove C4::Debug We are using Koha::Logger when it makes sense to keep the info, otherwise we simply remove it Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122218 [details] [review] Bug 28572: Replace missing occurrence in misc/admin/koha-preferences Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Awesome. I don't think C4::Debug even works that well with Plack? Looking forward to seeing this one in action..
Second patch will be squashed when pushed.
Pushed to master for 21.11, thanks to everybody involved!
Created attachment 122290 [details] [review] Bug 28572: Fix misc/admin/koha-preferences
Created attachment 122291 [details] [review] Bug 28572: Fix Search.t
(In reply to Jonathan Druart from comment #24) > Created attachment 122291 [details] [review] [review] > Bug 28572: Fix Search.t This does not work, "Can't open /var/log/z3950-error.log (Permission denied)" We need to mock the log4perl configuration for tests or we are going to face a lot of issues. Any good ideas?
Created attachment 122303 [details] [review] Bug 28572: Set missing __LOG_DIR__ variable in zebra_config.pl This patch makes zebra_config.pl create a temporary directory for logs and sets it in ENV so, when called, rewrite-config.PL sets it correctly. It also adds the new syspref Reference_NFL_Statuses to the big mock to silence many warnings introduced by bug 21260. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ok, Search.t got too noisy. It is because of undefined stuffs. It can be trivially fixed, but I also noticed that some warns that got replaced by calls to the logger, got the if $DEBUG; removed. Is that intended?
(In reply to Tomás Cohen Arazi from comment #27) > Ok, Search.t got too noisy. It is because of undefined stuffs. It can be > trivially fixed, but I also noticed that some warns that got replaced by > calls to the logger, got the if $DEBUG; removed. Is that intended? I wasn't paying attention :-D Koha::Logger, log level. he. Patch coming.
Created attachment 122304 [details] [review] Bug 28572: Avoid useless warnings There are too many warnings about uninitialized variables in Search.pm. This patch deals with that, the same way it is dealt accross the file: by setting an empty string when things are not defined. To test: 1. Run: $ kshell k$ prove t/db_dependent/Search.t => FAIL: Wow, too many warnings. Some related to this bug, some not. 2. Apply this patch 3. Repeat 1 => SUCCESS: No more warnings! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment on attachment 122304 [details] [review] Bug 28572: Avoid useless warnings Patch moved to bug 28483
c2fb056d7dc Bug 28572: Set missing __LOG_DIR__ variable in zebra_config.pl 3d1a92b6e1c Bug 28572: Fix Search.t 438cfbe1fc3 Bug 28572: Fix misc/admin/koha-preferences Pushed to master.
Enhancement not backported to 21.05