We're adding validation to the config import but the single import form is not validated at all. For example we should validate dependencies before importing - #2416109: Validate configuration dependencies before importing configuration

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Assigned: Unassigned » kim.pepper

Looking

kim.pepper’s picture

Assigned: kim.pepper » Unassigned

Not looking.

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
17 KB

Let's just use the ConfigImporter proper - then we have no need to do anything special - it just runs the validation AND we get to enable modules by editing core.extension - how cool is that?!?

kim.pepper’s picture

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?

alexpott’s picture

I think that if we address #4 it should be done in a followup. I've tried quite hard to keep the ConfigImporter object out of the container because it is a very ugly god object.

xjm’s picture

Issue tags: +rc target

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.

cilefen’s picture

Title: Single config import form needs to using the config validation events » Single config import form needs to use the config validation events
xjm’s picture

Issue tags: +Triaged core major
alexpott’s picture

Rerolled.

alexpott’s picture

Added tests. Test only patch is the interdiff.

The last submitted patch, 10: 2486467.test-only.10.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2486467-10.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
898 bytes
2.02 KB
21.02 KB

Fixing the patch. alexpott--

The last submitted patch, 13: 2486467.test-only.13.patch, failed testing.

alexpott’s picture

Let's try getting the test only patch right. Re-uploading the full patch too so that the latest comment has the correct patch.

The last submitted patch, 15: 2486467.13.test-only.patch, failed testing.

alexpott’s picture

Okay 4th time lucky... alexpott-- alexpott-- my .htaccess snuck into the test only patch.

The last submitted patch, 17: 2486467.17.test-only.patch, failed testing.

tim.plunkett’s picture

  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.

alexpott’s picture

Thanks @tim.plunkett

  1. Fixed
  2. Well the ConfigImporter is a bit of a god object cause it can do all sorts of things to the general state of the site. The difficulty with making it a service at this point is that the first argument to the constructor is not a service.
  3. Fixed
  4. Fixed - injected yet another service - the renderer.
mtift’s picture

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.

mtift’s picture

Status: Needs review » Needs work
alexpott’s picture

Status: Needs work » Needs review
FileSize
3.07 KB
23 KB
  1. Fixed
  2. Fixed
  3. Fixed
  4. Fixed
  5. I think this is an existing issue. Yep it is. We definitely should validate that the input is valid YAML but not in this issue.
alexpott’s picture

mtift’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

The last submitted patch, 10: 2486467.test-only.10.patch, failed testing.

The last submitted patch, 10: 2486467-10.patch, failed testing.

The last submitted patch, 13: 2486467.test-only.13.patch, failed testing.

tim.plunkett’s picture

  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

The last submitted patch, 15: 2486467.13.test-only.patch, failed testing.

alexpott’s picture

Thanks @tim.plunkett.

  1. Fixed
  2. Fixed
  3. Yeah... I think this is okay. Admittedly, not ideal.

I've replace the rest of the old style array syntax added by the patch...

The last submitted patch, 17: 2486467.17.test-only.patch, failed testing.

effulgentsia’s picture

Issue tags: -rc target +8.0.0 target

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.

effulgentsia’s picture

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.

effulgentsia’s picture

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

alexpott’s picture

@effulgentsia thanks for the reviews.
Re #34

  1. Fixed
  2. Fixed
  3. Fixed - renamed StorageReplaceDataWrapper and renamed the method too so that it is very separate from overrides.
  4. Fixed - yes it was a c&p error
  5. Fixed

Re #35
Good idea and we can do the same for processBatch too!

effulgentsia’s picture

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

effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.0.x.

  • effulgentsia committed fb101ee on 8.0.x
    Issue #2486467 by alexpott, mtift, tim.plunkett: Single config import...

  • effulgentsia committed fb101ee on 8.1.x
    Issue #2486467 by alexpott, mtift, tim.plunkett: Single config import...

Status: Fixed » Closed (fixed)

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