Bug 28606 - Replace $ENV{DEBUG} and $DEBUG with Koha::Logger->debug
Summary: Replace $ENV{DEBUG} and $DEBUG with Koha::Logger->debug
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 28699 (view as bug list)
Depends on: 28572
Blocks:
  Show dependency treegraph
 
Reported: 2021-06-21 12:08 UTC by Jonathan Druart
Modified: 2022-06-06 20:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 28606: Remove $DEBUG and $ENV{DEBUG} (59.75 KB, patch)
2021-06-22 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28606: Remove $DEBUG and $ENV{DEBUG} (59.72 KB, patch)
2021-06-23 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28620: Remove trailing space when logging with log4perl (7.83 KB, patch)
2021-06-23 12:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28606: Remove $DEBUG and $ENV{DEBUG} (59.87 KB, patch)
2021-06-23 12:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28606: Remove $DEBUG and $ENV{DEBUG} (60.09 KB, patch)
2021-06-23 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28606: Remove warning from C4::Auth 887 (1.15 KB, patch)
2021-07-13 09:42 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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. ***