Bug 7630

Summary: Warning on moremember.pl about param without key
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: PatronsAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: chris, fred.pierre, gmcharlt, mtompset, paul.poulain
Version: 3.8   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed Patch to the BUG
Bug 7630 - Warning on moremember.pl about param without key Move "$error => 1," to its own conditional $template->param()

Description Marcel de Rooy 2012-03-01 14:31:43 UTC
[Wed Feb 29 13:08:17 2012] [error] [client 82.173.53.17] [Wed Feb 29 13:08:17 2012] moremember.pl: Problem = a value of 1 has been passed to param without key at /usr/share/koha/testclone/C4/Templates.pm line 187.

Whoever finds it first ...
Comment 1 Mark Tompsett 2012-06-05 03:34:12 UTC
This is similar to bug 7352. However, I don't believe patron quick search is memberentry.pl

I was going to enter something similar, but since I think that would be a duplicate of this problem, I'll add this commentary with my debugging hunt.

C4::Template generated the following error log entry:
[Mon Jun 04 11:50:36 2012] [error] [client 192.168.100.2] [Mon Jun  4 11:50:36 2012] moremember.pl: Problem = a value of 1 has been passed to param without key at /usr/share/koha/lib/C4/Templates.pm line 186., referer: https://.../cgi-bin/koha/members/memberentry.pl


So I looked around line 186 of C4::Templates.pm
        if ( $key ) {
            $self->{VARS}->{$key} = $val;
        } else {
            warn "Problem = a value of $val has been passed to param without key";
        }

So I figured that moremember.pl must have $template->param( ... => 1 ...
Line 145 in moremember.pl looked like a candidate, but I figured not. $_ would have some value most likely. I kept looking (/\$template->param). I skipped a few lines which had concatenations, because I was hoping they'd be initialized and not be the problem. That's when I found line 445 which has a number of parameters across many lines. One of them was $error (around line 466).
        $error => 1,
I then decided to search for $error backwards (?\$error). That's when I saw the code of line 74:
        my $error = $input->param('error');

I then looked at memberentry.pl the only set of lines with moremember.pl mentioned are:
                $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber") :
                $input->redirect("/cgi-bin/koha/members/moremember.pl?borrowernumber=$borrowernumber") ;
        exit;           # You can only send 1 redirect!  After that, content or other headers don't matter.

-- If someone noticed this before, why is it here?
Also, notice there is no "&error=..." as well, so this isn't passing a value to $error.

I figure that something similar to bug 6840 can be done in moremember.pl
Split the $template->param() call:
-- original
++ tweak
$template->param(
    detailview => 1,
    AllowRenewalLimitOverride => C4::Context->preference("AllowRenewalLimitOverride"),
    DHTMLcalendar_dateformat => C4::Dates->DHTMLcalendar(),
    CANDELETEUSER    => $candeleteuser,
    roaddetails     => $roaddetails,
    borrowernumber  => $borrowernumber,
    othernames      => $data->{'othernames'},
    categoryname    => $data->{'description'},
    reregistration  => $reregistration,
    branch          => $branch,
    todaysdate      => C4::Dates->today(),
    totalprice      => sprintf("%.2f", $totalprice),
    totaldue        => sprintf("%.2f", $total),
    totaldue_raw    => $total,
    issueloop       => @issuedata,
    relissueloop    => @relissuedata,
        issuecount      => $issuecount,
    relissuecount   => $relissuecount,
    overdues_exist  => $overdues_exist,
-    error           => $error,
-    $error          => 1,
    StaffMember     => ($category_type eq 'S'),
    is_child        => ($category_type eq 'C'),
#   reserveloop     => \@reservedata,
    dateformat      => C4::Context->preference("dateformat"),
    "dateformat_" . (C4::Context->preference("dateformat") || '') => 1,
    samebranch     => $samebranch,
    quickslip             => $quickslip,
);
++$template->param(error => $error, $error => 1) if $error;
Comment 2 Mark Tompsett 2012-06-05 06:31:02 UTC
Actually, the template file (moremember.tt) has an error check:
[% IF ( error ) %]

So, I wondered how does it handle undefined values or empty strings. I found my answer: http://my.safaribooksonline.com/book/programming/perl/0596004761/template-directives/perltt-chp-4-sect-5
"If the result of this statement is 0 or "" (the empty string), test is considered to be false; everything else is true. Variables that have not been assigned a value, either with DEFAULT or SET, are considered to be false (the value of an undefined variable is an empty string)."

I was contemplating if my potential patch was overkill, or would break templates. It would seem that it wouldn't, though perhaps a simpler solution is better:
-- original
++ tweak
$template->param(
    detailview => 1,
    AllowRenewalLimitOverride => C4::Context->preference("AllowRenewalLimitOverride"),
    DHTMLcalendar_dateformat => C4::Dates->DHTMLcalendar(),
    CANDELETEUSER    => $candeleteuser,
    roaddetails     => $roaddetails,
    borrowernumber  => $borrowernumber,
    othernames      => $data->{'othernames'},
    categoryname    => $data->{'description'},
    reregistration  => $reregistration,
    branch          => $branch,
    todaysdate      => C4::Dates->today(),
    totalprice      => sprintf("%.2f", $totalprice),
    totaldue        => sprintf("%.2f", $total),
    totaldue_raw    => $total,
    issueloop       => @issuedata,
    relissueloop    => @relissuedata,
        issuecount      => $issuecount,
    relissuecount   => $relissuecount,
    overdues_exist  => $overdues_exist,
    error           => $error,
-    $error          => 1,
    StaffMember     => ($category_type eq 'S'),
    is_child        => ($category_type eq 'C'),
#   reserveloop     => \@reservedata,
    dateformat      => C4::Context->preference("dateformat"),
    "dateformat_" . (C4::Context->preference("dateformat") || '') => 1,
    samebranch     => $samebranch,
    quickslip             => $quickslip,
);
++$template->param($error => 1) if $error;
Comment 3 Mark Tompsett 2012-06-29 14:13:07 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2012-06-29 15:04:19 UTC
*** Bug 7352 has been marked as a duplicate of this bug. ***
Comment 5 Chris Cormack 2012-07-01 01:16:57 UTC
Created attachment 10581 [details] [review]
Bug 7630 - Warning on moremember.pl about param without key Move "$error => 1," to its own conditional $template->param()

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 6 Marcel de Rooy 2012-07-02 08:41:25 UTC
Good catch. Passed QA
Comment 7 Paul Poulain 2012-07-03 16:20:49 UTC
Patch pushed, welcome Mark, you're number #186 !
Comment 8 Chris Cormack 2012-07-04 19:24:04 UTC
Pushed to 3.8.x, will be in 3.8.3