Add a variable_rename() helper function for developers

Hi Dave, I think this is a really great idea. I am willing to write this piece of code however I'm wondering how this works as there is currently no 8.x-dev branch as far as I can see. Does this need to happen before we can start writing in new features?

Heres my attempt at variable_rename. I created a diff against HEAD of d7 so not sure how this gets rolled later on.

Cheers,
Tim

You're correct it does throw a warning but I think it's also good that it doesn't set the variable in $conf if it doesn't exist already. It also means the query doesn't needlessly need to run.

The reason it was variable_update in the comments was because thats what I originally called the function before I realised that it was the wrong name and changed it, I just forgot to update the comments as well.

I have now re-rolled patch with recommended changes.

Cheers,
Tim

Please put this in common.inc rather than bootstrap.inc

Re-rolled per catch's direction. I assume this is so the function doesn't get loaded on bootstrap as it will only be needed at a later stage of loading?

Looks great Dave.

This will break in the case a variable is set in settings.php but not in the database. It should probably do select, delete, insert instead of update. If a variable is renamed but was set in settings.php I'm not sure what the behaviour should be, maybe check conf as well as the return from the direct query. To be honest I'm not convinced of the usefulness of this, how often does variable renaming happen? Additionally the convention is now not to use api functions in update hooks, so adding an api function that will rarely be used outside of those goes against this.

Those two functions do not try to update a row that doesn't exist.

$foo = variable_get('foo'); variable_set('bar', $foo); works though, the equivalent situation is broken in this patch.

Also in your example deleting the variable won't remove it from $conf, but it does successfully delete it from the database, so the only cruft is a hopefully unused variable in settings.php. If the variable isn't in the db variable_del() doesn't care whether it's in conf or not in practice.

OK I'm back on computer now, shouldn't review patches from iphone....

The issue is that the current pattern for renaming a variable works:

$foo = variable_get('foo');
if (isset($foo)) {
  variable_set('bar', $foo);
}
variable_del('foo');

That example works for both variables that are set in $conf, and variables that aren't.

The current patch would fail to set the old value to the new variable name, if the variable was only set in $conf.

Update hooks in D7 core should not be using API functions, arguably this includes variable_get()/variable_set()/variable_del() - so if they are we need to consider adding _update_variable_get_7000() equivalents to be consistent with the examples elsewhere.

Catch, when I created the function I used the exact pattern above as my guide and expanded the functions out and then re-factored it, so I believe that any problem that there is with the new function also exists with the pattern listed above.

I question whether you would want this function to rename a variable that is only set in $conf as that variable would be persistent even after attempting the rename. The only thing this function or pattern would do is unset the variable from $conf until the page was reloaded again.

As far as using API functions in update hooks that's not an argument that I'm going to weight into except only to say that I see variable_get()/variable_set()/variable_del() in update hooks all the time.

.

I think we can close this. An 8.x solution would be completely different and I doubt this would be added to 7.x.