Problem

  • The upgrade path helper from variables to configuration does not do anything.
  • Module update functions do not explicitly define, which variables are migrated.
  • Variables that do not have a defined value are not migrated into config.
  • Migrating variables into config deletes the variables, which inherently imposes interdependencies between module update functions (since variables may have been migrated and removed in another update already).

Proposed solution

  • Explicitly declare the config object name and variables to migrate into it.
  • Require default configuration to exist for the variables being converted into a config object.
  • Install the default configuration of the config object name first. Introduce formal namespaces/owners for config objects to accomplish that.
  • Overload the config object values with possibly existing variable values, save it, and delete the migrated variables in the end.
  • Declare hook_update_dependencies() for known dependencies.
  • Declare and use a phpDoc group 'config_upgrade' to allow us to track, re-evaluate, and revise all update functions that operate on variables or configuration later on.

Notes

Files: 
CommentFileSizeAuthor
#31 config.upgrade.31.patch17.22 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,683 pass(es). View
#31 interdiff.txt997 bytessun
#19 config.upgrade.19.patch17.34 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,710 pass(es). View
#19 interdiff.txt2.32 KBsun
#18 config.upgrade.18.patch16.13 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,690 pass(es). View
#18 interdiff.txt1.23 KBsun
#17 config.upgrade.17.patch16.06 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,709 pass(es). View
#17 interdiff.txt1.15 KBsun
#14 config.upgrade.13.patch15.8 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,711 pass(es). View
#11 config.upgrade.11.patch16.01 KBsun
FAILED: [[SimpleTest]]: [MySQL] 35,308 pass(es), 1 fail(s), and 1 exception(s). View
#6 default_config_yaml.patch2.09 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 36,651 pass(es). View
#4 default_config.patch2.01 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 36,662 pass(es). View

Comments

marcingy’s picture

Initial thoughts on the config_install_default_config is something along these lines

function config_install_default_config($module, $config = '*') {
   $module_config_dir = drupal_get_path('module', $module) . '/config';
   $drupal_config_dir = config_get_config_directory();
   if (is_dir(drupal_get_path('module', $module) . '/config')) {
     $files = glob($module_config_dir . '/' . $config . '.xml');
     foreach ($files as $key => $file) {
       // Load config data into the active store and write it out to the
       // file system in the drupal config directory. Note the config name
alexpott’s picture

I think this makes sense... so are you suggesting that system_update_8010 should look something like this:

/**
 * Moves system settings from variable to config.
 */
function system_update_8010() {
  config_install_default_config('system', 'system.rss-publishing');
  update_variables_to_config('system.rss-publishing');
}
marcingy’s picture

Yes something like that.

marcingy’s picture

Status: Active » Needs review
FileSize
2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 36,662 pass(es). View

Initial roll adding in the missing updates and a means by which existing configs won't be overwritten.

marcingy’s picture

FileSize
2.09 KB
PASSED: [[SimpleTest]]: [MySQL] 36,651 pass(es). View

Re-rolled for yaml.

sun’s picture

Status: Needs review » Postponed (maintainer needs more info)

The updates 8009 and 8010 do not load configuration into the active store on update
...
They need to ensure that they load the config.

I have no idea what this means.

marcingy’s picture

Status: Postponed (maintainer needs more info) » Needs review

It means exactly what it says the updates in question do not call config_install_default_config which means that the systems config files would not have been available for those updates and hence the import of variables would not have happened. All updates that manipulate config need to load files into the active store.

As it stood no data for rss and cron was available.

sun’s picture

I'm pretty sure that my patch in #1496542: Convert site information to config system eliminates this issue. Am I correct?

marcingy’s picture

Status: Needs review » Closed (duplicate)

Yes changes in update_variables_to_config will solve this issue as that approach is namespacing the config that is loaded. Marking this as a dup.

sun’s picture

Title: system updates 8009 and 8010 do not load config into active store » Configuration upgrade path is broken
Assigned: Unassigned » sun
Status: Closed (duplicate) » Needs review
FileSize
16.01 KB
FAILED: [[SimpleTest]]: [MySQL] 35,308 pass(es), 1 fail(s), and 1 exception(s). View

Replaced the issue summary.

Extracted relevant changes from #1496542: Convert site information to config system

sun’s picture

Issue tags: +upgrade path, +D8 upgrade path
catch’s picture

Priority: Major » Critical
sun’s picture

Priority: Critical » Major
FileSize
15.8 KB
PASSED: [[SimpleTest]]: [MySQL] 36,711 pass(es). View
+++ b/core/modules/node/node.install
@@ -475,7 +475,6 @@ function node_uninstall() {
-  variable_del('default_nodes_main');

Stray left-over.

sun’s picture

Priority: Major » Critical
sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs review » Needs work
+++ b/core/includes/update.inc
@@ -877,64 +878,56 @@ function update_retrieve_dependencies() {
+  // Build the new configuration object.
+  // This potentially loads an existing configuration object, in case another
+  // update function migrated configuration values into $config_name already.
   $config = config($config_name);
...
+  // Load and set default configuration values.
+  // Throws a FileStorageReadException if there is no default configuration
+  // file, which is required to exist.
+  $file = new FileStorage($config_name);
+  $file->setPath(drupal_get_path('module', $module) . '/config');
+  $data = $file->read();
+
+  // Apply the default values.
+  $config->setData($data);

Apparently, I contradict myself. When being called repetitively to migrate (more) variables for the same config object name, then subsequent calls will reset the config object to the default configuration being defined in the default configuration file.

So far, this use-case does not exist in the module update functions, and at this point, I'm not sure whether the case will appear at all.

We'll have to decide whether to ignore that case for now, or whether to pro-actively prevent data rotten.

Upfront, though, DrupalConfig does not provide methods for handling the config data keys for a special case like this, so if we decide to account for it, then a proper solution will require a decent amount of code to handle the string-key to nested-array-key munging (in both directions).

sun’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
16.06 KB
PASSED: [[SimpleTest]]: [MySQL] 36,709 pass(es). View

oh. Luckily, we have a nice helper function for that already. Since this is in update.inc, we can use it. :)

sun’s picture

FileSize
1.23 KB
16.13 KB
PASSED: [[SimpleTest]]: [MySQL] 36,690 pass(es). View

And let's make that more explicit.

sun’s picture

FileSize
2.32 KB
17.34 KB
PASSED: [[SimpleTest]]: [MySQL] 36,710 pass(es). View

Added tests for repetitive migration into same config object.

sun’s picture

FWIW, this patch is ready to be committed.

cosmicdreams’s picture

Nice patch sun. Was going to comment about a update function but later found you moved what node_update_8002 is doing the system module.

I do have a follow up question though. When update functions like that are moved should we modify the function names so that they remain sequential?

If that's not an issue then I vote RTBC

sun’s picture

Update functions are normally not renamed.

(Although, admittedly, during the D7 cycle, there was a dedicated clean-up of the entire upgrade path, which not only renamed but also mixed and merged and rewrote plenty of update functions to speed up the upgrade performance. However, that was a special coordinated effort and shouldn't be done randomly IMO.)

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Good to know, reviewed, although not manually tested.

catch’s picture

Status: Reviewed & tested by the community » Needs review

OK just question really, looks really good to me though.

+++ b/core/includes/update.incundefined
@@ -877,64 +878,66 @@ function update_retrieve_dependencies() {
+  // Delete the migrated variables.

Do we really want to delete the variables here? During the release cycle I could see some variables being split into several more granular ones, with the current setting being used as the default for the new values. That could happen in contrib as well as between core modules.

Also we don't have explicit ownership of variables at the moment, and there's the potential for forked modules to have competing upgrade paths (think ubercart vs. commerce in D7).

I'm assuming we'll have an update in Drupal 9 that is just db_drop_table('variables'); which is going to have the same effect, so why not leave them there for now?

+++ b/core/lib/Drupal/Core/Config/FileStorage.phpundefined
@@ -105,13 +105,14 @@ class FileStorage {
+      // Use var_export to quote $this->name to separate from text.
+      throw new FileStorageReadException('Configuration file ' . var_export($this->name, TRUE) . ' does not exist.');

Don't really mind this, but why not just quote it?

+++ b/core/modules/system/system.installundefined
@@ -1864,16 +1888,30 @@ function system_update_8008() {
 function system_update_8009() {
-  update_variables_to_config('system.cron');
+  update_variables_to_config('system.cron', array(
+    'cron_max_threshold' => 'cron_max_threshold',
+    'cron_safe_threshold' => 'cron_safe_threshold',
+    'cron_threshold_warning' => 'cron_threshold_warning',
+    'cron_threshold_error' => 'cron_threshold_error',
+    'cron_key' => 'cron_key',

I much prefer the explicit mapping here, this is great.

sun’s picture

1) I don't think that retaining converted variables is a wise idea - exactly because of the reason that the already converted configuration might be changed further in a later update. Thus, the result would be duplicated and inconsistent values between variables and configuration, without having a clear master value. Due to that, I rather see modules using hook_update_dependencies() at this point.

But alas, that's exactly the reason for why I added the config_upgrade phpDoc group. I'm afraid we'll have to re-evaluate the entire upgrade path concerning variables and config conversions and make sure they run in the correct order at some point. :-/

2) @chx asked me the same, and I replied that var_export() does the quoting and also double-quoting if necessary.

--
Not sure what to do now... back to RTBC?

catch’s picture

hook_update_dependencies() is probably enough here.

With the quoting vs. double quoting, I'm not sure why that's needed? The only reason to add quotes around the variable is differentiate it from the rest of the error message, if the variable is "it's" then it doesn't really matter if it's mis-quoted - it's a sentence as opposed to PHP.

Dries’s picture

Status: Needs review » Needs work
+++ b/core/includes/update.incundefined
@@ -877,64 +878,66 @@ function update_retrieve_dependencies() {
+  // Load and set default configuration values.
+  // Throws a FileStorageReadException if there is no default configuration
+  // file, which is required to exist.
+  $file = new FileStorage($config_name);
+  $file->setPath(drupal_get_path('module', $module) . '/config');
+  $default_data = $file->read();
+
+  // Merge any possibly existing original data into default values.
+  // Only relevant when being called repetitively on the same config object.
+  if (!empty($original_data)) {
+    $data = drupal_array_merge_deep($default_data, $original_data);
+  }
+  else {
+    $data = $default_data;

The fact that we have to read and parse the default configuration file ourselves feels like a short-coming of the configuration management API. I'm happy to keep this as is but it did gave some pause. Seems like it would make sense to have an API to retrieve defaults? Maybe I misunderstood, in which case maybe this can be documented a bit better in the code.

+++ b/core/modules/system/system.installundefined
@@ -1864,16 +1888,30 @@ function system_update_8008() {
-  update_variables_to_config('system.cron');
+  update_variables_to_config('system.cron', array(
+    'cron_max_threshold' => 'cron_max_threshold',
+    'cron_safe_threshold' => 'cron_safe_threshold',
+    'cron_threshold_warning' => 'cron_threshold_warning',
+    'cron_threshold_error' => 'cron_threshold_error',
+    'cron_key' => 'cron_key',

Should we change 'system.cron.cron_max_threshold' to 'system.cron.max_threshold' now (i.e. drop 'cron_')? It seems like the module can be dropped from the variable names with the explicit naming and the extra namespacing.

sun’s picture

Status: Needs work » Needs review
  1. The configuration system (currently) has no business with default configuration. The default configuration for modules is installed by Drupal's installation functionality; it is "copied" from a module's /config folder into the site's configuration. The configuration system only cares for the configuration that exists in its known/defined stores.

    The fact that we're able to use the config system's file storage controller directly here is not a shortcoming, but instead the contrary, one of the accomplishments we've achieved through the config system re-architecture changes so far. There are many more to come, see #1560060: [meta] Configuration system roadmap and architecture clean-up for a complete picture.

    This particular code is special upgrade path behavior in itself. But since it's using public API methods of the config storage interface, I don't think the code/usage needs anymore explanation or documentation.

  2. This patch does not and will not rename any variables. We're going to re-open the original issues that introduced the new config object key names instead. However, that process is currently blocked on #1599554: Tutorial/guidelines for how to convert variables into configuration
sun’s picture

#19: config.upgrade.19.patch queued for re-testing.

sun’s picture

So the only remaining "blocker" is to change the var_export() into some fancy quoted string concatenation?

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
997 bytes
17.22 KB
PASSED: [[SimpleTest]]: [MySQL] 36,683 pass(es). View

Replaced var_export() with embedded string variables.

Tentatively moving back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright, I committed this to 8.x. We can debate the finer points of the CMI API and whether it should know about defaults, but at the end of the day this patch fixes a critical bug. :)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.