Bug 8880 - Uninitialized variable triggers error log entry in circulation.pl
Summary: Uninitialized variable triggers error log entry in circulation.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Mark Tompsett
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-05 12:32 UTC by Mark Tompsett
Modified: 2013-12-05 20:06 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Added/Replaced with "|| q{}" into variable initialization [3.8.x] (2.52 KB, patch)
2012-10-05 13:13 UTC, Mark Tompsett
Details | Diff | Splinter Review
Added/Replaced with "|| q{}" into variable initialization (2.37 KB, patch)
2012-10-05 13:16 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8880 - Uninitialized variables trigger error log entries (2.27 KB, patch)
2012-11-09 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2012-10-05 12:32:15 UTC
I am using bug 8777 as a master bug with several sub-bug reports, as various pieces may be accepted without issue, while others may need rebasing/refixing which would hinder one giant patch.

This update overlaps with bug 8826, because the initialization of a hash is within three lines of part of this fix. As such, I have made the same changes in this patch. That is changed the initialization of the hashes from {} to ().

To verify potential side effects, I did a search through the circulation.pl file for all occurrences of the variable initialized.

This patch initializes $findborrower
The error was triggered by trying to substitute on the very next line for an undefined findborrower. An if statement on $findborrow functions the same for undefined and q{}. And TT treats undefined as q{} as well. The || q{} should prevent the error and have no changes in logic.

This patch initializes $barcode
The error was triggered by trying to substitute on the very next line for an undefined barcode. The if condition on the following line behaves the same for undefined and q{}. The stickyduedate check if condition on $barcode behaves the same. The if condition to check "if we should print" now doesn't generate an error log entry. The inprocess initialization should not trigger an error log now. The next line's if still generates an error log due to a lack of the charges parameter, not due to an undefined barcode. The "STEP 3 : ISSUING" if condition behaves the same. There is no need to check inside the if, because q{} does not run that code (just like undefined didn't). And TT treats undefined as q{} as well. The || q{} should prevent errors and have no changes in logic.

This patch initializes $print.
Make sure not to confuse it with $printer. :) The "if we should print" if condition no longer generates an error log. However, the if condition containing "$borrowernumber ne ''" still generates an error log, as $borrowernumber is not initialized. Tracing the logic for $borrowernumber is more difficult as external functions are called using it. The || q{} for $print prevents an error and has no effect on the processing logic.

Test Plan
---------
1) Login to staff client
2) Circulation
3) Click the 'Check out' link.
4) Go check the error log.
Currently under 3.8.5 this generates error log entries for lines:
97,101,163,168,175,610,644, and 660 (in my testing on 3.8.5).
5) apply patch.
6) Home
7) Circulation
8) Click the 'Check out' link.
9) Go check the error log.
This should resolve lines 97, 101, 163, and 175 (under 3.8.5)

Patches to follow as master and 3.8.x differ.
Comment 1 Mark Tompsett 2012-10-05 13:13:43 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2012-10-05 13:16:25 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-11-09 15:50:10 UTC
Created attachment 13358 [details] [review]
[SIGNED-OFF] Bug 8880 - Uninitialized variables trigger error log entries

Simply by logging in, clicking Circulation, clicking the
'Check out' link, error log entries were generated.

This patch ensures $findborrower, $barcode, and $print are all
initialized by adding "|| q{}" onto their initializations.
This reduces the silent error log entries generated.

As this patch would overlap with bug 8826 in the initialization
of hashes to prevent other error logs, those changes are in this
patch. Applying this patch without having applied the patch for
bug 8826.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Jared Camins-Esakov 2012-11-10 14:00:23 UTC
This was signed off yesterday by Kyle M Hall.
Comment 5 Jonathan Druart 2012-11-20 11:45:16 UTC
QA Comments:

Remove warning in log.

Marked as Passed QA.
Comment 6 Jared Camins-Esakov 2012-11-20 12:35:20 UTC
This patch has been pushed to master.
Comment 7 Paul Poulain 2012-11-20 15:27:25 UTC
Patch pushed to branch 3.10.x
Comment 8 Chris Cormack 2013-04-23 09:51:07 UTC
Released in 3.10.0