Bug 4491 - Cleaning some weird code in *search.pl
Summary: Cleaning some weird code in *search.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.10
Hardware: All All
: P3 trivial (vote)
Assignee: Jonathan Druart
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-12 01:58 UTC by Nahuel Angelinetti
Modified: 2014-05-26 21:04 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 4491: fix weird code in search scripts (5.38 KB, patch)
2012-07-24 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4491: FIX buildQuery if called with an empty limit (1.17 KB, patch)
2012-08-22 12:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4491: fix weird code in search scripts (5.48 KB, patch)
2012-08-27 17:05 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 4491: FIX buildQuery if called with an empty limit (1.50 KB, patch)
2012-08-27 17:06 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:27:48 UTC


---- Reported by nahuel.angelinetti@biblibre.com 2010-05-12 13:58:58 ----

There is some weird code in opac-search.pl and search.pl to parse some multivalued variable that broke index search in some cases.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:27 UTC  ---

This bug was previously known as _bug_ 4491 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4491

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.

Comment 1 Owen Leonard 2010-07-06 19:19:12 UTC
Patch: http://lists.koha-community.org/pipermail/koha-patches/2010-May/011815.html

I see the patch was pushed to rel_3_0 but the old code is still in HEAD. Does this fix need to be ported to HEAD?
Comment 2 Chris Cormack 2010-12-13 03:39:53 UTC
Yep, this needs porting to master, if people want it in there
Comment 3 Jonathan Druart 2012-07-24 14:16:21 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-07-25 01:48:13 UTC
Jonathan

At first glance this looks good, but do you have an example of a search that provides bad results before this patch, and that is fixed by it please?
Comment 5 Jonathan Druart 2012-07-25 11:25:11 UTC
(In reply to comment #4)
Chris,
> At first glance this looks good, but do you have an example of a search that
> provides bad results before this patch, and that is fixed by it please?

There is no bug before the patch.
It is purely esthetic :)

It replaces the dumpy slit('\0') with a more "graceful" cgi->param (which is used everywhere into Koha).
Comment 6 Chris Cormack 2012-07-25 22:56:57 UTC
Oh interesting

Because it was reported as

"here is some weird code in opac-search.pl and search.pl to parse some multivalued variable that broke index search in some cases."

And then 
"This change some code a bit strange, that generate strange effetcs.
Now we use $cgi->param to get the values."

If its purely aesthetic, then it is definitely not major. If however Nahuel was right we can bump it back up to major.
Comment 7 Jared Camins-Esakov 2012-07-28 13:27:50 UTC
I, too, am confused about what I'm supposed to testing here. Perhaps you could clarify what you meant by "that generate strange effects"?
Comment 8 Jonathan Druart 2012-07-30 08:26:30 UTC
(In reply to comment #6 and comment #7)

Nahuel is not reachable currently. And I don't know if he remembers about this bug :)
But when I accidentally found this bug, it seemed to me good to replace the dumpy split.
If you want I can re-submit the patch by changing the comment to underline this point.
Comment 9 Jared Camins-Esakov 2012-08-11 12:03:46 UTC
This patch breaks the OPAC search with the following message in the log:

[Sat Aug 11 08:02:14 2012] [error] [client 192.168.56.1] [Sat Aug 11 08:02:13 2012] opac-search.pl: WARNING: query problem with (rk=(Title-cover,ext,r1="smith" or ti,ext,r2="smith" or Title-cover,phr,r3="smith" or wrdl,right-Truncation,r9="smith? " or wrdl,r9="smith")) and ZOOM error 10014 "CCL parsing error" (addinfo: "Search word expected") from diag-set 'ZOOM' at /home/jcamins/kohaclone/C4/Search.pm line 356., referer: http://192.168.56.57/cgi-bin/koha/opac-search.pl?limit=

(my search was "smith")

The staff client search works fine.
Comment 10 Jonathan Druart 2012-08-22 12:13:13 UTC Comment hidden (obsolete)
Comment 11 Jared Camins-Esakov 2012-08-27 17:05:59 UTC
Created attachment 11847 [details] [review]
Bug 4491: fix weird code in search scripts

This change some code a bit strange, that generate strange effetcs.
Now we use $cgi->param to get the values.

Test: search at the opac and intranet using limit, etc.
Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 12 Jared Camins-Esakov 2012-08-27 17:06:14 UTC
Created attachment 11848 [details] [review]
Bug 4491: FIX buildQuery if called with an empty limit

if @limit=('') buildQuery failed

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
All searches that I tried (keyword, indexed, CCL, with limits, without,
etc.) worked fine. There are warnings about uninitialized variables in
the OPAC, but they exist on master as well and therefore should not
block these patches.
Comment 13 Mason James 2012-09-14 15:05:14 UTC
> 
> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>

patches looks good, passing QA…

mason@xen1:~/g/head$ koha-qa.pl  -c 2

testing 2 commit(s) (applied to commit 0acebb8)
 * 65911df Bug 4491: FIX buildQuery if called with an empty limit
      C4/Search.pm

 * d8b1b52 Bug 4491: fix weird code in search scripts
      catalogue/search.pl
      opac/opac-search.pl

* C4/Search.pm                                                             OK
* catalogue/search.pl                                                      OK
* opac/opac-search.pl                                                      OK
Comment 14 Paul Poulain 2012-09-17 16:22:25 UTC
Patch pushed to master

I think there's no need to push it to 3.8, (just in case we missed some side effect)
Comment 15 Fridolin SOMERS 2013-04-17 13:28:08 UTC
(In reply to comment #14)
> Patch pushed to master
> 
> I think there's no need to push it to 3.8, (just in case we missed some side
> effect)

Second patch (FIX buildQuery if called with an empty limit) is needed.