Bug 28572

Summary: Replace C4::Debug with Koha::Logger->debug
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: dcook, kyle, m.de.rooy, martin.renvoize, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch simplifies and clarifies how developers should add debug statements to the Koha codebase.
Version(s) released in:
21.11.00
Bug Depends on: 28570, 28590, 28591    
Bug Blocks: 28606, 28615    
Attachments: Bug 28572: Remove C4::Debug
Bug 28572: Remove C4::Debug
Bug 28572: Replace missing occurrence in misc/admin/koha-preferences
Bug 28572: Remove C4::Debug
Bug 28572: Replace missing occurrence in misc/admin/koha-preferences
Bug 28572: Fix misc/admin/koha-preferences
Bug 28572: Fix Search.t
Bug 28572: Set missing __LOG_DIR__ variable in zebra_config.pl
Bug 28572: Avoid useless warnings

Description Jonathan Druart 2021-06-16 13:31:26 UTC
Should not we replace C4::Debug (and all "$debug && warn" statements) with Koha::Logger->debug?
Comment 1 Marcel de Rooy 2021-06-17 06:40:07 UTC
Great idea. Probably we could miss a lot of such statements too. Remove instead of replace where appropriate?
Comment 2 Martin Renvoize 2021-06-17 11:55:10 UTC
+1, I love this idea
Comment 3 Kyle M Hall 2021-06-17 12:09:16 UTC
(In reply to Jonathan Druart from comment #0)
> Should not we replace C4::Debug (and all "$debug && warn" statements) with
> Koha::Logger->debug?

+1
Comment 4 Tomás Cohen Arazi 2021-06-17 12:50:05 UTC
(In reply to Jonathan Druart from comment #0)
> Should not we replace C4::Debug (and all "$debug && warn" statements) with
> Koha::Logger->debug?

+1
Comment 5 Jonathan Druart 2021-06-18 10:11:27 UTC
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
Comment 6 Martin Renvoize 2021-06-18 14:28:24 UTC
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>
Comment 7 Martin Renvoize 2021-06-18 14:29:54 UTC
Great work.. signing off.
Comment 8 Martin Renvoize 2021-06-18 14:35:21 UTC
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.
Comment 9 Marcel de Rooy 2021-06-21 09:28:31 UTC
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.
Comment 10 Marcel de Rooy 2021-06-21 09:29:39 UTC
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;
Comment 11 Marcel de Rooy 2021-06-21 09:35:04 UTC
-                $debug and warn "Error ExportFramework $@\n";
+                Koha::Logger->get->warn("Error ExportFramework $@");

Warn instead of debug ?
Comment 12 Marcel de Rooy 2021-06-21 09:35:25 UTC
More cases of the former too
Comment 13 Marcel de Rooy 2021-06-21 09:39:53 UTC
-BEGIN {
-    $DEBUG = ($ENV{DEBUG}) ? 1 : 0;
-}

We still have other cases of using $ENV{DEBUG} ? What about them?
Comment 14 Marcel de Rooy 2021-06-21 09:45:33 UTC
git grep "C4::Debug"
misc/admin/koha-preferences:use C4::Debug;
misc/admin/koha-preferences:    print STDERR $message . "\n" if ( $C4::Debug::debug );
Comment 15 Jonathan Druart 2021-06-21 10:21:18 UTC
(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!
Comment 16 Jonathan Druart 2021-06-21 10:23:13 UTC
Created attachment 122216 [details] [review]
Bug 28572: Replace missing occurrence in misc/admin/koha-preferences
Comment 17 Jonathan Druart 2021-06-21 10:29:51 UTC
(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.
Comment 18 Marcel de Rooy 2021-06-21 11:32:08 UTC
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>
Comment 19 Marcel de Rooy 2021-06-21 11:32:12 UTC
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>
Comment 20 David Cook 2021-06-21 23:27:14 UTC
Awesome. I don't think C4::Debug even works that well with Plack? Looking forward to seeing this one in action..
Comment 21 Jonathan Druart 2021-06-22 10:04:22 UTC
Second patch will be squashed when pushed.
Comment 22 Jonathan Druart 2021-06-22 10:14:53 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 23 Jonathan Druart 2021-06-22 14:58:24 UTC
Created attachment 122290 [details] [review]
Bug 28572: Fix misc/admin/koha-preferences
Comment 24 Jonathan Druart 2021-06-22 14:58:28 UTC
Created attachment 122291 [details] [review]
Bug 28572: Fix Search.t
Comment 25 Jonathan Druart 2021-06-22 15:00:19 UTC
(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?
Comment 26 Tomás Cohen Arazi 2021-06-22 19:19:00 UTC
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>
Comment 27 Tomás Cohen Arazi 2021-06-22 19:25:36 UTC
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?
Comment 28 Tomás Cohen Arazi 2021-06-22 19:41:04 UTC
(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.
Comment 29 Tomás Cohen Arazi 2021-06-22 19:45:41 UTC
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 30 Jonathan Druart 2021-06-23 08:00:33 UTC
Comment on attachment 122304 [details] [review]
Bug 28572: Avoid useless warnings

Patch moved to bug 28483
Comment 31 Jonathan Druart 2021-06-23 08:04:09 UTC
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.
Comment 32 Kyle M Hall 2021-07-09 12:26:16 UTC
Enhancement not backported to 21.05