Issue #2711645 by alexpott, dawehner: ConfigInstaller::isSyncing() should...

Issue #2711645 by alexpott, dawehner: ConfigInstaller::isSyncing() should return true always during a config sync

core/lib/Drupal/Core/Config/ConfigImporter.php

@@ -484,14 +484,17 @@ public function import() {
    */
   public function doSyncStep($sync_step, &$context) {
     if (!is_array($sync_step) && method_exists($this, $sync_step)) {
+      \Drupal::service('config.installer')->setSyncing(TRUE);
       $this->$sync_step($context);
     }
     elseif (is_callable($sync_step)) {
+      \Drupal::service('config.installer')->setSyncing(TRUE);
       call_user_func_array($sync_step, array(&$context, $this));
     }
     else {
       throw new \InvalidArgumentException('Invalid configuration synchronization step');
     }
+    \Drupal::service('config.installer')->setSyncing(FALSE);
   }
 
   /**
@@ -778,7 +781,6 @@ protected function processExtension($type, $op, $name) {
     // Set the config installer to use the sync directory instead of the
     // extensions own default config directories.
     \Drupal::service('config.installer')
-      ->setSyncing(TRUE)
       ->setSourceStorage($this->storageComparer->getSourceStorage());
     if ($type == 'module') {
       $this->moduleInstaller->$op(array($name), FALSE);
@@ -805,8 +807,6 @@ protected function processExtension($type, $op, $name) {
     }
 
     $this->setProcessedExtension($type, $op, $name);
-    \Drupal::service('config.installer')
-      ->setSyncing(FALSE);
   }
 
   /**

core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php

@@ -382,8 +382,7 @@ protected function getQueryServiceName() {
    * {@inheritdoc}
    */
   public function importCreate($name, Config $new_config, Config $old_config) {
-    $entity = $this->createFromStorageRecord($new_config->get());
-    $entity->setSyncing(TRUE);
+    $entity = $this->_doCreateFromStorageRecord($new_config->get(), TRUE);
     $entity->save();
     return TRUE;
   }
@@ -425,6 +424,27 @@ public function importRename($old_name, Config $new_config, Config $old_config)
    * {@inheritdoc}
    */
   public function createFromStorageRecord(array $values) {
+    return $this->_doCreateFromStorageRecord($values);
+  }
+
+  /**
+   * Helps create a configuration entity from storage values.
+   *
+   * Allows the configuration entity storage to massage storage values before
+   * creating an entity.
+   *
+   * @param array $values
+   *   The array of values from the configuration storage.
+   * @param bool $is_syncing
+   *   Is the configuration entity being created as part of a config sync.
+   *
+   * @return ConfigEntityInterface
+   *   The configuration entity.
+   *
+   * @see \Drupal\Core\Config\Entity\ConfigEntityStorageInterface::createFromStorageRecord()
+   * @see \Drupal\Core\Config\Entity\ImportableEntityStorageInterface::importCreate()
+   */
+  protected function _doCreateFromStorageRecord(array $values, $is_syncing = FALSE) {
     // Assign a new UUID if there is none yet.
     if ($this->uuidKey && $this->uuidService && !isset($values[$this->uuidKey])) {
       $values[$this->uuidKey] = $this->uuidService->generate();
@@ -432,6 +452,7 @@ public function createFromStorageRecord(array $values) {
     $data = $this->mapFromStorageRecords(array($values));
     $entity = current($data);
     $entity->original = clone $entity;
+    $entity->setSyncing($is_syncing);
     $entity->enforceIsNew();
     $entity->postCreate($this);
 
@@ -439,6 +460,7 @@ public function createFromStorageRecord(array $values) {
     // entity object, for instance to fill-in default values.
     $this->invokeHook('create', $entity);
     return $entity;
+
   }
 
   /**

core/modules/config/tests/config_test/config_test.hooks.inc

@@ -9,6 +9,8 @@
  * config_test entity hooks themselves.
  */
 
+use Drupal\config_test\Entity\ConfigTest;
+
 /**
  * Implements hook_config_test_load().
  */
@@ -16,37 +18,69 @@ function config_test_config_test_load() {
   $GLOBALS['hook_config_test']['load'] = __FUNCTION__;
 }
 
+/**
+ * Implements hook_ENTITY_TYPE_create() for 'config_test'.
+ */
+function config_test_config_test_create(ConfigTest $config_test) {
+  if (\Drupal::state()->get('config_test.prepopulate')) {
+    $config_test->set('foo', 'baz');
+  }
+  _config_test_update_is_syncing_store('create', $config_test);
+}
+
 /**
  * Implements hook_config_test_presave().
  */
-function config_test_config_test_presave() {
+function config_test_config_test_presave(ConfigTest $config_test) {
   $GLOBALS['hook_config_test']['presave'] = __FUNCTION__;
+  _config_test_update_is_syncing_store('presave', $config_test);
 }
 
 /**
  * Implements hook_config_test_insert().
  */
-function config_test_config_test_insert() {
+function config_test_config_test_insert(ConfigTest $config_test) {
   $GLOBALS['hook_config_test']['insert'] = __FUNCTION__;
+  _config_test_update_is_syncing_store('insert', $config_test);
 }
 
 /**
  * Implements hook_config_test_update().
  */
-function config_test_config_test_update() {
+function config_test_config_test_update(ConfigTest $config_test) {
   $GLOBALS['hook_config_test']['update'] = __FUNCTION__;
+  _config_test_update_is_syncing_store('update', $config_test);
 }
 
 /**
  * Implements hook_config_test_predelete().
  */
-function config_test_config_test_predelete() {
+function config_test_config_test_predelete(ConfigTest $config_test) {
   $GLOBALS['hook_config_test']['predelete'] = __FUNCTION__;
+  _config_test_update_is_syncing_store('predelete', $config_test);
 }
 
 /**
  * Implements hook_config_test_delete().
  */
-function config_test_config_test_delete() {
+function config_test_config_test_delete(ConfigTest $config_test) {
   $GLOBALS['hook_config_test']['delete'] = __FUNCTION__;
+  _config_test_update_is_syncing_store('delete', $config_test);
+}
+
+/**
+ * Helper function for testing hooks during configuration sync.
+ *
+ * @param string $hook
+ *   The fired hook.
+ * @param \Drupal\config_test\Entity\ConfigTest $config_test
+ *   The ConfigTest entity.
+ */
+function _config_test_update_is_syncing_store($hook, ConfigTest $config_test) {
+  $current_value = \Drupal::state()->get('config_test.store_isSyncing', FALSE);
+  if ($current_value !== FALSE) {
+    $current_value['global_state::' . $hook] = \Drupal::isConfigSyncing();
+    $current_value['entity_state::' . $hook] = $config_test->isSyncing();
+    \Drupal::state()->set('config_test.store_isSyncing', $current_value);
+  }
 }

core/modules/config/tests/config_test/config_test.module

@@ -5,8 +5,6 @@
  * Provides Config module hook implementations for testing purposes.
  */
 
-use Drupal\config_test\Entity\ConfigTest;
-
 require_once dirname(__FILE__) . '/config_test.hooks.inc';
 
 /**
@@ -17,15 +15,6 @@ function config_test_cache_flush() {
   $GLOBALS['hook_cache_flush'] = __FUNCTION__;
 }
 
-/**
- * Implements hook_ENTITY_TYPE_create() for 'config_test'.
- */
-function config_test_config_test_create(ConfigTest $config_test) {
-  if (\Drupal::state()->get('config_test.prepopulate')) {
-    $config_test->set('foo', 'baz');
-  }
-}
-
 /**
  * Implements hook_entity_type_alter().
  */

core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php

@@ -686,4 +686,98 @@ public function testConfigGetConfigDirectory() {
     }
   }
 
+  /**
+   * Tests the isSyncing flags.
+   */
+  public function testIsSyncingInHooks() {
+    $dynamic_name = 'config_test.dynamic.dotted.default';
+    $storage = $this->container->get('config.storage');
+
+    // Verify the default configuration values exist.
+    $config = $this->config($dynamic_name);
+    $this->assertSame('dotted.default', $config->get('id'));
+
+    // Delete the config so that create hooks will fire.
+    $storage->delete($dynamic_name);
+    \Drupal::state()->set('config_test.store_isSyncing', []);
+    $this->configImporter->reset()->import();
+
+    // The values of the syncing values should be stored in state by
+    // config_test_config_test_create().
+    $state = \Drupal::state()->get('config_test.store_isSyncing');
+    $this->assertTrue($state['global_state::create'], '\Drupal::isConfigSyncing() returns TRUE');
+    $this->assertTrue($state['entity_state::create'], 'ConfigEntity::isSyncing() returns TRUE');
+    $this->assertTrue($state['global_state::presave'], '\Drupal::isConfigSyncing() returns TRUE');
+    $this->assertTrue($state['entity_state::presave'], 'ConfigEntity::isSyncing() returns TRUE');
+    $this->assertTrue($state['global_state::insert'], '\Drupal::isConfigSyncing() returns TRUE');
+    $this->assertTrue($state['entity_state::insert'], 'ConfigEntity::isSyncing() returns TRUE');
+
+    // Cause a config update so update hooks will fire.
+    $config = $this->config($dynamic_name);
+    $config->set('label', 'A new name')->save();
+    \Drupal::state()->set('config_test.store_isSyncing', []);
+    $this->configImporter->reset()->import();
+
+    // The values of the syncing values should be stored in state by
+    // config_test_config_test_create().
+    $state = \Drupal::state()->get('config_test.store_isSyncing');
+    $this->assertTrue($state['global_state::presave'], '\Drupal::isConfigSyncing() returns TRUE');
+    $this->assertTrue($state['entity_state::presave'], 'ConfigEntity::isSyncing() returns TRUE');
+    $this->assertTrue($state['global_state::update'], '\Drupal::isConfigSyncing() returns TRUE');
+    $this->assertTrue($state['entity_state::update'], 'ConfigEntity::isSyncing() returns TRUE');
+
+    // Cause a config delete so delete hooks will fire.
+    $sync = $this->container->get('config.storage.sync');
+    $sync->delete($dynamic_name);
+    \Drupal::state()->set('config_test.store_isSyncing', []);
+    $this->configImporter->reset()->import();
+
+    // The values of the syncing values should be stored in state by
+    // config_test_config_test_create().
+    $state = \Drupal::state()->get('config_test.store_isSyncing');
+    $this->assertTrue($state['global_state::predelete'], '\Drupal::isConfigSyncing() returns TRUE');
+    $this->assertTrue($state['entity_state::predelete'], 'ConfigEntity::isSyncing() returns TRUE');
+    $this->assertTrue($state['global_state::delete'], '\Drupal::isConfigSyncing() returns TRUE');
+    $this->assertTrue($state['entity_state::delete'], 'ConfigEntity::isSyncing() returns TRUE');
+  }
+
+  /**
+   * Tests that the isConfigSyncing flag is cleanup after an invalid step.
+   */
+  public function testInvalidStep() {
+    $this->assertFalse(\Drupal::isConfigSyncing(), 'Before an import \Drupal::isConfigSyncing() returns FALSE');
+    $context = [];
+    try {
+      $this->configImporter->doSyncStep('a_non_existent_step', $context);
+      $this->fail('Expected \InvalidArgumentException thrown');
+    }
+    catch (\InvalidArgumentException $e) {
+      $this->pass('Expected \InvalidArgumentException thrown');
+    }
+    $this->assertFalse(\Drupal::isConfigSyncing(), 'After an invalid step \Drupal::isConfigSyncing() returns FALSE');
+  }
+
+  /**
+   * Tests that the isConfigSyncing flag is set correctly during a custom step.
+   */
+  public function testCustomStep() {
+    $this->assertFalse(\Drupal::isConfigSyncing(), 'Before an import \Drupal::isConfigSyncing() returns FALSE');
+    $context = [];
+    $this->configImporter->doSyncStep([self::class, 'customStep'], $context);
+    $this->assertTrue($context['is_syncing'], 'Inside a custom step \Drupal::isConfigSyncing() returns TRUE');
+    $this->assertFalse(\Drupal::isConfigSyncing(), 'After an valid custom step \Drupal::isConfigSyncing() returns FALSE');
+  }
+
+  /**
+   * Helper meothd to test custom config installer steps.
+   *
+   * @param array $context
+   *   Batch context.
+   * @param \Drupal\Core\Config\ConfigImporter $importer
+   *   The config importer.
+   */
+  public static function customStep(array &$context, ConfigImporter $importer) {
+    $context['is_syncing'] = \Drupal::isConfigSyncing();
+  }
+
 }