|
Description
Andrii Nugged
2020-05-10 10:21:14 UTC
Created attachment 104635 [details] [review] Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl This warning emitted: Use of uninitialized value $maxsuspensiondays in string eq at /admin/smart-rules.pl line 257. But that not just undef-warning, there is broken logic, these two lines are mutually contradictory and goes one-by-one: $maxsuspensiondays = undef if $maxsuspensiondays eq q||; $maxsuspensiondays = '' if $maxsuspensiondays eq q||; Fix is simple: both removed. Created attachment 104636 [details] [review] Bug 25440: Extra duplicated call to CGI->param method removed In code CGI param 'no_auto_renewal_after_hard_limit' assigned to "$no_auto_renewal_after_hard_limit" var, and then just in the next line again variable "$no_auto_renewal_after_hard_limit" reassigned with call to same "$input->param('no_auto_renewal_after_hard_limit')". Fixed. No logic or results should be changed. Created attachment 104637 [details] [review] Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl This warning emitted: Use of uninitialized value in string eq at /admin/smart-rules.pl line 289. It is solved by one line added to exclude comparison with 'on' string when variable is 'undef'. Created attachment 104638 [details] [review] Bug 25440: Fix for "CGI::param called in list context" in smart-rules.pl This warning emitted: CGI::param called in list context from /admin/smart-rules.pl line 262, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at CGI.pm line 412. Explained here: https://metacpan.org/pod/CGI#Fetching-the-value-or-values-of-a-single-named-parameter And because all these params are not multi-params, so simple "scalar .." forcing for CGI->param is the fix. Changes are transparent and same values should be assigned as before, just no more warnings. Created attachment 104639 [details] [review] Bug 25440: Fix for "uninitialized value in hash" warning in smart-rules.pl This warning emitted: Use of uninitialized value in hash element at /admin/smart-rules.pl line 569. that happened because we have NULLs in SQL results for 'categorycode' and 'itemtype' which later used as 'any' kind of category/item in the template, so for the template it passed this way: $rules->{ $r->{categorycode} }->{ $r->{itemtype} }->... but undef will stringify as "" to become a hash key ("Hashes are unordered collections of scalar values indexed by their associated string key" https://perldoc.perl.org/perldata.html), that's why "undef warning". To prevent warning here is the simple fix: $rules->{ $r->{categorycode} // '' }->{ $r->{itemtype} // '' }->... Created attachment 104640 [details] [review] Bug 25440: Fixed list of 'show_rule' forming variables in the template In smart-rules.tt we have `SET show_rule = ...` section which filled with 'all used in the loop' variables. Because if historical reasons it seems that there are some missing, few old, and even doubled ones. This list is fixed now by: - variable names 'article_requests' and 'renewalsallowed' repeated so duplicates are removed; - 'hardduedatebefore' and 'hardduedateexact' not present in the whole site code anywhere anymore; IMPORTANT NOTE: these 'hardduedatebefore/hardduedateexact' also exists as remnants in .po-translation files, a lot. - 'note', 'hardduedatecompare', 'renewalperiod', 'rentaldiscount' template variables were missing from this 'show_rule =' checking code so they are added. Comment on attachment 104637 [details] [review] Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl Review of attachment 104637 [details] [review]: ----------------------------------------------------------------- ::: admin/smart-rules.pl @@ +284,5 @@ > my $opacitemholds = $input->param('opacitemholds') || 0; > my $article_requests = $input->param('article_requests') || 'no'; > my $overduefinescap = $input->param('overduefinescap') || ''; > + my $cap_fine_to_replacement_price = $input->param('cap_fine_to_replacement_price'); > + $cap_fine_to_replacement_price = $cap_fine_to_replacement_price && $cap_fine_to_replacement_price eq 'on'; What about cap_fine_to_replacement_price = ($cap_fine_to_replacement_price || '') eq 'on'; Comment on attachment 104635 [details] [review] Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl Review of attachment 104635 [details] [review]: ----------------------------------------------------------------- ::: admin/smart-rules.pl @@ -253,5 @@ > my $fine = $input->param('fine'); > my $finedays = $input->param('finedays'); > my $maxsuspensiondays = $input->param('maxsuspensiondays'); > - $maxsuspensiondays = undef if $maxsuspensiondays eq q||; > - $maxsuspensiondays = '' if $maxsuspensiondays eq q||; Maybe a bit radical :) This is coming from a bad merge resolution conflict, I think it should be: my $maxsuspensiondays = $input->param('maxsuspensiondays') || q||; Just a feeling, it should be tested. (In reply to Jonathan Druart from comment #7) > > + my $cap_fine_to_replacement_price = $input->param('cap_fine_to_replacement_price'); > > + $cap_fine_to_replacement_price = $cap_fine_to_replacement_price && $cap_fine_to_replacement_price eq 'on'; > > What about > > cap_fine_to_replacement_price = ($cap_fine_to_replacement_price || '') eq > 'on'; I just mimicked code style in this same block, but of course (and I also like that in Perl) we can undef-protect inline if you want. Should I recreate the patch? Yes please :) (In reply to Jonathan Druart from comment #8) > > - $maxsuspensiondays = undef if $maxsuspensiondays eq q||; > > - $maxsuspensiondays = '' if $maxsuspensiondays eq q||; > > Maybe a bit radical :) > > This is coming from a bad merge resolution conflict, I think it should be: > my $maxsuspensiondays = $input->param('maxsuspensiondays') || q||; > > Just a feeling, it should be tested. In old db before circulation_rules table, it was "NULL-possible" value, so I was confused too. $input->param('maxsuspensiondays') here always comes as '' from posted form but if parameter absent so default might be When I removed these both lines I found in code only one place where this key is used: File: /opt/n/koha/git/KohaCommunity/C4/Circulation.pm 2326: my $max_sd = $issuing_rule->{maxsuspensiondays}; 2327: if ( defined $max_sd && $max_sd ne '' ) { 2328: $max_sd = DateTime::Duration->new( days => $max_sd ); 2329: $suspension_days = $max_sd 2330: if DateTime::Duration->compare( $max_sd, $suspension_days ) < 0; 2331: } and there both '' and undef is possible, so no matter which one we will have in db. But because current circulation_rules not allow "NULLS" for key/val pairs but pair absence is assumed as null so it might be the consequences, that why we had few other fixes and slightly mess with default states for parameters after conversion from issuinrules to circulation_rules, but yes, I agree, let's stick with one value, let it be ''. Recreating the patch now and will follow your proposal: and it works as on me. Created attachment 105059 [details] [review] Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl This warning emitted: Use of uninitialized value in string eq at /admin/smart-rules.pl line 289. It is solved by adding ".. || ''" to exclude comparison with 'on' string when variable is 'undef'. Created attachment 105063 [details] [review] Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl This warning emitted: Use of uninitialized value $maxsuspensiondays in string eq at /admin/smart-rules.pl line 257. But that not just undef-warning, there is broken logic, these two lines are mutually contradictory and goes one-by-one: $maxsuspensiondays = undef if $maxsuspensiondays eq q||; $maxsuspensiondays = '' if $maxsuspensiondays eq q||; Fix is simple: to make it '' if it comes undef. This last patch makes sense to me. (In reply to Andrew Nugged from comment #6) > Created attachment 104640 [details] [review] [review] > Bug 25440: Fixed list of 'show_rule' forming variables in the template Can you rewrite this patch keeping the exact same order of the variable list before? note, maxissueqty, maxonsiteissueqty, issuelength, etc. Order in "SET show_rule = ..."? It is as before, but changes are: 1. "hardduedatebefore || hardduedateexact" obsolete and removed, 2. "renewalsallowed" duplicate removed, 3. "article_requests" duplicate removed, 4. and new missed list "note || hardduedatecompare || renewalperiod || rentaldiscount || " added on the beginning of the line, ... no order changed. Can it be that was those p4 changes to the line start made you think that order changed? (or you want this " || " set to be the exact match with lines 129-157 separate one-by-one sets list?) You added: note || hardduedatecompare || renewalperiod || rentaldiscount before maxissueqty. I'd like the order of the variables in the "SET show_rule" statement to be the same as the order of the variables defined previously (l.129-157 yes) Created attachment 105985 [details] [review] Bug 25440: Fixed list of 'show_rule' forming variables in the template In smart-rules.tt we have `SET show_rule = ...` section which filled with 'all used in the loop' variables. Because if historical reasons it seems that there are some missing, few old, and even doubled ones. This list is fixed now by: - variable names 'article_requests' and 'renewalsallowed' repeated so duplicates are removed; - 'hardduedatebefore' and 'hardduedateexact' not present in the whole site code anywhere anymore; IMPORTANT NOTE: these 'hardduedatebefore/hardduedateexact' also exists as remnants in .po-translation files, a lot. - 'note', 'hardduedatecompare', 'renewalperiod', 'rentaldiscount' template variables were missing from this 'show_rule =' checking code so they are added. Order of fields updated to match with above "SET field = ..." pack. Created attachment 106300 [details] [review] Bug 25440: Extra duplicated call to CGI->param method removed In code CGI param 'no_auto_renewal_after_hard_limit' assigned to "$no_auto_renewal_after_hard_limit" var, and then just in the next line again variable "$no_auto_renewal_after_hard_limit" reassigned with call to same "$input->param('no_auto_renewal_after_hard_limit')". Fixed. No logic or results should be changed. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 106301 [details] [review] Bug 25440: Fix for "CGI::param called in list context" in smart-rules.pl This warning emitted: CGI::param called in list context from /admin/smart-rules.pl line 262, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at CGI.pm line 412. Explained here: https://metacpan.org/pod/CGI#Fetching-the-value-or-values-of-a-single-named-parameter And because all these params are not multi-params, so simple "scalar .." forcing for CGI->param is the fix. Changes are transparent and same values should be assigned as before, just no more warnings. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 106302 [details] [review] Bug 25440: Fix for "uninitialized value in hash" warning in smart-rules.pl This warning emitted: Use of uninitialized value in hash element at /admin/smart-rules.pl line 569. that happened because we have NULLs in SQL results for 'categorycode' and 'itemtype' which later used as 'any' kind of category/item in the template, so for the template it passed this way: $rules->{ $r->{categorycode} }->{ $r->{itemtype} }->... but undef will stringify as "" to become a hash key ("Hashes are unordered collections of scalar values indexed by their associated string key" https://perldoc.perl.org/perldata.html), that's why "undef warning". To prevent warning here is the simple fix: $rules->{ $r->{categorycode} // '' }->{ $r->{itemtype} // '' }->... Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 106303 [details] [review] Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl This warning emitted: Use of uninitialized value in string eq at /admin/smart-rules.pl line 289. It is solved by one line added to exclude comparison with 'on' string when variable is 'undef'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 106304 [details] [review] Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl This warning emitted: Use of uninitialized value $maxsuspensiondays in string eq at /admin/smart-rules.pl line 257. But that not just undef-warning, there is broken logic, these two lines are mutually contradictory and goes one-by-one: $maxsuspensiondays = undef if $maxsuspensiondays eq q||; $maxsuspensiondays = '' if $maxsuspensiondays eq q||; Fix is simple: to make it '' if it comes undef. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 106305 [details] [review] Bug 25440: Fixed list of 'show_rule' forming variables in the template In smart-rules.tt we have `SET show_rule = ...` section which filled with 'all used in the loop' variables. Because if historical reasons it seems that there are some missing, few old, and even doubled ones. This list is fixed now by: - variable names 'article_requests' and 'renewalsallowed' repeated so duplicates are removed; - 'hardduedatebefore' and 'hardduedateexact' not present in the whole site code anywhere anymore; IMPORTANT NOTE: these 'hardduedatebefore/hardduedateexact' also exists as remnants in .po-translation files, a lot. - 'note', 'hardduedatecompare', 'renewalperiod', 'rentaldiscount' template variables were missing from this 'show_rule =' checking code so they are added. Order of fields updated to match with above "SET field = ..." pack. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Trivial fixes, clear code. All works as expected and qa scripts pass. Going straight for a QA here :) Created attachment 106388 [details] [review] Bug 25440: (QA follow-up) Correction for typo 'engthunit -> lengthunit Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> <3 Pushed to master for 20.11, thanks to everybody involved! backported to 20.05.x for 20.05.02 Hi all, this does not apply cleanly on 19.11.x, please rebase if you'd like me to backport. |