Summary: | Perl "not numeric" warning on the "Holds" page | ||
---|---|---|---|
Product: | Koha | Reporter: | Peter Vashchuk <stalkernoid> |
Component: | Hold requests | Assignee: | Slava Shishkin <slavashishkin> |
Status: | Failed QA --- | QA Contact: | Testopia <testopia> |
Severity: | trivial | ||
Priority: | P5 - low | CC: | emmi.takkinen, gmcharlt, m.de.rooy, martin.renvoize, nugged |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 25790 | ||
Attachments: |
Bug 25533: Fix for 'Argument "" isn't numeric' warning
Bug 25533: Fix for 'Argument "" isn't numeric' warning |
Description
Peter Vashchuk
2020-05-18 16:11:16 UTC
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? |