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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Title: remove case sensitivity from variable names (get/set/del) » Remove case sensitivity from variable names (get/set/del)

Ok, 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.

Anonymous’s picture

Version: 5.7 » 7.x-dev
Assigned: Unassigned »

This 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.

David Strauss’s picture

"The true answer is that the variable.name column should really be a BLOB."

BLOBs are for binary data, not for achieving case sensitivity.

Damien Tournoud’s picture

@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.

Damien Tournoud’s picture

Oh. And the current schema API knows nothing about collations.

Anonymous’s picture

The _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.

catch’s picture

There 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.

Anonymous’s picture

Definitely 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.

marcingy’s picture

Version: 7.x-dev » 8.x-dev

Bumping version

arhak’s picture

Version: 8.x-dev » 7.x-dev

@#9 why should an existing bug be bumped to D8 while D7 is still in dev?

marcingy’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » feature

I 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.

marcingy’s picture

Version: 8.x-dev » 7.x-dev
Component: base system » documentation
Category: feature » task

reclassifying correctly

David Strauss’s picture

Category: task » bug

This is not a feature request. It is a bug.

jhodgdon’s picture

OK. 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.

mr.baileys’s picture

Status: Active » Needs review

@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:

Functions and variables

Functions and variables should be named using lowercase, and words should be separated with an underscore. Functions should in addition have the grouping/module name as a prefix, to avoid name collisions between modules.

to:

Functions, variables and persistent variables

Functions and (persistent) variables should be named using lowercase, and words should be separated with an underscore. Functions and persistent variables (managed through variable_get and variable_set) should in addition have the grouping/module name as a prefix, to avoid name collisions between modules.

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.

jhodgdon’s picture

These 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?

Dries’s picture

The 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.

Damien Tournoud’s picture

@param $name The name of the variable to return. Case insensitive matching is used.

That's not true. variable_set() and variable_del() are case insensitive on MySQL, case sensitive mostly everywhere else. variable_get() is always case sensitive.

mr.baileys’s picture

Agreed 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.

Dries’s picture

+++ includes/bootstrap.inc	1 Jun 2010 05:27:22 -0000
@@ -760,7 +760,11 @@
+ * variable names. See http://drupal.org/coding-standards#naming

I think the links are redundant. The comment proceeding the link explains it perfectly, not?

mr.baileys’s picture

Assigned: » Unassigned
FileSize
1.33 KB

True. 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.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Do we need a coding standard entry about referencing or not the coding standards? :)

jhodgdon’s picture

+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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Great. Committed to CVS HEAD. Thanks.

jhodgdon’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

I think maybe we should also make the doc change in the patch from #21 in Drupal 6.

rdrh555’s picture

Assigned: Unassigned » rdrh555
Status: Patch (to be ported) » Needs review
FileSize
1.28 KB

From #21

arhak’s picture

+1 @#26

jhodgdon’s picture

Status: Needs review » Needs work

Patch needs reroll - you need to roll patches from Drupal root.

rdrh555’s picture

Status: Needs work » Needs review
FileSize
1.3 KB

Wrong Eclipse setting.....

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.