The default collation in the majority of Drupal installations is utf8_general_ci
. This can result in inconsistent data in the database if varying cases are used for the same variable names. Since the default collation is for case insensitivity, the logic should be made case insensitive as well since this would be a cleaner (and probably easier) approach than having a single table with a different collation.
With a MySQL database, this code:
variable_set('test_name', 42);
variable_set('TEST_name', 0);
$var1 = variable_get('test_name', 1);
$var2 = variable_get('TEST_name', 1);
$var3 = variable_get('TEST_NAME', 1);
echo "var1 = {$var1}\n";
echo "var2 = {$var2}\n";
echo "var3 = {$var3}\n";
Produces this output:
var1 = 42
var2 = 0
var3 = 1
In the variable
table, the variable test_name
has a value of 0
which is inconsistent with what is in memory (42
).
Druapl version: 5.7
MySQL version: 5.0.41
PHP Version 5.2.5
Comment | File | Size | Author |
---|---|---|---|
#29 | get-set-vars2-D6.patch | 1.3 KB | rdrh555 |
#26 | get-set-vars-D6.patch | 1.28 KB | rdrh555 |
#21 | persistent_variables_lower_case.patch | 1.33 KB | mr.baileys |
#19 | persistent_variables_lower_case.patch | 1.46 KB | mr.baileys |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, this is really undocumented, but variable names should really be plain readable ASCII in lowercase. That's seems like the general consensus at least in Drupal Core.
The true answer is that the
variable.name
column should really be a BLOB.Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedThis won't be fixed for version 5 but maybe a patch for version 7 would be beneficial. I'll see about whipping something into shape.
Comment #3
David Strauss"The true answer is that the variable.name column should really be a BLOB."
BLOBs are for binary data, not for achieving case sensitivity.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commented@David Strauss: $name is not defined as a text, and does not specify encoding. As far as variable_get/variable_set are concerned, it can be any raw data. But ok, that's pretty far-fetched.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedOh. And the current schema API knows nothing about collations.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedThe _ci part of the table encoding is what causes the real issue. Since _ci is used the variable names within the static caches must also be Case Insensitive. And for the record variable.name is a variable character column with a maximum length of 128. The patch will simply be a strtolower conversion on the parameter $name in each function.
Comment #7
catchThere are many ways variables can be mis-typed, not just cases, so IMO this is a duplicate of #260226: Replace variable string literals with defined constants and maybe: #145164: DX: Use hook_variable_info to declare variables and defaults - better to deal with the general issue than just the case sensitivity.
Also, this seems like more a documentation issue to me - should put lowercase/ascii only in coding standards and if a module breaks from this it's a bug in the module.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedDefinitely not a duplicate of #260226: Replace variable string literals with defined constants; maybe for #145164: DX: Use hook_variable_info to declare variables and defaults but I need to study it more. However, we still need to make the variable names case insensitive throughout the set of variable_* functions. That is a bug because the database storage for the name column is case insensitive.
Comment #9
marcingy CreditAttribution: marcingy commentedBumping version
Comment #10
arhak CreditAttribution: arhak commented@#9 why should an existing bug be bumped to D8 while D7 is still in dev?
Comment #11
marcingy CreditAttribution: marcingy commentedI don't believe this is a bug to be honest more a feature request variables should use a consistent case which in drupal is lower case. Maybe at best this is a documentation issue in that this should be specified as a coding standard.
Comment #12
marcingy CreditAttribution: marcingy commentedreclassifying correctly
Comment #13
David StraussThis is not a feature request. It is a bug.
Comment #14
jhodgdonOK. This is currently in the documentation component and classified as a documentation bug.
What is it that we should be documenting, and where should it be documented? From reading the above thread, I'm not seeing any consensus really.
Comment #15
mr.baileys@jhodgdon: there is consensus on the fact that variables names should be all lower case (as is done throughout core) and that this should be documented in the Naming Conventions section of the Coding Standards.
I'm not sure what the correct term is for these kinds of variables (variable_get calls these persistent variables). To get the ball rolling, I suggest we change the first item under naming conventions from:
to:
There is no consensus on whether and how to protect developers using differently cased names for the same variable.
My 2 cents: using differently cased names for the same variable is sloppy coding (broken code?). Adhere to the guideline above (or at least be consistent when addressing the same variable) and there is no problem.
Comment #16
jhodgdonThese are Drupal variable_get/variable_set variables, used for settings in Drupal. I think we should have a separate entry for them, not lump them with functions and variables, which are PHP constructs.
And I guess you are right that they are called "persistent variables" in the variable_get/variable_set function docs:
http://api.drupal.org/api/function/variable_get/7
How about adding this in the naming conventions area:
Persistent variables
Persistent variables (variables/settings defined using Drupal's variable_get()/variable_set() functions) should be named using all lowercase letters, and words should be separated with an underscore. They should use the grouping/module name as a prefix, to avoid name collisions between modules.
I think we should also add a note about case insensitivity to the variable_set/get function doc. Something like this for variable_get:
@param $name The name of the variable to return. Case insensitive matching is used.
Thoughts?
Comment #17
Dries CreditAttribution: Dries commentedThe proposed help text looks good to me. Adding a note to the phpdoc of variable_set/get is recommended. If desired, we could add a @todo for Drupal 8, but I'm not 100% convinced this needs more than a convention.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat's not true. variable_set() and variable_del() are case insensitive on MySQL, case sensitive mostly everywhere else. variable_get() is always case sensitive.
Comment #19
mr.baileysAgreed on the text change for the coding guidelines proposed by jhodgdon in #16.
Patch for review attached to change the docs on variable_get/variable_set.
Comment #20
Dries CreditAttribution: Dries commentedI think the links are redundant. The comment proceeding the link explains it perfectly, not?
Comment #21
mr.baileysTrue. The coding guidelines do offer additional advice (like namespacing the variable name with your module's name), but nowhere else in core do we reference the coding standards, so we probably shouldn't do that here either.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedDo we need a coding standard entry about referencing or not the coding standards? :)
Comment #23
jhodgdon+1 on the patch in #21. Thanks whoever decided to get the first line of the doc up to doc standards. Always appreciated (at least by me). :)
Since no one seems to be objecting, I went ahead and added the text from #16 to http://drupal.org/coding-standards#naming also. If someone objects now, we can always edit it.
Comment #24
Dries CreditAttribution: Dries commentedGreat. Committed to CVS HEAD. Thanks.
Comment #25
jhodgdonI think maybe we should also make the doc change in the patch from #21 in Drupal 6.
Comment #26
rdrh555 CreditAttribution: rdrh555 commentedFrom #21
Comment #27
arhak CreditAttribution: arhak commented+1 @#26
Comment #28
jhodgdonPatch needs reroll - you need to roll patches from Drupal root.
Comment #29
rdrh555 CreditAttribution: rdrh555 commentedWrong Eclipse setting.....
Comment #30
jhodgdonLooks good, thanks!
Comment #31
Gábor HojtsyThanks, committed.