Perl warning on the intranet "Holds" page (/cgi-bin/koha/reserve/request.pl): Argument "" isn't numeric in numeric gt (>) at .../C4/Reserves.pm line 2233.
Created attachment 105101 [details] [review] Bug 25533: Fix for 'Argument "" isn't numeric' warning Perl warning on the intranet "Holds" page (/cgi-bin/koha/reserve/request.pl) Argument "" isn't numeric in numeric gt (>) at .../C4/Reserves.pm line 2233. Fixed by assigning 0 for $holds_per_record when it has value that can be considered as false. To test: 1) Ensure that values of "Holds per record" is set to Unlimited on /cgi-bin/koha/admin/smart-rules.pl 2) Go to the intranet page (/cgi-bin/koha/reserve/request.pl). 3) Open item record, like /cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1 4) Observe the warning in the log file. 5) Apply patch. 6) Repeat steps 2 and 3. 7) Check that previous warning was suppressed. Mentored-by: Peter Vashchuk <stalkernoid@gmail.com>
Created attachment 105995 [details] [review] Bug 25533: Fix for 'Argument "" isn't numeric' warning Perl warning on the intranet "Holds" page (/cgi-bin/koha/reserve/request.pl) Argument "" isn't numeric in numeric gt (>) at .../C4/Reserves.pm line 2233. Fixed by assigning 0 for $holds_per_record when it has value that can be considered as false. To test: 1) Ensure that values of "Holds per record" is set to Unlimited on /cgi-bin/koha/admin/smart-rules.pl 2) Go to the intranet page (/cgi-bin/koha/reserve/request.pl). 3) Open item record, like /cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1 4) Observe the warning in the log file. 5) Apply patch. 6) Repeat steps 2 and 3. 7) Check that previous warning was suppressed. Mentored-by: Peter Vashchuk <stalkernoid@gmail.com> Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Thanks for your patch ! You remove the warning, but are you interpreting the value correctly? - my $holds_per_record = $rule ? $rule->{holds_per_record} : 0; + my $holds_per_record = $rule ? $rule->{holds_per_record} || 0 : 0; $max = $holds_per_record if $holds_per_record > $max; If space or empty string means Unlimited and we replace it by 0, would our compares be correct ? Note that perl now does the same thing but removing the warn will only hide the problem you found in this code ! I see that code is calculating the number of remaining holds: my $remaining_holds_for_record = $max_holds_for_record - $holds->count(); If max is 0 but should mean Unlimited, this definitely goes wrong ?
(In reply to Marcel de Rooy from comment #3) > Thanks for your patch ! > You remove the warning, but are you interpreting the value correctly? > > - my $holds_per_record = $rule ? $rule->{holds_per_record} : 0; > + my $holds_per_record = $rule ? $rule->{holds_per_record} || 0 : 0; > $max = $holds_per_record if $holds_per_record > $max; > > If space or empty string means Unlimited and we replace it by 0, would our > compares be correct ? > Note that perl now does the same thing but removing the warn will only hide > the problem you found in this code ! > I see that code is calculating the number of remaining holds: > > my $remaining_holds_for_record = $max_holds_for_record - $holds->count(); > > If max is 0 but should mean Unlimited, this definitely goes wrong ? Good catch
Yes, we will re-investigate and analyze/argue or re-create the fix, few days please :).
(In reply to Andrii Nugged from comment #5) > Yes, we will re-investigate and analyze/argue or re-create the fix, few days > please :). Ping?