Bug 6840

Summary: error in Template.pm in logs
Product: Koha Reporter: Paul Poulain <paul.poulain>
Component: Architecture, internals, and plumbingAssignee: Paul Poulain <paul.poulain>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: minor    
Priority: PATCH-Sent (DO NOT USE) CC: chris, fred.pierre, koha.sekjal, mtompset
Version: 3.6   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: proposed patch
Signed-off patch

Description Paul Poulain 2011-09-02 16:34:50 UTC
In staff interface, in search (catalogue/search.pl), there is a warning saying:
[Fri Sep  2 18:20:24 2011] search.pl: Use of uninitialized value $key in hash element at /home/paul/koha.dev/koha-community/C4/Templates.pm line 227.

The problem happens when searching from the quick search toolbar, not when you search from advanced search.
Investigating, it seems it's because the sort_by is not defined in this case.

Patch will follow
Comment 1 Paul Poulain 2011-09-02 16:40:27 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2011-09-30 18:41:13 UTC
Created attachment 5671 [details] [review]
Signed-off patch

Before: Error appears in the log each time a search is done from the header search form.
After: No error appears.
Comment 3 Ian Walls 2011-10-12 21:25:09 UTC
Easy patch to read, well-documented.  Adds conditional checks and clearer error messages.  Marking as Passed QA
Comment 4 Chris Cormack 2011-10-13 03:18:34 UTC
No template changes, pushed please test
Comment 5 Fred P 2011-12-13 16:52:38 UTC
  We were receiving the same error while searching patrons from the patron quick search. The patch is helpful, but still generates tons of log messages, so we altered the moremember.pl around line 474 to omit the "$error => 1," editing the error parameter as follows:

---------------------------------------------------
    overdues_exist  => $overdues_exist,
    error           => 0,
    StaffMember     => ($category_type eq 'S'),
---------------------------------------------------


then we added this to the moremember.pl at line 485:
---------------------------------------------------

#addition to test error parameter status
if ( $error ) {
	$template -> param (
		$error => 1,
		error => 1,
	);
}

-----------------------------------------
Comment 6 Jared Camins-Esakov 2012-05-23 12:26:25 UTC
It sounds like there are still problems, so resetting status to ASSIGNED. Please close if I have misunderstood the comments.
Comment 7 Mark Tompsett 2012-06-05 07:58:50 UTC
This patch is fine, in my opinion. The comment made by Fred P 2011-12-13 16:52:38 UTC is a different problem, in my reading. I proposed a solution to his bug in bug 7630 (see also bug 7352).

The problem is not all modules call with the 'error' parameter set. And Fred P's suggestion of:
#addition to test error parameter status
if ( $error ) {
	$template -> param (
		$error => 1,
		error => 1,
	);
}
is overkill compared to simply splitting up the large $template->param() call,
and having
$template->param($error => 1) if $error;
separately from the long list.

No, I didn't do any testing. I just don't want a good patch wasted. :)
Comment 8 Chris Cormack 2012-06-05 08:54:54 UTC
(In reply to comment #7)
> This patch is fine, in my opinion. The comment made by Fred P 2011-12-13
> 16:52:38 UTC is a different problem, in my reading. I proposed a solution to
> his bug in bug 7630 (see also bug 7352).
> 
> The problem is not all modules call with the 'error' parameter set. And Fred
> P's suggestion of:
> #addition to test error parameter status
> if ( $error ) {
> 	$template -> param (
> 		$error => 1,
> 		error => 1,
> 	);
> }
> is overkill compared to simply splitting up the large $template->param()
> call,
> and having
> $template->param($error => 1) if $error;
> separately from the long list.
> 
> No, I didn't do any testing. I just don't want a good patch wasted. :)

Well setting $error=>1 is different because the value of $error isn't 'error' so you would have to change the template as well. To match that in your patch you arent setting error in the template
Comment 9 Mark Tompsett 2012-06-05 10:38:07 UTC
(In reply to comment #8)
> (In reply to comment #7)
...
> > Fred P's suggestion of:
> > #addition to test error parameter status
> > if ( $error ) {
> > 	$template -> param (
> > 		$error => 1,
> > 		error => 1,
> > 	);
> > }
> > is overkill compared to simply splitting up the large $template->param()
> > call,
> > and having
> > $template->param($error => 1) if $error;
> > separately from the long list.
> > 
> > No, I didn't do any testing. I just don't want a good patch wasted. :)
> 
> Well setting $error=>1 is different because the value of $error isn't
> 'error' so you would have to change the template as well. To match that in
> your patch you arent setting error in the template

True it should be:
> > 		$error => 1,
> > 		error => $error,
I was merely quoting from comment #5

Otherwise, it would potentially break:
upload-images.tt
edithelp.tt
addbooks.tt
paycollect.tt
step2.tt (the web installer!)
step3.tt (yes, the web installer!)
sms-home.tt

But, it would probably still work in:
linkitem.tt
moveitem.tt
senshelfform.tt
moremember.tt
label-manage.tt
clone-rules.tt
sendbasketform.tt
opac-sendbasketform.tt
opac-sendshelfform.tt

On a git repo of master, I did a:
$ vi `grep -l "IF ( error )" \`find . -name '*.tt'\``
This obviously may miss the plain [% error %]. So I did a:
$ vi `grep -l "% error %" \`find . -name '*.tt'\``
And it returned 6 of the 7 files I listed as potentially broken.

And as I said before, Fred P.'s suggestion to fix what bug he did find is better served by taking the '$error => 1' out of the larger $template->param() section and putting:
    $template->param($error => 1) if $error;
just after that large section. The bug described by Fred P. is likely the same as the one discussed in bug 7352 and bug 7630.

In fact, I decided to do a:
$ grep "\$error\s*=>\s1" `find .`
I found:
./tools/batchMod.pl
./cataloguing/additem.pl
./members/moremember.pl
./members/member-password.pl
./members/memberentry.pl
If $error is undefined, all these modules will have problems. I did not look further, as that is beyond the scope of this error and the two bug reports I have mentioned. The patch viewable by diff is perfectly sufficient for the bug described in this bug report. However, as I said before Fred P.'s suggestion is better addressed by bug 7352 and bug 7630 being fixed as suggested above.

A warning, which this patch generates, is better than the error generated by not having it. Clearly this patch is functioning, because it seems to be in 3.6.3 which we are currently running, and I found the other errors as a result of this cleanly generated "error" (really a warning), rather than an ugly error.

In short, I think the status should be closed, so focus on bug 7352 and bug 7630 can be dealt with, which is what I believe Fred P. encountered.
Comment 10 Mark Tompsett 2012-06-29 14:58:16 UTC
I have posted a patch on bug 7630 which should address Fred P's issues. This patch is still live. I like it. It's a friendly warning, rather than an error to track down.

Thanks to jcamins for helping me get my DEV setup done. :)

Can someone close this bug? It's not an issue, in my opinion.
Comment 11 Jared Camins-Esakov 2012-07-31 17:42:49 UTC
At Mark's request I have looked into it a bit more, and can confirm that this bug has been fixed. Closing.