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.
Created attachment 3822 [details] [review] Patch This patch sets system preferences to be case insensitive.
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?
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?
Created attachment 6529 [details] [review] New rebased patch If you call twice C4::Context->preference with the same preference, but with different case, the sql query will be executed twice and global hash %sysprefs will have two different keys for the same system preference. It can causes inconsistencies if syspref is changed between the two calls. Even if it will happen very rarely, at least this patch doesn't introduce new problems (as far as we are sure the sql comparison is case insensitive, because i removed LOWER from the query in this new patch), and it prevents the query to be executed more than once.
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.
Created attachment 6558 [details] [review] Bug 6132: preference were case sensitive. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
QA: Looks good. Updating patch status.
patch pushed, please test (changed severity from ENH to MIN, as it's a bugfix, not an enhancement I think)
Created attachment 7009 [details] [review] Update sysprefs cache in set_preference From POD: C4::Context->set_preference( $variable, $value ); This updates a preference's value both in the systempreferences table and in the sysprefs cache. but the sysprefs cache isn't updated. This patch fix that, applying lowercase to 'variable'.
Created attachment 7010 [details] [review] unit test Test getting and setting a syspref value, using different case.
Created attachment 7019 [details] [review] Bug 6132: Update sysprefs cache in set_preference Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 7020 [details] [review] Bug 6132: Add unit test for system preferences Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Works well, 30 bonus points for the unit tests.
QA Comment: Looks good, including a test. patch_status++
This fix was included in the 3.6.x branch prior to 3.6.4.
Ok in master, 3.8.x and 3.6.x branches