Bug 27581 - Rename UseICU system preference to UseICUStyleQuotes
Summary: Rename UseICU system preference to UseICUStyleQuotes
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-29 19:46 UTC by Nick Clemens
Modified: 2021-06-14 21:35 UTC (History)
5 users (show)

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


Attachments
Bug 27581: Rename USeICU preference to UseICUStyleQuotes (11.66 KB, patch)
2021-01-29 20:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27581: Rename USeICU preference to UseICUStyleQuotes (11.73 KB, patch)
2021-01-29 20:26 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 27581: Rename USeICU preference to UseICUStyleQuotes (10.42 KB, patch)
2021-02-01 12:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27581: Rename USeICU preference to UseICUStyleQuotes (10.48 KB, patch)
2021-02-07 11:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27581: (QA follow-up) Fix occurence in it-IT custom.sql (1.47 KB, patch)
2021-02-07 11:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27581: Fix missing occurrences in tests (1.27 KB, patch)
2021-02-08 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-01-29 19:46:02 UTC
This system preference has no effect on whether ICU is used or not, it simply switches the character used to quote subjects for searching.

I am not really sure whether this pref is needed, or why other search indexes don't seem to be affected by it.

While we should probably look towards removing it, the first step is to aptly describe it
Comment 1 Nick Clemens 2021-01-29 20:04:13 UTC
Created attachment 116078 [details] [review]
Bug 27581: Rename USeICU preference to UseICUStyleQuotes

Rather thna have a poorly named preference with an explanation of how it doesn't
do what it implies it does, let's rename it!

To test:
 1 - Set UseICU to 'Using'
 2 - Go to the details page of a record in the staff interface
 3 - Hover over a subject heading that has subfields
 4 - Note the link has curly brackets around the subject, like:
     http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=su:{Winnie-the-Pooh Fictitious character}
 5 - Change UseICU to 'Not using'
 6 - Note the link is now:
     http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=su:"Winnie-the-Pooh Fictitious character"
 7 - Repeat on OPAC
 8 - Apply patch
 9 - Restart all
10 - Repeat tests, the behaviour has not changed
11 - Read the new syspref description and confirm it makes sense
Comment 2 Andrew Fuerste-Henry 2021-01-29 20:26:22 UTC
Created attachment 116082 [details] [review]
Bug 27581: Rename USeICU preference to UseICUStyleQuotes

Rather thna have a poorly named preference with an explanation of how it doesn't
do what it implies it does, let's rename it!

To test:
 1 - Set UseICU to 'Using'
 2 - Go to the details page of a record in the staff interface
 3 - Hover over a subject heading that has subfields
 4 - Note the link has curly brackets around the subject, like:
     http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=su:{Winnie-the-Pooh Fictitious character}
 5 - Change UseICU to 'Not using'
 6 - Note the link is now:
     http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=su:"Winnie-the-Pooh Fictitious character"
 7 - Repeat on OPAC
 8 - Apply patch
 9 - Restart all
10 - Repeat tests, the behaviour has not changed
11 - Read the new syspref description and confirm it makes sense

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 3 Katrin Fischer 2021-01-31 12:36:46 UTC
Hi Nick, I am sorry, but some unrelated changes slipped into this patch:
 koha-tmpl/intranet-tmpl/prog/en/modules/tags/review.tt
Comment 4 Nick Clemens 2021-02-01 12:06:39 UTC
Created attachment 116154 [details] [review]
Bug 27581: Rename USeICU preference to UseICUStyleQuotes

Rather thna have a poorly named preference with an explanation of how it doesn't
do what it implies it does, let's rename it!

To test:
 1 - Set UseICU to 'Using'
 2 - Go to the details page of a record in the staff interface
 3 - Hover over a subject heading that has subfields
 4 - Note the link has curly brackets around the subject, like:
     http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=su:{Winnie-the-Pooh Fictitious character}
 5 - Change UseICU to 'Not using'
 6 - Note the link is now:
     http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=su:"Winnie-the-Pooh Fictitious character"
 7 - Repeat on OPAC
 8 - Apply patch
 9 - Restart all
10 - Repeat tests, the behaviour has not changed
11 - Read the new syspref description and confirm it makes sense

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 5 Nick Clemens 2021-02-01 12:07:01 UTC
(In reply to Katrin Fischer from comment #3)
> Hi Nick, I am sorry, but some unrelated changes slipped into this patch:
>  koha-tmpl/intranet-tmpl/prog/en/modules/tags/review.tt

Oops, reverted those
Comment 6 Katrin Fischer 2021-02-07 11:09:56 UTC
Created attachment 116455 [details] [review]
Bug 27581: Rename USeICU preference to UseICUStyleQuotes

Rather thna have a poorly named preference with an explanation of how it doesn't
do what it implies it does, let's rename it!

To test:
 1 - Set UseICU to 'Using'
 2 - Go to the details page of a record in the staff interface
 3 - Hover over a subject heading that has subfields
 4 - Note the link has curly brackets around the subject, like:
     http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=su:{Winnie-the-Pooh Fictitious character}
 5 - Change UseICU to 'Not using'
 6 - Note the link is now:
     http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=su:"Winnie-the-Pooh Fictitious character"
 7 - Repeat on OPAC
 8 - Apply patch
 9 - Restart all
10 - Repeat tests, the behaviour has not changed
11 - Read the new syspref description and confirm it makes sense

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2021-02-07 11:10:00 UTC
Created attachment 116456 [details] [review]
Bug 27581: (QA follow-up) Fix occurence in it-IT custom.sql
Comment 8 Jonathan Druart 2021-02-08 13:48:07 UTC
Created attachment 116498 [details] [review]
Bug 27581: Fix missing occurrences in tests
Comment 9 Jonathan Druart 2021-02-08 13:57:54 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 10 Katrin Fischer 2021-02-08 14:33:28 UTC
Sorry, not sure how the test ones escaped me. hmpf.
Comment 11 Fridolin Somers 2021-02-12 13:15:55 UTC
I prefer not change syspref on stable branch.
This may be used in home-made XSLT files.

Not pushed to 20.11.x
Comment 12 Caroline Cyr La Rose 2021-02-22 17:45:24 UTC
Just putting here what the manual said about this pref.

ICU is a set of code libraries providing Unicode and Globalization support for software applications. What this means is ICU Zebra indexing is only necessary if you use non-roman characters in your cataloging. If using ICU Zebra indexing you will want to not use QueryFuzzy.

This setting will not affect Zebra indexing, it should only be used
to tell Koha that you have activated ICU indexing if you have
actually done so, since there is no way for Koha to figure this out
on its own.

Does the 'new' system preference still block QueryFuzzy from working? Do the different quotes (either double quotes or curly brackets) have an influence when tracing subjects using non-roman characters?
Comment 13 Nick Clemens 2021-02-22 18:31:01 UTC
(In reply to Caroline Cyr La Rose from comment #12)
> Just putting here what the manual said about this pref.
> 
> ICU is a set of code libraries providing Unicode and Globalization support
> for software applications. What this means is ICU Zebra indexing is only
> necessary if you use non-roman characters in your cataloging. If using ICU
> Zebra indexing you will want to not use QueryFuzzy.
> 
> This setting will not affect Zebra indexing, it should only be used
> to tell Koha that you have activated ICU indexing if you have
> actually done so, since there is no way for Koha to figure this out
> on its own.
> 
> Does the 'new' system preference still block QueryFuzzy from working? Do the
> different quotes (either double quotes or curly brackets) have an influence
> when tracing subjects using non-roman characters?

There are several places on the wiki that cover using ICU chains - it is much more involved than a syspref swtich (well, it's a switch in two config files)
https://wiki.koha-community.org/wiki/ICU_chains_configuration
https://wiki.koha-community.org/wiki/ICU_do_not_undiacritic
https://wiki.koha-community.org/wiki/Correcting_Search_of_Arabic_records

ICU still doesn't work with queryfuzzy afaik

I did not find the documentation from Zebra on the use of quotes, but I assume ICU requires the change - many of our partner sites were using non-ICU indexing with ICU style quotes, so at least one=-way it is not harmful
Comment 14 David Cook 2021-02-23 01:50:50 UTC
(In reply to Nick Clemens from comment #13)
> I did not find the documentation from Zebra on the use of quotes, but I
> assume ICU requires the change - many of our partner sites were using
> non-ICU indexing with ICU style quotes, so at least one=-way it is not
> harmful

Actually, now that I think about this again, I think that Koha's idea of "ICU quotes" is totally wrong.

Koha doesn't speak ICU or CHR to Zebra. Koha just sends PQF queries to Zebra (and it uses a CCL2RPN conversion to produce these PQF queries). The quotes are relevant only in terms of CCL and PQF queries. So... they're relevant regardless of indexing mode (which basically just governs normalization and tokenization).

Now... Bug 27299 shows and solves a tokenization issue with ICU and phrase registers, and that caused there to be a difference in how records were being indexed. 

I see that there is a TraceCompleteSubfields system preference which adds a qualifier of ",complete-subfield", which would cause the phrase register to be used as it's a 6=2 attribute (see https://github.com/indexdata/idzebra/blob/master/tab/default.idx for details on the register)

Technically speaking, we should always be wrapping the operands in double quotes so that queries are like this: au="Cook, David" and su="ICU".
Comment 15 David Cook 2021-02-23 01:59:36 UTC
Oh I see... with UseICU, we're using curly braces instead of double quotes. Yeah... that's not an ICU thing. 

You can see some discussion about this here:
http://lists.indexdata.dk/pipermail/zebralist/2007-July/001674.html
http://lists.indexdata.dk/pipermail/yazlist/2001-September/000090.html

I think it might be a case of poor string handling in Perl in the past. Going to dredge up some more detail..
Comment 16 David Cook 2021-02-23 02:05:27 UTC
I think that curly braces are just an undocumented synonym for double quotes. We could reach out to Indexdata for comment on that.
Comment 17 David Cook 2021-02-23 02:13:33 UTC
This is using ICU:

Z> find su,complete-subfield={Rock musicians United States Biography 1705}
Sent searchRequest.
Received SearchResponse.
Search was a success.
Number of hits: 2, setno 16
SearchResult-1: term=Rock musicians United States Biography 1705 cnt=2
records returned: 0
Elapsed: 0.000290

Z> find su,complete-subfield="Rock musicians United States Biography 1705"
Sent searchRequest.
Received SearchResponse.
Search was a success.
Number of hits: 2, setno 17
SearchResult-1: term=Rock musicians United States Biography 1705 cnt=2
records returned: 0
Elapsed: 0.000361

So UseICUStyleQuotes doesn't really make sense to me. It's probably just hiding a bug in Koha.
Comment 18 David Cook 2021-02-23 02:16:13 UTC
Note that the above used the fix from Bug 27299, but I just tried again without the tokenization fix, and both {} and "" works fine.
Comment 19 David Cook 2021-02-23 02:40:10 UTC
Looking at Bug 7092 which introduced UseICU... it looks like this was always a workaround. 

If someone has a test plan that shows ICU not working with "" or CHR not working with {}, I'd happily test it.