Bug 6786

Summary: False detection of index names in Search; make index names case insensitive
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: SearchingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Ian Walls <koha.sekjal>
Severity: critical    
Priority: PATCH-Sent (DO NOT USE) CC: chris, f.demians, katrin.fischer, paul.poulain
Version: 3.6   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Patch
Signed-off patch
Follow up fix for hyphens in index names
Bug 6786: Follow up fixing index names with hyphens

Description Marcel de Rooy 2011-08-25 11:17:35 UTC
An erroneous regex in Search.pm makes that some search expressions are marked ccl= queries while they do not contain an index name.
Note that these queries contain a : or = (Users should not enter them if they do not intend to use indexes, but unfortunately they do.) 

Example:
opac-search.pl?q=rembrandt+%3A contains the index name "an" (for authority); this search results in an 500 error.
opac-search.pl?q=hals+%3A does not contain an index name and gives results
opac-search.pl?q=vermeer+%3A does not contain an index name and gives results

A tiny adjustment in the regex and a change in the loop structure makes the code slightly faster and solves the false index detections. While doing that, I included a comparison with lowercase. This makes index names in searches case insensitive.
Comment 1 Marcel de Rooy 2011-08-25 11:32:43 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2011-08-25 11:36:11 UTC
*** Bug 6317 has been marked as a duplicate of this bug. ***
Comment 3 Frédéric Demians 2011-09-01 12:04:25 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2011-10-06 14:27:39 UTC
QA comment
* perfect test case
* small patch, nothing to say from a QA POV

marking passed QA !
Comment 5 Chris Cormack 2011-10-07 01:10:53 UTC
Pushed, please test
Comment 6 Katrin Fischer 2011-12-04 08:56:23 UTC
Hi, 

this breaks searches for Control-number as used to link back from analytics to serials using $w in MARC21. 

To test:
- Add a record and fill in 001
- Try to search for Control-number:<001> in a keyword search

This needs to be reverted until a better fix can be found that does not break existing features.
Comment 7 Marcel de Rooy 2011-12-04 16:08:01 UTC
(In reply to comment #6)
> this breaks searches for Control-number as used to link back from analytics to
> serials using $w in MARC21. 
> 
> This needs to be reverted until a better fix can be found that does not break
> existing features.

Good catch, Katrin. I think we should not revert it; only a small change in the regex in buildQuery is needed. Problem here seems to be the hyphen in Control-number.

I tested this one:
/(?:^|\W)([\w-]+)(,[\w-]+)*[:=]/g
And it seems to work.

I hope to send a follow up patch for this tomorrow. If you are faster, that is fine too.
Comment 8 Marcel de Rooy 2011-12-05 07:47:49 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2011-12-05 07:54:05 UTC
Created attachment 6573 [details] [review]
Bug 6786: Follow up fixing index names with hyphens

Adds hyphen to regex looking for index names in buildQuery.
Test by searching on Control-number=...

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 10 Marcel de Rooy 2011-12-05 07:56:23 UTC
QA Request for Paul or Ian:
Could you please set this to Passed QA and push it. Resolves searching on index names with hyphen.
Comment 11 Marcel de Rooy 2011-12-05 08:09:50 UTC
Can and should be applied to 3.6.X too.
Comment 12 Ian Walls 2011-12-05 14:52:59 UTC
Adds hyphen back into regex, which is required for many/most of the search indexes in bib1.att.

Please note that / is also used in several of the bib1.att indexes... now as to whether we actually support those particular indexes, I'm not sure, but theoretically, we should probably also include regex matching for / as well.

Passed QA
Comment 13 Paul Poulain 2011-12-06 10:50:14 UTC
follow-up patch pushed, please test

Chris: the patch was pushed on new/bug_6786 branch, but it should have been a follow-up. Where did you pushed the initial fix ? did I missed something ?
Comment 14 Katrin Fischer 2011-12-18 20:43:14 UTC
I see both patches in master and 3.6.x now. Search for "Control-number" works in master.