Bug 6132 - System preferences are case sensitive
Summary: System preferences are case sensitive
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: 3.6
Hardware: All All
: P3 minor (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-08 07:49 UTC by Julian Maurice
Modified: 2013-12-05 19:53 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch (961 bytes, patch)
2011-04-08 08:03 UTC, Julian Maurice
Details | Diff | Splinter Review
New rebased patch (751 bytes, patch)
2011-12-02 13:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6132: preference were case sensitive. (803 bytes, patch)
2011-12-04 03:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Update sysprefs cache in set_preference (898 bytes, patch)
2012-01-02 09:55 UTC, Julian Maurice
Details | Diff | Splinter Review
unit test (989 bytes, patch)
2012-01-02 09:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6132: Update sysprefs cache in set_preference (948 bytes, patch)
2012-01-03 06:16 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6132: Add unit test for system preferences (1.01 KB, patch)
2012-01-03 06:18 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2011-04-08 07:49:40 UTC
Actually, system preferences are case sensitive. It can be a source of no-easy-to-detect problems and I think it would be better if system preferences were case insensitive.
Comment 1 Julian Maurice 2011-04-08 08:03:59 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2011-08-11 08:10:43 UTC
The idea of this patch sounded very good! Unfortunately, this patch does not really make a difference.
Just test this without your patch:
use C4::Context;
print C4::Context->preference('OPACNAV')."\n";

It will print the OpacNav pref too. Why?
Because MySQL uses the utf8_general_ci collation here and that collation is case insensitive. So the statement with LOWER(variable)=? is not needed.

So, more interesting is now: Where did you have problems with the case of prefs in Koha? Should we do something else there?
Comment 3 Chris Cormack 2011-08-11 08:27:09 UTC
Marcel, this bit does make a difference

+    $var = lc($var);
     if (exists $sysprefs{$var}) {
         return $sysprefs{$var};
     }

Hash keys are case sensitive (as are template variables), I agree the lower in the sql is not nessecary, Henri-Damien or Julian can you give us a test plan, or how to recreate the issue please?
Comment 4 Julian Maurice 2011-12-02 13:14:44 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2011-12-04 03:56:40 UTC
As we move to persistent code, as we are, this patch becomes more important. 
Because the variables will persist for a lot longer, so inconsistent results are more likely to occur.
Comment 6 Chris Cormack 2011-12-04 03:57:25 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2011-12-05 08:45:15 UTC
QA: Looks good. Updating patch status.
Comment 8 Paul Poulain 2011-12-06 10:42:07 UTC
patch pushed, please test

(changed severity from ENH to MIN, as it's a bugfix, not an enhancement I think)
Comment 9 Julian Maurice 2012-01-02 09:55:52 UTC Comment hidden (obsolete)
Comment 10 Julian Maurice 2012-01-02 09:56:59 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2012-01-03 06:16:04 UTC
Created attachment 7019 [details] [review]
Bug 6132: Update sysprefs cache in set_preference

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 12 Chris Cormack 2012-01-03 06:18:01 UTC
Created attachment 7020 [details] [review]
Bug 6132: Add unit test for system preferences

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 13 Chris Cormack 2012-01-03 06:18:53 UTC
Works well, 30 bonus points for the unit tests.
Comment 14 Marcel de Rooy 2012-01-13 15:08:13 UTC
QA Comment: Looks good, including a test. patch_status++
Comment 15 Jared Camins-Esakov 2012-05-23 12:53:22 UTC
This fix was included in the 3.6.x branch prior to 3.6.4.
Comment 16 Julian Maurice 2012-06-25 09:12:49 UTC
Ok in master, 3.8.x and 3.6.x branches