I am wondering whether there is a specific reason to use dashes in variable names. If there aren't any specific reasons, I suggest replace dashes with an underscore in variables mobile-tools-device-capabilities and mobile-tools-device-detection.

Additionally, it seems that the latest version of Strongarm cannot export these variables because of the naming issue however it's still unconfirmed.

CommentFileSizeAuthor
#6 variable_rename-1294836-7.patch11.54 KBminoroffense
#5 variable_rename-1294836-5.patch27.88 KBminoroffense
#3 variable_rename-1294836-3.patch27.62 KBmelon

Comments

minoroffense’s picture

minoroffense commented

You're right. Per the Drupal coding standard, they should all be underscores. No dashes. http://drupal.org/coding-standards#naming

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.

We'll have to write a patch to rename them along with an update hook to migrate the old values to their new home.

Feel up to the task?

melon’s picture

melon commented

Sure, I put the task of rolling a patch for this on my todo list. Will get back with it soon.

melon’s picture

melon commented
Status: Active » Needs review
StatusFileSize
new variable_rename-1294836-3.patch27.62 KB

I rolled the patch against the 7.x-2.x branch. Please test.
Please note that my editor auto-corrected many whitespace and no-end-of-line-marker issues hence the largish diff.

I was also wondering whether there are modules that rely on mobile_tools. They should change their variable gets as well.

minoroffense’s picture

minoroffense commented

If other modules are referencing the variables directly they'll have to update. But since this is a dev version, I'm not too concerned about forcing updates on other modules. The other maintainers should expect dev code to be in flux.

But it is a good point and worth including in the release notes.

minoroffense’s picture

minoroffense commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new variable_rename-1294836-5.patch27.88 KB

I added a few code comments and used the get_t() in the update hook just in case (not sure if this is still needed in D7 but what the hell, can't hurt).

Thanks, the patch looks good (didn't see anything break) and I'll apply it to the 7.x-2.x branch.

minoroffense’s picture

minoroffense commented
StatusFileSize
new variable_rename-1294836-7.patch11.54 KB

I trimmed out the whitespace fixes to make the patch easier to apply.

The trim was done in a separate commit.

melon’s picture

melon commented

Yes, I suppose dev versions shouldn't really care about these kind of changes, especially if it fixes something.
Thanks for taking your time and dealing with this issue.

minoroffense’s picture

minoroffense commented
Status: Reviewed & tested by the community » Closed (fixed)

Committed