Single config import form needs to use the config validation events

Looking

Not looking.

At a quick glance, I noticed we have to inject all the dependencies of ConfigImporter into ConfigSimpleImportForm.

+++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
@@ -241,28 +325,114 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    $config_importer = new ConfigImporter(
+      $storage_comparer,
+      $this->eventDispatcher,
+      $this->configManager,
+      $this->lock,
+      $this->typedConfigManager,
+      $this->moduleHandler,
+      $this->moduleInstaller,
+      $this->themeHandler,
+      $this->getStringTranslation()
+    );

Possibly a good candidate for a ConfigImporterFactory so we can hide this?

Discussed with alexpott. This patch will make this form much safer to use, and is not disruptive to existing sites, so tagging as an rc target.

1. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -35,6 +46,56 @@ class ConfigSingleImportForm extends ConfirmFormBase {
   +  protected $moduleInstaller;
   +
   +
   +  /**
   

   extra newline

2. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -67,7 +135,14 @@ public function __construct(EntityManagerInterface $entity_manager, StorageInter
   -      $container->get('config.storage')
   +      $container->get('config.storage'),
   +      $container->get('lock.persistent'),
   +      $container->get('event_dispatcher'),
   +      $container->get('config.manager'),
   +      $container->get('config.typed'),
   +      $container->get('module_handler'),
   +      $container->get('module_installer'),
   +      $container->get('theme_handler')
   
   @@ -222,10 +299,52 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +        $config_importer = new ConfigImporter(
   +          $storage_comparer,
   +          $this->eventDispatcher,
   +          $this->configManager,
   +          $this->lock,
   +          $this->typedConfigManager,
   +          $this->moduleHandler,
   +          $this->moduleInstaller,
   +          $this->themeHandler,
   +          $this->getStringTranslation()
   +        );
   

   O_O

   I know I've thought this before, may have asked it, but why isn't this a service or something? Why must my class have all 7 of these injected?

3. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -222,10 +299,52 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +        } catch (ConfigImporterException $e) {
   

   newline please

4. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -222,10 +299,52 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +          $form_state->setErrorByName('import', drupal_render($item_list));
   

   I'm not up to date on such things, but dow we really throw drupal_render calls in like this now?

My eyes glossed over at the StorageOverrideWrapper stuff, I'll have to come back.

This looks good to me. Mostly just nitpicks.

1. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -51,14 +119,42 @@ class ConfigSingleImportForm extends ConfirmFormBase {
   +   *   The module handler
   

   Missing a period.

2. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -51,14 +119,42 @@ class ConfigSingleImportForm extends ConfirmFormBase {
   +   *   The theme handler
   

   Missing a period.

3. +++ b/core/modules/config/src/StorageOverrideWrapper.php
   @@ -0,0 +1,187 @@
   +  /**
   +   *
   +   * @var array
   +   */
   

   Missing short description.

4. +++ b/core/modules/config/src/StorageOverrideWrapper.php
   @@ -0,0 +1,187 @@
   +  public function override($name, array $data) {
   +    $this->overrideData[$this->collection][$name] = $data;
   +    return $this;
   +  }
   

   Missing docblock.

5. +++ b/core/modules/config/src/StorageOverrideWrapper.php
   @@ -0,0 +1,187 @@
   +    foreach ($names as $name) {
   +      if (isset($this->overrideData[$this->collection][$name])) {
   +        $data[$name] = $this->overrideData[$this->collection][$name];
   +      }
   +    }
   

   I'm not sure if this would be the place, or if this would be too much of a rabbit hole, but should we try to throw an error somewhere if the YAML format is incorrect? As it is now, a missing space before the value ("key:value"), or a key without a value ("key"), results in an uncaught PHP exception.

Looks good to me

1. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -241,28 +390,89 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +          'title' => t('Importing configuration'),
   +          'init_message' => t('Starting configuration import.'),
   +          'progress_message' => t('Completed @current step of @total.'),
   +          'error_message' => t('Configuration import has encountered an error.'),
   

   $this->t()

2. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -241,28 +390,89 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +          $batch['operations'][] = array(array(get_class($this), 'processBatch'), array($config_importer, $sync_step));
   

   $batch['operations'][] = [[static::class, 'processBatch'], [$config_importer, $sync_step];

3. +++ b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
   @@ -66,7 +66,7 @@ public function testImport() {
   -    $this->assertRaw(t('The @entity_type %label was imported.', array('@entity_type' => 'config_test', '%label' => $entity->label())));
   +    $this->assertRaw(t('The configuration was imported successfully.'));
   

   :\ oh well

We're in the final home stretch of preparing 8.0.0, and we won't be committing the majority of "rc target" issues any more, but still leaving the tag on them to triage them for patch-release or minor-release post 8.0.0 release. Meanwhile, switching to "8.0.0 target" for issues we'd still love to get in before 8.0.0.

This patch looks great. Here are some docs nits, but doesn't need to block commit. Leaving at RTBC, and I'll commit later today with or without these addressed, if another committer doesn't beat me to it.

1. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -35,6 +47,62 @@ class ConfigSingleImportForm extends ConfirmFormBase {
   +   * Event dispatcher.
   

   s/Event/The event/

2. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -51,14 +119,42 @@ class ConfigSingleImportForm extends ConfirmFormBase {
   +   */
   +  /**
   

   Remove.

3. +++ b/core/modules/config/src/StorageOverrideWrapper.php
   @@ -0,0 +1,204 @@
   + * Wraps a configuration storage and allows overrides of configuration data.
   

   It's a bit unclear in this comment that "overrides" in this class means the configuration is fully overridden, rather than the partial overrides we support in ConfigFactoryOverrideInterface.

4. +++ b/core/modules/config/src/StorageOverrideWrapper.php
   @@ -0,0 +1,204 @@
   +   * The configuration storage to be cached.
   

   Copy/paste error from some other code that does caching?

5. +++ b/core/modules/config/src/StorageOverrideWrapper.php
   @@ -0,0 +1,204 @@
   +   * Constructs a new CachedStorage.
   

   And here.

+++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
@@ -241,28 +390,89 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+  /**
+   * Finish batch.
+   *
+   * This function is a static function to avoid serializing the ConfigSync
+   * object unnecessarily.
+   */
+  public static function finishBatch($success, $results, $operations) {
+    if ($success) {
+      if (!empty($results['errors'])) {
+        foreach ($results['errors'] as $error) {
+          drupal_set_message($error, 'error');
+          \Drupal::logger('config_sync')->error($error);
+        }
+        drupal_set_message(\Drupal::translation()->translate('The configuration was imported with errors.'), 'warning');
+      }
+      else {
+        drupal_set_message(\Drupal::translation()->translate('The configuration was imported successfully.'));
       }
     }
+    else {
+      // An error occurred.
+      // $operations contains the operations that remained unprocessed.
+      $error_operation = reset($operations);
+      $message = \Drupal::translation()->translate('An error occurred while processing %error_operation with arguments: @arguments', ['%error_operation' => $error_operation[0], '@arguments' => print_r($error_operation[1], TRUE)]);
+      drupal_set_message($message, 'error');
+    }
   }

Rather than duplicating ConfigSync::finishBatch(), should we call it instead?

Ticking credit boxes for @mtift and @tim.plunkett for reviews.

Pushed to 8.0.x.