Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system

This code passed review by heyrocker in another issue, so I think it's ok to set rtbc (pending test results).

I think it's a great function, I've changed it a little so if there are no var values, the deletion doesn't happen

One issue with this is that in certain cases we are amending the name of the variable I think we need some way to rename the variable and if if only a one name is supplied then the it should be used in both the from to operation.

ah misread but we still have issue in that the code assumes that we are not renaming anything. I am thinking we really need tests for this function - so taging as such

I think for situations where the variables are getting renamed, then you just need to write a manual update function. This is a helper for the simple (and most common) use case.

However I agree it needs tests.

Clarifying issue status.

New version with the ability to map fields.

Little coding fix, submitting again to the testbot.

+++ b/core/includes/update.inc
@@ -850,6 +850,59 @@ function update_retrieve_dependencies() {
+ * Provide a generalised method to migrate variables from Drupal 7 to Drupal 8's
+ * configuration management system.

Drupal's coding standards require the first line of docblock to start with an active verb, be a single line and have no more than 80 characters in total. The current text could be kept as an explanation with a blank line in between.

Thanks Alex this is looking good. Two thoughts while reviewing this patch:

1) Both @param directives in docblock could use type hinting.
2) The intent in time is to have the variables table go away. Won't this function then fail and as a consequence cause a site upgrade to fail? This function needs to remain around for quite sometime to help straggling sites upgrade to D8 or even D9.

To prevent such a failure possibility, I think we should check if the table exists before starting the db_select(). We might also want to add a test to make sure that this function runs correctly after dropping the variables table. With that, I think this can remain forever to help D7 upgrades to new content management subsystem.

Drupal 8 will always have the variable table, we need to keep it around so that contrib modules have an upgrade path. It won't go away until Drupal 9. So the extra functionality introduced in #19 won't be necessary. If we can revert that but keep the new comments then I think we are good to go here.

Ah thanks for the update @heyrocker. I thought the intent was for variables table to go away for D8.

@alexpott - My understanding is that @param type hinting is being introduced with D8 as a nice to have, but not yet required. Your expanded comments there look great.

Patch removing the extra test that was added above.

Fixing tag

Ship it. Thanks everyone.

+++ b/core/modules/config/config/config.test.xmlundefined
+<?xml version="1.0"?>
+<config>
+  <config_test_foo>bar</config_test_foo>
+  <config_test_bar>foo</config_test_bar>

catch pointed out that this really shouldn't live in the config module's main config directory, It's going to get installed with all the real config and clutter up things. A better option would be to make a module just for testing in core/modules/simpletests. There are several there already.

+++ b/core/modules/config/config.testundefined
@@ -296,3 +296,40 @@ class ConfOverrideTestCase extends DrupalWebTestCase {
+/**
+ * Tests configuration overriding from settings.php.
+ */

Also while talking to catch in IRC, I noticed incorrect comment this which I assume is a copy/paste error from another test.

Powered by Dreditor.

Awesome, going back to RTBC. Thanks for your patience on this.

I think we might still be lacking something in that the new configuration files are not loaded in the active store for modules already enabled (this would brought up in chat with Zlender today) a call to config_install_default_config only exists when performing the initial install and when modules are being enabled which will not be the case when an update happens.

I am thinking in update_variables_to_config we need to check if $config = config($config_name); returns a valid config object and if it doesn't we then config_install_default_config and again check if config can be retrieved. I may of course be missing something.

For modules already enabled, they will have to manage this themselves in their upgrade path. I don't want to add a call to config_install_default_config() in this function, as it feels like we would be making too many assumptions and doing too much in this function. I think it should be up to the modules themselves to deal with that part as they wish, and then just call this function for the conversion.

Ok so in effect all core modules are going to have to load config as part of the upgrade process which seems like a lot of duplication one way round that could be

function update_variables_to_config($config_name, $variable_map = array(), $module = NULL) {
  if ($module ) {
    config_install_default_config($module);
  }

That way the caller has control over the upgrade process.

So for instance for core system module config update hook would look something like

function system_update_800x() {
  update_variables_to_config('system.foo', array(), 'system');
  update_variables_to_config('system.bar');
  update_variables_to_config('system.baz');
}

or if we leave it up to modules

function system_update_800x() {
  config_install_default_config('system')
  update_variables_to_config('system.foo');
  update_variables_to_config('system.bar');
  update_variables_to_config('system.baz');
}

I prefer the 2nd option though

Seeing the 2 options laid out above I agree the 2nd option looks cleaner, so no issues with what we have here at the moment from me as that was the only concern which later dawn on me.

Looks good now. Committed/pushed to 8.x.

This will need a change notice explaining how to use the new function.

And this will need a followup afterwards to remove those trailing whitespaces...

Still no type hinting ...

With type hinting for the array.

RTBC for the doc changes and the change notice is probbaly OK to.

Change notice looks fine, and I've committed the follow-up. Marking fixed, thanks all!