Bug 28606

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

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>

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.

QA: Looking here now

Oops

+#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 ?

+    warn #"Finished \%memberhash has ", scalar(keys %memberhash), " keys\n",
                                        "Referencing \%mapping with ", scalar(keys %mapping), " keys\n";

Typo typo

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 ;)

-$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?

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.

(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.

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

Pushed to master for 21.11, thanks to everybody involved!

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.

Enhancement not pushed to 21.05.x

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.

Follow-up pushed to master.

*** Bug 28699 has been marked as a duplicate of this bug. ***