Summary: | Uninitialized variable errors in opac-search.pl | ||
---|---|---|---|
Product: | Koha | Reporter: | Owen Leonard <oleonard> |
Component: | OPAC | Assignee: | Mark Tompsett <mtompset> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | normal | ||
Priority: | P5 - low | CC: | chris, jonathan.druart, julian.maurice, mtompset, paul.poulain |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Optimized two if statements, but otherwise fixed already in master.
[SIGNED-OFF] Bug 4064 - Uninitialized variable errors in opac-search.pl Merely optimized two if statements, already fixed in master. Optimized two if statements (3.8.x) |
Description
Chris Cormack
2010-05-21 01:22:44 UTC
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 |