Bug 21036

Summary: Fix a bunch of older warnings
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: trivial    
Priority: P5 - low CC: jonathan.druart, kelly, lucas, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21547
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06
Attachments: Bug 21036: Fix CGI::param in list context warning from opac-passwd.pl
Bug 21036: Fix uninitialized value within @itemnumber in string ne
Bug 21036: Fix warnings from C4/Biblio
Bug 21036: Remove odd number of elements warnings from AuthoritiesMarc.pm
Bug 21036: Remove odd number of elements warnings from AuthoritiesMarc.pm
Bug 21036: Fix warnings from C4/Biblio
Bug 21036: Remove odd number of elements warnings from AuthoritiesMarc.pm
Bug 21036: Remove odd number of elements warnings from AuthoritiesMarc.pm
Bug 21036: Fix uninitialized value within @itemnumber in string ne
Bug 21036: Fix warnings from C4/Biblio
Bug 21036: Remove odd number of elements warnings from AuthoritiesMarc.pm

Description Marcel de Rooy 2018-07-04 14:31:03 UTC

    
Comment 1 Marcel de Rooy 2018-07-04 14:34:23 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2018-07-04 14:53:38 UTC
Created attachment 76689 [details] [review]
Bug 21036: Fix uninitialized value within @itemnumber in string ne

Line from 16.11 log:
Use of uninitialized value within @itemnumber in string ne at /usr/share/koha/prodclone/reserve/modrequest.pl line 70.

Test plan:
Read the change. Not 100% identical (numeric zero) but should be enough.
This line is probably not needed at all.
Comment 3 Jonathan Druart 2018-07-04 19:05:26 UTC
(In reply to Marcel de Rooy from comment #2)
> Read the change. Not 100% identical (numeric zero) but should be enough.
> This line is probably not needed at all.

Regarding
  commit 26469d87a261682dcbde0f41faa844133d10ed5e
  bug 2520: don't change item-level requests to bib-level

It is used to test
  <input type="hidden" name="itemnumber" value="" />
if item_level_hold it not set.
Comment 4 Marcel de Rooy 2018-07-05 06:41:25 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Marcel de Rooy from comment #2)
> > Read the change. Not 100% identical (numeric zero) but should be enough.
> > This line is probably not needed at all.
> 
> Regarding
>   commit 26469d87a261682dcbde0f41faa844133d10ed5e
>   bug 2520: don't change item-level requests to bib-level
> 
> It is used to test
>   <input type="hidden" name="itemnumber" value="" />
> if item_level_hold it not set.

Thx.
That should still work as expected. The itemnumber==0 case is only hypothetical; we do not have such items. Clearing the field should be fine.

If the line is needed or not here actually depends on if ModReserve and the routines it calls, can handle an empty string for itemnumber. I would think they do, but to be safe I do not remove the line.
Comment 5 Marcel de Rooy 2018-07-05 08:16:51 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2018-07-05 10:27:07 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2018-07-05 10:30:35 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2018-09-05 19:25:48 UTC
Comment on attachment 76702 [details] [review]
Bug 21036: Fix warnings from C4/Biblio

Review of attachment 76702 [details] [review]:
-----------------------------------------------------------------

::: C4/Biblio.pm
@@ +2324,4 @@
>          if ( ( @$tags[$i] ne $prevtag ) ) {
>              $close_last_tag = 0;
>              $j++ unless ( @$tags[$i] eq "" );
> +            my $str = ( $indicator->[$j] // q{} ) . ' '; # extra space prevents substr outside of string warn

Shouldn't this be two spaces? just because substr(' ',1,1) doesn't explode, doesn't mean you shouldn't hand it a nice string.
Comment 9 Marcel de Rooy 2018-09-06 06:33:21 UTC
(In reply to M. Tompsett from comment #8)
> Comment on attachment 76702 [details] [review] [review]
> Bug 21036: Fix warnings from C4/Biblio
> 
> Review of attachment 76702 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Biblio.pm
> @@ +2324,4 @@
> >          if ( ( @$tags[$i] ne $prevtag ) ) {
> >              $close_last_tag = 0;
> >              $j++ unless ( @$tags[$i] eq "" );
> > +            my $str = ( $indicator->[$j] // q{} ) . ' '; # extra space prevents substr outside of string warn
> 
> Shouldn't this be two spaces? just because substr(' ',1,1) doesn't explode,
> doesn't mean you shouldn't hand it a nice string.

This seems to be nice enough ;) Since $str goes directly into _default_ind_to_space, it really does not matter. But I do not mind adding a space..
Comment 10 Marcel de Rooy 2018-09-06 06:38:15 UTC
Created attachment 78455 [details] [review]
Bug 21036: Fix warnings from C4/Biblio

Use of uninitialized value $isbn in string ne at /usr/share/koha/prodclone/C4/Biblio.pm line 1794. (16.11 line number)
Trivial edit.

And these warnings from TransformHtmlToXml (with 16.11 line numbers):
Use of uninitialized value in substr at /usr/share/koha/prodclone/C4/Biblio.pm line 2527.
Use of uninitialized value in substr at /usr/share/koha/prodclone/C4/Biblio.pm line 2528.
substr outside of string at /usr/share/koha/prodclone/C4/Biblio.pm line 2528.
Indicator in 952 is empty at /usr/share/koha/prodclone/C4/Biblio.pm line 2534.

The last warning is not needed and can be removed.
Note that the code used the construct @$indicator[$j] for $$indicator[$j].
The first is an array slice. This worked in list context. But apparently
the second was meant to be used. And can be rewritten as $indicator->[$j]
which generally is considered more readable.
The code around indicator1/2 and ind1/2 is simplified. This change is applied
twice in the same sub.

Test plan:
Read the changes.
Run t/Biblio/TransformHtmlToXml.t
Comment 11 Marcel de Rooy 2018-09-06 06:38:19 UTC
Created attachment 78456 [details] [review]
Bug 21036: Remove odd number of elements warnings from AuthoritiesMarc.pm

Like this one (16.11 line number):
Odd number of elements in anonymous hash at C4/AuthoritiesMarc.pm line 1070.

We need to add a scalar to some MARC::Field::subfield calls. In list context
an empty list returned affects the hash built around it.

Test plan:
Could reproduce this warning easily from OPAC authority search.
opac-authorities-home.pl calling BuildSummary.
Comment 12 Marcel de Rooy 2018-10-11 10:12:40 UTC
Comment on attachment 76686 [details] [review]
Bug 21036: Fix CGI::param in list context warning from opac-passwd.pl

Moved this fix to 21547
Comment 13 Bouzid Fergani 2019-04-26 20:29:11 UTC
Created attachment 88969 [details] [review]
Bug 21036: Remove odd number of elements warnings from AuthoritiesMarc.pm

Like this one (16.11 line number):
Odd number of elements in anonymous hash at C4/AuthoritiesMarc.pm line 1070.

We need to add a scalar to some MARC::Field::subfield calls. In list context
an empty list returned affects the hash built around it.

Test plan:
Could reproduce this warning easily from OPAC authority search.
opac-authorities-home.pl calling BuildSummary.

Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 14 Martin Renvoize 2019-05-01 09:43:15 UTC
Created attachment 89164 [details] [review]
Bug 21036: Fix uninitialized value within @itemnumber in string ne

Line from 16.11 log:
Use of uninitialized value within @itemnumber in string ne at /usr/share/koha/prodclone/reserve/modrequest.pl line 70.

Test plan:
Read the change. Not 100% identical (numeric zero) but should be enough.
This line is probably not needed at all.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2019-05-01 09:43:17 UTC
Created attachment 89165 [details] [review]
Bug 21036: Fix warnings from C4/Biblio

Use of uninitialized value $isbn in string ne at /usr/share/koha/prodclone/C4/Biblio.pm line 1794. (16.11 line number)
Trivial edit.

And these warnings from TransformHtmlToXml (with 16.11 line numbers):
Use of uninitialized value in substr at /usr/share/koha/prodclone/C4/Biblio.pm line 2527.
Use of uninitialized value in substr at /usr/share/koha/prodclone/C4/Biblio.pm line 2528.
substr outside of string at /usr/share/koha/prodclone/C4/Biblio.pm line 2528.
Indicator in 952 is empty at /usr/share/koha/prodclone/C4/Biblio.pm line 2534.

The last warning is not needed and can be removed.
Note that the code used the construct @$indicator[$j] for $$indicator[$j].
The first is an array slice. This worked in list context. But apparently
the second was meant to be used. And can be rewritten as $indicator->[$j]
which generally is considered more readable.
The code around indicator1/2 and ind1/2 is simplified. This change is applied
twice in the same sub.

Test plan:
Read the changes.
Run t/Biblio/TransformHtmlToXml.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2019-05-01 09:43:20 UTC
Created attachment 89166 [details] [review]
Bug 21036: Remove odd number of elements warnings from AuthoritiesMarc.pm

Like this one (16.11 line number):
Odd number of elements in anonymous hash at C4/AuthoritiesMarc.pm line 1070.

We need to add a scalar to some MARC::Field::subfield calls. In list context
an empty list returned affects the hash built around it.

Test plan:
Could reproduce this warning easily from OPAC authority search.
opac-authorities-home.pl calling BuildSummary.

Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2019-05-01 09:45:16 UTC
Always nice to see a few warning cleared up.. these three patches perform well for me and I can't find any regressions.

Passing QA
Comment 18 Marcel de Rooy 2019-05-01 10:51:47 UTC
Thanks Bouzid and Martin
Comment 19 Nick Clemens 2019-05-07 11:44:17 UTC
Awesome work all!

Pushed to master for 19.05
Comment 20 Martin Renvoize 2019-05-08 15:49:09 UTC
Pushed to 18.11.x for 18.11.06
Comment 21 Lucas Gass 2019-05-30 14:35:58 UTC
backported to 18.05.x for 18.05.13