Bug 8861 - Undefined variables in batchMod.pl trigger error logs
Summary: Undefined variables in batchMod.pl trigger error logs
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-02 12:56 UTC by Mark Tompsett
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

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


Attachments
Fix inconsequential error loggling in batchMod.pl (1.46 KB, patch)
2012-10-02 13:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Fix inconsequential error loggling in batchMod.pl (3.8, 3.6) (1.44 KB, patch)
2012-10-02 13:47 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8861 - Undefined variables in batchMod.pl trigger error logs (1.52 KB, patch)
2012-10-05 13:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8861 - Undefined variables in batchMod.pl trigger error logs (3.8, 3.6) (1.49 KB, patch)
2012-10-05 13:40 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-02 12:56:09 UTC
This module was looked at because of:
[Tue Oct 02 17:00:56 2012] [error] [client 192.168.100.2] [Tue Oct  2 17:00:56 2012] batchMod.pl: Problem = a value of 1 has been passed to param without key at /usr/share/koha/lib/C4/Templates.pm line 187.

There are two sections that match, though this was likely the $op case, not the $error case:

$template->param(
    op      => $nextop,
    $op => 1,
);

foreach my $error (@errors) {
    $template->param($error => 1);
}

I would normally change:
    my $op           = $input->param('op');
into
    my $op           = $input->param('op') || q{};
However there was code that read:
    if (!defined $op) {
So, I looked to see how $op was used after that:
    if ($op eq "action") {
    if ($op eq "show"){
Of course, the above error'ing code, and lastly.
    if ($op eq "action") {
As such, I decided to define $op = q{} inside the if statement, after it ran it's !defined check. And I still moved the $op => 1 to an external, conditional template param call.

Additionally, I looked for where @errors was used in the file, but only the declaration, and this foreach had a @errors reference. And as an array could be filled with undef's, I err'd on the side of caution by adding "if $error" on the template parameter call.

This was found in 3.8.5. Looks like it is master too.
Patch to follow shortly.
Comment 1 Mark Tompsett 2012-10-02 13:45:41 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2012-10-02 13:47:37 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-10-05 13:38:10 UTC
Created attachment 12708 [details] [review]
[SIGNED-OFF] Bug 8861 - Undefined variables in batchMod.pl trigger error logs

Initialized $op, and changed lines like "$op => 1" and
"$error => 1" into separate, conditional template param calls.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall 2012-10-05 13:40:11 UTC
Created attachment 12709 [details] [review]
[SIGNED-OFF] Bug 8861 - Undefined variables in batchMod.pl trigger error logs (3.8, 3.6)

Initialized $op, and changed lines like "$op => 1" and
"$error => 1" into separate, conditional template param calls.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Jonathan Druart 2012-10-18 14:17:32 UTC
Looks good, marked as Passed QA.
Comment 6 Paul Poulain 2012-10-19 15:48:47 UTC
Patch pushed to master
Comment 7 Chris Cormack 2013-04-21 00:51:07 UTC
Pushed to 3.8.x will be in 3.8.12