Configuration upgrade path is broken

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');
}

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.

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

Replaced the issue summary.

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

+++ b/core/modules/node/node.install
@@ -475,7 +475,6 @@ function node_uninstall() {
-  variable_del('default_nodes_main');

Stray left-over.

Updated issue summary.

+++ 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).

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

And let's make that more explicit.

Added tests for repetitive migration into same config object.

FWIW, this patch is ready to be committed.

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

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.)

Good to know, reviewed, although not manually tested.

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.

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?

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.

+++ 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.

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

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

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

Replaced var_export() with embedded string variables.

Tentatively moving back to RTBC.

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. :)