A common task in upgrading from Drupal 7 to 8 will be to move variables into the configuration management system. The patch creates a generalised function to do this by readings the keys from the config xml file. It assumes that the new config keys have the same name as the old variables.

Files: 
CommentFileSizeAuthor
#39 1497132-configuration_management_upgrade-39.patch2.53 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 35,920 pass(es). View
#37 1497132-configuration_management_upgrade-37.patch2.36 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 35,917 pass(es). View
#28 1497132-configuration_management_upgrade-28.patch5.81 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 35,787 pass(es). View
#24 1497132-configuration_management_upgrade-24.patch4.89 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 35,713 pass(es). View
#22 1497132-configuration_management_upgrade-22.patch4.15 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 35,730 pass(es), 2 fail(s), and 0 exception(s). View
#19 1497132-configuration_management_upgrade-19.patch5.69 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 35,734 pass(es). View
#17 1497132-configuration_management_upgrade-17.patch4.32 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 35,713 pass(es). View
#15 1497132-configuration_management_upgrade-15.patch4.11 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 35,700 pass(es). View
#13 1497132-configuration_management_upgrade-12.patch4.13 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1497132-configuration_management_upgrade-12.patch. Unable to apply patch. See the log in the details link for more information. View
#11 1497132-configuration_management_upgrade-11.patch3.77 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 35,703 pass(es), 2 fail(s), and 0 exception(s). View
#10 1497132-configuration_management_upgrade-10.patch3.71 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 35,689 pass(es), 2 fail(s), and 0 exception(s). View
#9 1497132-configuration_management_upgrade-9.patch2.06 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 35,678 pass(es). View
#7 1497132-configuration_management_upgrade-6.patch2.06 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/update.inc. View
#2 1497132-configuration_management_upgrade-3.patch1.55 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 35,701 pass(es). View
configuration_management_upgrade_7_to_8.patch1.5 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 35,698 pass(es). View

Comments

acrollet’s picture

Status: Needs review » Reviewed & tested by the community

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

pcambra’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 35,701 pass(es). View

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

marcingy’s picture

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.

marcingy’s picture

Issue tags: +Needs tests

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

heyrocker’s picture

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.

acrollet’s picture

Assigned: alexpott » Unassigned
Status: Needs review » Needs work

Clarifying issue status.

marcingy’s picture

Assigned: Unassigned » alexpott
Status: Needs work » Needs review
FileSize
2.06 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/update.inc. View

New version with the ability to map fields.

Status: Needs review » Needs work

The last submitted patch, 1497132-configuration_management_upgrade-6.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
PASSED: [[SimpleTest]]: [MySQL] 35,678 pass(es). View

Little coding fix, submitting again to the testbot.

alexpott’s picture

FileSize
3.71 KB
FAILED: [[SimpleTest]]: [MySQL] 35,689 pass(es), 2 fail(s), and 0 exception(s). View

Updated patch with tests and better documentation.

alexpott’s picture

FileSize
3.77 KB
FAILED: [[SimpleTest]]: [MySQL] 35,703 pass(es), 2 fail(s), and 0 exception(s). View

Improved documentation.

Status: Needs review » Needs work

The last submitted patch, 1497132-configuration_management_upgrade-11.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1497132-configuration_management_upgrade-12.patch. Unable to apply patch. See the log in the details link for more information. View

Add in missing config xml file

Status: Needs review » Needs work

The last submitted patch, 1497132-configuration_management_upgrade-12.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.11 KB
PASSED: [[SimpleTest]]: [MySQL] 35,700 pass(es). View

Fixed patch

Lars Toomre’s picture

Status: Needs review » Needs work
+++ 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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.32 KB
PASSED: [[SimpleTest]]: [MySQL] 35,713 pass(es). View

Fix function documentation as outlined in #16 and remove variable_set from test so that when this function is removed from drupal 8 the test will continue to function.

Lars Toomre’s picture

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.

alexpott’s picture

FileSize
5.69 KB
PASSED: [[SimpleTest]]: [MySQL] 35,734 pass(es). View

1) Not sure that type hinting is in the standards - see http://drupal.org/node/1354 but I've improved the comments anyway.
2) Good point - implemented so that the test drops the table if it exists and always creates it. This way once Drupal 8 no longer has the variable table this test will continue to work... don't think extra tests needed.

heyrocker’s picture

Status: Needs review » Needs work

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.

Lars Toomre’s picture

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.

marcingy’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.15 KB
FAILED: [[SimpleTest]]: [MySQL] 35,730 pass(es), 2 fail(s), and 0 exception(s). View

Patch removing the extra test that was added above.

beejeebus’s picture

Status: Needs review » Needs work
+      // Delete the old variables. The config system will throw an exception if a
+      // value cannot be saved, so this code will not run if there is a problem
+      // running the update.
+      $del = db_delete('variable')->condition('name', $variables, 'IN');
+      $del->execute();

this doesn't look right to me. we should catch this exception, and log it so the site admin knows what blew up.

other than that though, this looks ready to go.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.89 KB
PASSED: [[SimpleTest]]: [MySQL] 35,713 pass(es). View

Added try around config set and variable delete to catch and log errors to watchdog.

heyrocker’s picture

Issue tags: +Configuration system

Fixing tag

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

Ship it. Thanks everyone.

heyrocker’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.81 KB
PASSED: [[SimpleTest]]: [MySQL] 35,787 pass(es). View

Here's a new patch that fixes the cut & paste comment error and moves the config.xml into simpletest/tests/config_upgrade. I created the config_upgrade directory so the test config is not installed with any other test module in the simpletest/tests directory.

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

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

marcingy’s picture

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.

heyrocker’s picture

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.

marcingy’s picture

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.

Rok Žlender’s picture

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

marcingy’s picture

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.

catch’s picture

Title: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system » Change notice for:Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

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

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

aspilicious’s picture

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

alexpott’s picture

Status: Active » Needs review
FileSize
2.36 KB
PASSED: [[SimpleTest]]: [MySQL] 35,917 pass(es). View

Added change notice: http://drupal.org/node/1511542

Patch attached fixes whitespace issues and improves code code comments as http://api.drupal.org/api/drupal/core%21includes%21update.inc/function/u... doesn't look so good.

Lars Toomre’s picture

Still no type hinting ...

pcambra’s picture

FileSize
2.53 KB
PASSED: [[SimpleTest]]: [MySQL] 35,920 pass(es). View

With type hinting for the array.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Title: Change notice for:Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system » Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system
Status: Reviewed & tested by the community » Fixed

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

Tor Arne Thune’s picture

Priority: Critical » Normal

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