Description
Marcel de Rooy
2018-07-04 14:31:03 UTC
Created attachment 76686 [details] [review] Bug 21036: Fix CGI::param in list context warning from opac-passwd.pl Resolve: CGI::param called in list context from package CGI::Compile::ROOT::usr_share_koha_prodclone_opac_opac_2dpasswd_2epl line 55, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436. Add a scalar. Test plan: Speaks for itself. 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. (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. (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. Created attachment 76702 [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 Created attachment 76703 [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 the empty list returned might affect the parameter hash around it. Test plan: Could reproduce this warning easily from OPAC authority search. opac-authorities-home.pl calling BuildSummary. Created attachment 76704 [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 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. (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.. 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 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 on attachment 76686 [details] [review] Bug 21036: Fix CGI::param in list context warning from opac-passwd.pl Moved this fix to 21547 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> 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> 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> 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> 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 Thanks Bouzid and Martin Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.06 backported to 18.05.x for 18.05.13 |