Bug 28606

Summary: Replace $ENV{DEBUG} and $DEBUG with Koha::Logger->debug
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart+koha>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: dcook, kyle, m.de.rooy, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28620
Change sponsored?: --- Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.11.00
Bug Depends on: 28572    
Bug Blocks:    
Attachments: Bug 28606: Remove $DEBUG and $ENV{DEBUG}
Bug 28606: Remove $DEBUG and $ENV{DEBUG}
Bug 28620: Remove trailing space when logging with log4perl
Bug 28606: Remove $DEBUG and $ENV{DEBUG}
Bug 28606: Remove $DEBUG and $ENV{DEBUG}
Bug 28606: Remove warning from C4::Auth 887

Description Jonathan Druart 2021-06-21 12:08:25 UTC
Same as bug 28572 for $ENV{DEBUG} and $DEBUG.
Comment 1 Jonathan Druart 2021-06-22 13:17:12 UTC
Created attachment 122281 [details] [review]
Bug 28606: Remove $DEBUG and $ENV{DEBUG}

We should remove the debug statements or use Koha::Logger when we want
to keep it.

Test plan:
Confirm that occurrences of remaining occurrences of DEBUG need to be
kept (historical scripts for instance)
Confirm that the occurrences removed by this patch can be removed
Confirm that the occurrences replaced by Koha::Logger are correct
Comment 2 Martin Renvoize 2021-06-23 10:41:10 UTC
Created attachment 122321 [details] [review]
Bug 28606: Remove $DEBUG and $ENV{DEBUG}

We should remove the debug statements or use Koha::Logger when we want
to keep it.

Test plan:
Confirm that occurrences of remaining occurrences of DEBUG need to be
kept (historical scripts for instance)
Confirm that the occurrences removed by this patch can be removed
Confirm that the occurrences replaced by Koha::Logger are correct

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2021-06-23 10:42:11 UTC
Another nice bit of cleanup here.

There are a few cases of "$debug and warn..." lines, but they appear to hard code setting `$debug = 0` internally and can be handled in another report I think.

Signing off.
Comment 4 Marcel de Rooy 2021-06-23 11:37:26 UTC
QA: Looking here now
Comment 5 Marcel de Rooy 2021-06-23 12:03:05 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2021-06-23 12:03:45 UTC
Oops
Comment 7 Marcel de Rooy 2021-06-23 12:04:09 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2021-06-23 12:09:38 UTC
+#warn "Got ", scalar(@mapkeys), " ldap mapkeys (  total  ): ", join ' ', @mapkeys, "\n";
 @mapkeys = grep {defined $mapping{$_}->{is}} @mapkeys;
-$debug and print STDERR "Got ", scalar(@mapkeys), " ldap mapkeys (populated): ", join ' ', @mapkeys, "\n";
+#warn "Got ", scalar(@mapkeys), " ldap mapkeys (populated): ", join ' ', @mapkeys, "\n";

Any plans here ?
Comment 9 Marcel de Rooy 2021-06-23 12:10:44 UTC
+    warn #"Finished \%memberhash has ", scalar(keys %memberhash), " keys\n",
                                        "Referencing \%mapping with ", scalar(keys %mapping), " keys\n";

Typo typo
Comment 10 Marcel de Rooy 2021-06-23 12:16:16 UTC
Just a side note:
-       if ($pkt =~ /AZ(....)$/) {
-               $debug and warn "verify_cksum: sum ($1) detected";
-       } else {
+    unless ($pkt =~ /AZ(....)$/) {

I have something with unless. When the ifs go down and the unlesses go up, Koha gets more complicated ;)
Comment 11 Marcel de Rooy 2021-06-23 12:24:17 UTC
-$ENV{DEBUG} = 1;
-warning_like { in_iprange("192.168.1.1/36") }
-    qr/cidrlookup failed for/,
-    'noisy simple invalid ip range/36 with remote ip in it';

Why removed?
Comment 12 Marcel de Rooy 2021-06-23 12:26:07 UTC
Created attachment 122330 [details] [review]
Bug 28606: Remove $DEBUG and $ENV{DEBUG}

We should remove the debug statements or use Koha::Logger when we want
to keep it.

Test plan:
Confirm that occurrences of remaining occurrences of DEBUG need to be
kept (historical scripts for instance)
Confirm that the occurrences removed by this patch can be removed
Confirm that the occurrences replaced by Koha::Logger are correct

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me, noting a few minor points on BZ.
Comment 13 Jonathan Druart 2021-06-23 12:34:32 UTC
(In reply to Martin Renvoize from comment #3)
> Another nice bit of cleanup here.
> 
> There are a few cases of "$debug and warn..." lines, but they appear to hard
> code setting `$debug = 0` internally and can be handled in another report I
> think.

Yes, it is expected. It's from scripts, not module. I thought it would be good to keep them.

(In reply to Marcel de Rooy from comment #8)
> +#warn "Got ", scalar(@mapkeys), " ldap mapkeys (  total  ): ", join ' ',
> @mapkeys, "\n";
>  @mapkeys = grep {defined $mapping{$_}->{is}} @mapkeys;
> -$debug and print STDERR "Got ", scalar(@mapkeys), " ldap mapkeys
> (populated): ", join ' ', @mapkeys, "\n";
> +#warn "Got ", scalar(@mapkeys), " ldap mapkeys (populated): ", join ' ',
> @mapkeys, "\n";
> 
> Any plans here ?

I don't think they should be logged, but they sounded useful, in case someone need to debug LDAP.

(In reply to Marcel de Rooy from comment #9)
> +    warn #"Finished \%memberhash has ", scalar(keys %memberhash), " keys\n",
>                                         "Referencing \%mapping with ",
> scalar(keys %mapping), " keys\n";
> 
> Typo typo

Oops, will fix.

(In reply to Marcel de Rooy from comment #10)
> Just a side note:
> -       if ($pkt =~ /AZ(....)$/) {
> -               $debug and warn "verify_cksum: sum ($1) detected";
> -       } else {
> +    unless ($pkt =~ /AZ(....)$/) {
> 
> I have something with unless. When the ifs go down and the unlesses go up,
> Koha gets more complicated ;)

I like unless :)

(In reply to Marcel de Rooy from comment #11)
> -$ENV{DEBUG} = 1;
> -warning_like { in_iprange("192.168.1.1/36") }
> -    qr/cidrlookup failed for/,
> -    'noisy simple invalid ip range/36 with remote ip in it';
> 
> Why removed?

Heh, it was not trivial to keep as we are now using Koha::Logger, and didn't think very useful to keep anyway.
Comment 14 Jonathan Druart 2021-06-23 12:37:01 UTC
Created attachment 122331 [details] [review]
Bug 28606: Remove $DEBUG and $ENV{DEBUG}

We should remove the debug statements or use Koha::Logger when we want
to keep it.

Test plan:
Confirm that occurrences of remaining occurrences of DEBUG need to be
kept (historical scripts for instance)
Confirm that the occurrences removed by this patch can be removed
Confirm that the occurrences replaced by Koha::Logger are correct

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me, noting a few minor points on BZ.

JD amended patch: replace "warn #Finished" with "#warn Finished", and
put the statement on a single line
Comment 15 Jonathan Druart 2021-06-24 09:54:33 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 16 Jonathan Druart 2021-06-24 10:50:23 UTC
t/Auth_with_shibboleth.t is failing

12:06:13 koha_1       | Log::Log4perl configuration looks suspicious: No loggers defined at /usr/share/perl5/Log/Log4perl/Config.pm line 325.
Comment 17 Kyle M Hall 2021-07-09 14:39:58 UTC
Enhancement not pushed to 21.05.x
Comment 18 Jonathan Druart 2021-07-13 09:42:35 UTC
Created attachment 122796 [details] [review]
Bug 28606: Remove warning from C4::Auth 887

[WARN] Use of uninitialized value in sprintf at /kohadevbox/koha/C4/Auth.pm line 887.
Comment 19 Jonathan Druart 2021-07-16 07:22:17 UTC
Follow-up pushed to master.
Comment 20 Jonathan Druart 2021-07-16 13:13:33 UTC
*** Bug 28699 has been marked as a duplicate of this bug. ***