---- Reported by oleonard@myacpl.org 2010-01-26 17:19:26 ---- I'm seeing a couple of errors in my error log coming from opac-search.pl: opac-search.pl: Use of uninitialized value $borrowernumber in string eq at /home/oleonard/kohaclone/opac/opac-search.pl line 465. ...referring to: if ($borrowernumber eq '') { (error occurs if the user is not logged in). And: opac-search.pl: Use of uninitialized value in string eq at /home/oleonard/kohaclone/opac/opac-search.pl line 468. ...referring to: if ($params->{'offset'} eq '') { I notice that on line 329 a variable $offset is initialized like this: my $offset = $params->{'offset'} || 0; ...which made me wonder whether the later check for $params->{'offset'} should be checking $offset instead? But it's not clear to me how that's being used. --- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:22 UTC --- This bug was previously known as _bug_ 4064 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4064 Actual time not defined. Setting to 0.0 Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one.
Created attachment 10572 [details] [review] Optimized two if statements, but otherwise fixed already in master. I was hunting through opac-search.pl and only found two references to $params->{'offset'} other than the initialization line that oleonard pointed out. 616: if (($params->{'offset'}||'') eq '') { 594: if (($params->{'offset'}||'') eq '') { If the parameter is undefined, the two lines above will be true. If the parameter is an empty string, the two lines above will be true. If the parameter is 0 or "0", the two lines above will be true. If the parameter is a non-zero value or an otherwise non-empty string, the two lines above will be false. It would seem to me that the above code on lines 594 and 616 in master is sufficient. Additionally, oleonard mentioned $borrowernumber. 592: if (!$borrowernumber || $borrowernumber eq '') { This is the only line that resembles what oleonard referred to. It would seem to me that the above code on line 592 in master is sufficient. The initialization of $offset is now on line 400 in master: my $offset = $params->{'offset'} || 0; This implies that the parameter is expected to be numeric, not a string. I think oleonard's observation that there is a $offset variable is useful to note as everywhere else in opac-search.pl uses the $offset variable. Based on the initialization line above: If the parameter is undefined, $offset is 0. If the parameter is an empty string, $offset is 0. If the parameter is 0 or "0", $offset is 0. If the parameter is a non-zero value or an otherwise non-empty string, $offset is that value or string. This means that lines 616 and 594 could be optimized to: 616: if (!$offset) { 594: if (!$offset) { Lines 592 and 594 correspond to the two lines oleonard mentions. This means I believe the bug is fixed already. I am changing the status to Needs Signoff, as this is not NEW. I'm attaching a patch accordingly, but feel free to reject it and just close this.
Created attachment 11384 [details] [review] [SIGNED-OFF] Bug 4064 - Uninitialized variable errors in opac-search.pl Merely optimized two if statements, already fixed in master. Simple patch, makes code clearer.
QA Comments: Remove a muzzy comparison in opac-search.pl Passed QA
Question: why do you write "already fixed in master." in your commit comment ? the patch applies to master, and it should be pushed so... patch pushed
When I said fixed in master, I meant there were warning messages generated in the error log file for 3.6.3, but that the ugly logic which was there solved the problem. I merely attempted to wade through the ugly logic to find a more elegant solution. Pushing it is perfectly fine with me, but it also applies to 3.6.x and 3.8.x too, which is why I have changed the version to unspecified.
Patch does not apply cleanly to 3.8.x. Please submit a patch formatted against 3.8.x if you want it in there.
Created attachment 11948 [details] [review] Optimized two if statements (3.8.x) Thanks for letting me know, Chris. Someone added a comment which seems to have thrown the first patch off. Here is a patch which applies to 3.8.x, and hopefully 3.6.x too. If there are problems, please let me know.
The 3.8.x patch applies fine, pushed, will be in 3.8.5