# Refactor configuration import and sync functions

 Project: Drupal core Version: 8.x-dev Component: configuration system Category: task Priority: major Assigned: catch Status: fixed Issue tags: Avoid commit conflicts, Configuration system

## Issue Summary

### Proposed resolution

• refactor code to create ConfigSync class to manage configuration import and installs

• needs validation of approach
• needs tests for Symfony events

### #1

 Status: active » needs review

Here's an initial patch the converts config_import, config_install_default_config, and ViewTestData::importTestViews to use a new ConfigSync class to manage and perform the synchronisation tasks.

It also fixes a bug in ViewTestData::importTestViews - the current version only works for a single module in the modules array - fortunately we only ever pass it a single module.

AttachmentSizeStatusTest resultOperations
1890784.drupal8.config-sync.patch24.54 KBIdleFAILED: [[SimpleTest]]: [MySQL] 50,894 pass(es), 2 fail(s), and 0 exception(s).View details | Re-test

### #2

 Status: needs review » needs work

The last submitted patch, 1890784.drupal8.config-sync.patch, failed testing.

### #4

 Status: needs work » needs review

This patch contains the fix from #1889752: Remove unnecessary manifest creation in config_install_default_config() and so was broken in the same way as explained in http://drupal.org/node/1889752#comment-6949574

Additionally the processed list was not correctly maintained so config entities were imported twice resulting in views with no uuid.

AttachmentSizeStatusTest resultOperations
1-4-interdiff.txt1.99 KBIgnored: Check issue status.NoneNone

### #5

 Status: needs review » needs work

The last submitted patch, 1890784.4.drupal8.config-sync.patch, failed testing.

### #6

Doing a retest as install works locally for me.

### #7

 Status: needs work » needs review

#4: 1890784.4.drupal8.config-sync.patch queued for re-testing.

### #8

#4: 1890784.4.drupal8.config-sync.patch queued for re-testing.

### #9

 Status: needs review » needs work

The last submitted patch, 1890784.4.drupal8.config-sync.patch, failed testing.

### #10

Rerolled

AttachmentSizeStatusTest resultOperations
1890784-refactor-import-10.patch14.11 KBIdleFAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.View details | Re-test

### #11

 Status: needs work » needs review

### #12

 Status: needs review » needs work

The last submitted patch, 1890784-refactor-import-10.patch, failed testing.

### #13

So four hours ago I was getting on a plane and the patch in #4 failed to apply. So I rerolled it, incorrectly, without the new files. So now I just started over with a fresh checkout and a fresh branch ... and the patch in #4 now magically applies. So I'm throwing it back to the bot.

### #14

 Status: needs work » needs review

#4: 1890784.4.drupal8.config-sync.patch queued for re-testing.

### #15

It conflicted with #1831264: Use the Entity manager to create new controller instances, rerolled.

AttachmentSizeStatusTest resultOperations
1890784-config-15.patch25.28 KBIdlePASSED: [[SimpleTest]]: [MySQL] 50,813 pass(es).View details | Re-test

### #16

I'm not sure I see where this patch is a net win, but then again I don't see moving procedural code to objects as being a net win in and of itself in general. Is there a specific reason why this is better encapsulated into objects than just leaving it in the include file?

- Calling a function called setChanges() with no parameters strikes me as odd. I'd actually expect the change list to be generated automatically when the ConfigSync object is constructed since in the majority of cases you'll just want to construct and sync. If you want to set a different change set, use setChanges() then.

- I think doSync() would make more sense just being named sync().

- I do like the switch to using events, as it makes our notification system more consistent with what we're doing for overrides.

### #17

 Issue tags: +Stalking Crell

- I do like the switch to using events, as it makes our notification system more consistent with what we're doing for overrides.

this isn't a switch to events. it's an addition of an event - nothing equivalent existed before this patch.

i agree with #16 - i don't see the benefit here with OO. i'd rather we dropped this patch entirely and changed it to just the change to add the sync event.

if we are going to OOify this, then i think we need to register this with the container, and inject the event dispatcher? and invoke Crell on it?

### #18

I started this patch after spending some time thinking about the config validation patch. I realised that in order to mirror the rest of the Config sub system we'd need to use Symfony events to be more consistent. Once I was going down this route I wanted to pass the event something to act on and that could provide functions to get information about and manage the synchronisation - hence the conversion to OO.

I've been working on the addition of a validate event and moving the config name validation to an event subscriber to try to show the value of this approach. In doing this I've created a config_sync service on the container. This has turned out to have a significant benefit. At the moment when we press submit on the ConfigUI we calculate the changelist to build the form and then we rebuild it completely during config_import. Registering the config_sync service allows us to do this only once.

I think we might discover further advantages to an OO approach - for example - creating an event to be able to defer changes or reorder the changelist depending on dependencies.

### #19

i'm mostly meh about the OO conversion, so i don't mind if it goes in. wouldn't be my first choice, but meh ;-)

if you want an event fired after import, lets discuss that.

if you want to implement deferring, reordering based on dependencies etc during import, lets discuss that.

but - can we please not push functional changes as side effects of implementation details?

### #20

This patch:

• Creates the config.import service on the container
• Moves config name validation out of the import functions by creating the validate event. We need to validate the import before beginning the import - if we do this then we won't end up with a half imported site. At the moment if you have 5 config files to import and the third has a too long name the first two will be imported before the config import fails leaving your site in an unintended state.
• setChanges() is requires a list of changes and adds a function to create a default list by comparing source and target storages
• Renames all *Sync to *Import as this is actually more consistent - since we did away with synchronising to staging we now only need to provide functionality for importing.

All I'm trying to do is to refactor config import to point where we can solve some of the issues we have in the follow up patches. I was not saying that this patch should implement deferring or re-ordering. What I was trying to say is that we should try to make these easy to implement. In my opinion if we keep the current procedural approach this will lead to extra complexity.

The interdiff is bigger that the patch because some of the new classes have been renamed.

AttachmentSizeStatusTest resultOperations
1890784.19.drupal8.config-import.patch32.05 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,371 pass(es).View details | Re-test
15-19-interdiff.txt34.74 KBIgnored: Check issue status.NoneNone

### #21

I absolutely and totally agree that config.import/config.sync should be a service. @alexpott and I discussed this at DrupalCon Munich already, with the important conclusion that turning this into a classed service will be the only way for contrib/custom sites to invent a better configuration import/sync/staging implementation in D8. Procedural functions can't be swapped out.

And as things currently stand, contrib will have to re-invent in D8, in order to fix and close the gaps that we failed to address in core...

(I, for one, will probably work on an implementation that replaces all machine names with UUIDs in config names across the board, in order to truly guarantee uniqueness.)

So, thanks for kick-starting this, Alex. Will try to review this patch some more in-depth as soon as possible. Any chance you could push your code into the cmi sandbox? :)

### #23

sun is correct. A straight conversion of procedural functions to injected DIC services automatically gets you:

2) Lazy-initializing services (when you need it it's initialized, but you don't spend time initializing until you need it).
3) Contrib can rip it out and replace it if it wants, as long as it follows the interface.

That's a strong case for service-ifying most systems all on its own, IMO.

I don't fully understand the problem space, but since I was invoked, some additional cleanliness/architectural comments below. From my incomplete read through, this does seem like a reasonably good use of events.

+++ b/core/lib/Drupal/Core/Config/ConfigImport.php@@ -0,0 +1,373 @@+  public function __construct(StorageInterface $target_storage, EventDispatcher$event_dispatcher = NULL) {+    $this->targetStorage =$target_storage;+    $this->eventDispatcher =$event_dispatcher ? $event_dispatcher : drupal_container()->get('event_dispatcher');+$this->resetLists();

Don't bother with the hard coded drupal_container() call. If the event dispatcher is required, then it's required. Passing it in is what the DIC is for.

+++ b/core/lib/Drupal/Core/CoreBundle.php@@ -60,6 +60,12 @@ public function build(ContainerBuilder $container) {+ // Register configuration synchronisation.+$container+      ->register('config.import', 'Drupal\Core\Config\ConfigImport')+      ->addArgument(new Reference('config.storage'))+      ->addArgument(new Reference('event_dispatcher'));

Shouldn't this be config.importer? It's an object that does things, so it has an "er" suffix for thing it does.

+++ b/core/modules/config/config.admin.inc@@ -28,15 +28,16 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa+  $config_import = drupal_container()->get('config.import')->setSourceStorage(drupal_container()->get('config.storage.staging'));+ if (!$config_import->defaultChangelist()->hasChanges()) {     $form['no_changes'] = array( This should be done in the container configuration. You can call: <?php$definition->addMethodCall('setSourceStorage', new Reference('config.storage.staging'));?>

And you're good.

If this is something that gets set per-use case, then it should either be multiple DIC registrations or a factory in the DIC, I think. (Although I don't know the specific use case in detail.)

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php@@ -180,8 +188,10 @@ function testNameValidation() {+      drupal_container()->get('config.import')+        ->setSourceStorage(drupal_container()->get('config.storage.staging'))+        ->defaultChangelist()+        ->import();

Ibid.

Or, really, this is in a test so you shouldn't be calling the container in the first place. Especially inside DUTB. Just make objects yourself and use 'em. That's how you keep unit tests unit-y.

### #24

Thanks for the great review @crell!

Patch attached tries to implement all your suggestions:

• ConfigImport renamed ConfigImporter
• Removes hardcoded container call
• Adds addMethodCall to config.importer service... and creates an new config.installer service
• Fixes ConfigImport DUTB to just test the ConfigImporter class and not use the container

Unfortunately the interdiff can't be constructed since this is a reroll.

Pushed rerolled branch to heyrocker's sandbox.

AttachmentSizeStatusTest resultOperations
1890784.24.drupal8.config-sync.patch34.1 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,042 pass(es).View details | Re-test

### #25

 Status: needs review » needs work

The last submitted patch, 1890784.24.drupal8.config-sync.patch, failed testing.

### #26

 Status: needs work » needs review

This failed on Drupal\text\Tests\TextSummaryTest

The test did not complete due to a fatal error. Completion check TextSummaryTest.php 169 Drupal\text\Tests\TextSummaryTest->testOnlyTextSummary()

Does not occur locally - sending for a retest.

### #27

#24: 1890784.24.drupal8.config-sync.patch queued for re-testing.

### #28

Config snapshot creation is now an event subscriber to the import event.

AttachmentSizeStatusTest resultOperations
1890784.28.drupal8.config-import.patch42.41 KBIdlePASSED: [[SimpleTest]]: [MySQL] 52,234 pass(es).View details | Re-test

### #29

Forgot to attach diff of 24 to 28 - it's not a true interdiff as this was a reroll...

AttachmentSizeStatusTest resultOperations
24-28-interdiff.txt16.29 KBIgnored: Check issue status.NoneNone

+++ b/core/includes/config.incundefined@@ -24,28 +23,23 @@+    drupal_container()->get('config.installer')->setSourceStorage($source_storage)+ // Ignore manifest files when creating changelist to import.+ ->defaultChangelist(FALSE)+ // Only import new config. Changed config is from previous enables and+ // should not be overwritten.+ ->removeChangelist('delete')+ ->removeChangelist('change')+ ->import(); The removeChangelist sucks... Patch attached fixes this and removes the config.installer service from the container as this is only used in one place and does not need to injected. AttachmentSizeStatusTest resultOperations 28-30-interdiff.txt8.04 KBIgnored: Check issue status.NoneNone 1890784.30.drupal8.config-import.patch42.79 KBIdleFAILED: [[SimpleTest]]: [MySQL] 48,779 pass(es), 98 fail(s), and 0 exception(s).View details | Re-test ### #31 Invoking https://twitter.com/chx/status/242326777538695168/photo/1 , trying to imagine what stof would do with this API. What about <?php$installer  ->setSourceStorage($source_storage) // Ignore manifest files when creating changelist to import. Only import // new config. Changed config is from previous enables and should not be // overwritten. ->useManifest(FALSE) ->addChangeList('create') ->import();?> ### #32  Status: needs review » needs work The last submitted patch, 1890784.30.drupal8.config-import.patch, failed testing. ### #33  Status: needs work » needs review Thanks for the review @chx AttachmentSizeStatusTest resultOperations 30-33-interdiff.txt13.55 KBIgnored: Check issue status.NoneNone 1890784.33.drupal8.config-import.patch43.24 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1890784.33.drupal8.config-import.patch. Unable to apply patch. See the log in the details link for more information.View details | Re-test ### #34  Assigned to: Anonymous » sun I think this is RTBC but as sun written this , he should review it. ### #35 ### #36  Status: needs review » needs work The last submitted patch, 1890784.33.drupal8.config-import.patch, failed testing. ### #37  Assigned to: sun » alexpott Working on a reroll... Changes due to the config context patch :) ### #38  Priority: normal » major Assigned to: alexpott » Anonymous Status: needs work » needs review Refactored the patch again due to the impact of context and other changes in core since this #33. This really requires a re-review... and interdiff is not possible because of the re-roll. Summary of changes to #33: • Made the patch play nicely with ConfigContext • Injected more of the dependencies into the ConfigImporter class • Instead of doing a reset of the ConfigFactory static cache after every configuration object is written during import... only do this once. • Changed 'change' operation to 'update' as that is what it is :) ... this is U of a CRUD and change encompasses both delete and creation • Removed the concept of being able to limit the default changelist to a particular operation. Added new methods addChangelistCreate, addChangelistUpdate, addChangelistDelete on the importer to solve this issue in a much cleaner way. AttachmentSizeStatusTest resultOperations 1890784.38.drupal8.config-import.patch51.49 KBIdleFAILED: [[SimpleTest]]: [MySQL] 53,098 pass(es), 117 fail(s), and 387 exception(s).View details | Re-test ### #39  Status: needs review » needs work The last submitted patch, 1890784.38.drupal8.config-import.patch, failed testing. ### #40 Okay some views test's have an unmet dependency on the theme system being configured... And config factory reset needs to clear the static cache not just reinitialise the config. I think this is the correct thing to do because currently we are reinitialising all the config stored in the static cache on a reset() but we have no idea if we're actually going to use it again. AttachmentSizeStatusTest resultOperations 38-40-interdiff.txt2.33 KBIgnored: Check issue status.NoneNone 1890784.40.drupal8.config-import.patch52.58 KBIdlePASSED: [[SimpleTest]]: [MySQL] 53,387 pass(es).View details | Re-test ### #41  Status: needs work » needs review ### #42 This one has gone a bit quiet. Do we still want it? ### #43 I nIRC, @alexpott confirms that we still want this and he offerred to reroll tomorrow. ### #44 Rerolled... AttachmentSizeStatusTest resultOperations 1890784.44.drupal8.config-import.patch52.78 KBIdleFAILED: [[SimpleTest]]: [MySQL] 55,841 pass(es), 7 fail(s), and 45 exception(s).View details | Re-test ### #45 After chatting to @timplunkett in IRC decided to remove the config_import function so everything is contained with the ConfigImporter class. AttachmentSizeStatusTest resultOperations 44-45-interdiff.txt15.63 KBIgnored: Check issue status.NoneNone 1890784.45.drupal8.config-import.patch61.49 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1890784.45.drupal8.config-import.patch. Unable to apply patch. See the log in the details link for more information.View details | Re-test ### #46  Status: needs review » needs work The last submitted patch, 1890784.45.drupal8.config-import.patch, failed testing. ### #47  Status: needs work » needs review Rerolled... AttachmentSizeStatusTest resultOperations 1890784.47.drupal8.config-import.patch61.49 KBIdleFAILED: [[SimpleTest]]: [MySQL] 55,649 pass(es), 9 fail(s), and 45 exception(s).View details | Re-test ### #48 This is beautiful. Once this is in and I reroll #1945398: Convert config_admin_import_form to a new FormInterface implementation and routing definition., it will be clear how nice this is. It really makes it clear how you can interact with the config import process, and will be immensely useful to contrib enhancements of this workflow. I'll do a thorough review tonight. ### #49  Status: needs review » needs work The last submitted patch, 1890784.47.drupal8.config-import.patch, failed testing. ### #50  Status: needs work » needs review Messed up the reroll of core/lib/Drupal/Core/CoreBundle.php Some more DrupalUnitTestBase tests have undeclared dependencies on the theme system... Patched attached fixes both these issues. AttachmentSizeStatusTest resultOperations 47-50-interdiff.txt3.24 KBIgnored: Check issue status.NoneNone 1890784.50.drupal8.config-import.patch62.5 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,752 pass(es).View details | Re-test ### #51 +++ b/core/includes/config.incundefined@@ -38,22 +30,23 @@ function config_install_default_config($type, $name) {+$installer = new ConfigImporter(+      'config.installer',+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.phpundefined@@ -45,21 +46,32 @@ function testSnapshot() {+    $config_importer = new ConfigImporter(+ 'snapshot.comparer',+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestData.phpundefined@@ -54,16 +48,27 @@ public static function importTestViews($class, $modules = array()) {+$installer = new ConfigImporter(+          'views.test.installer',

This makes me wonder if there is another way to pass the string to the service, so the constructor could be the same and you wouldn't need to always pass 5 of the same services

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined@@ -193,8 +202,26 @@ function testNameValidation() {+    $config_importer = new ConfigImporter(+ 'config.importer',+$this->container->get('config.storage'),+      $this->container->get('event_dispatcher'),+$this->container->get('config.context.free'),+      $this->container->get('config.factory'),+$this->container->get('plugin.manager.entity'),+      $this->container->get('lock')+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.phpundefined@@ -39,6 +47,17 @@ function setUp() {+$this->config_importer = new ConfigImporter(+      'config.importer',+      $this->container->get('config.storage'),+$this->container->get('event_dispatcher'),+      $this->container->get('config.context.free'),+$this->container->get('config.factory'),+      $this->container->get('plugin.manager.entity'),+$this->container->get('lock')

These could be $this->container->get('config.importer');, right? +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.phpundefined@@ -45,21 +46,32 @@ function testSnapshot() {+$snapshot,+      drupal_container()->get('event_dispatcher'),+      $this->container->get('config.context.free'),$this->container

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestStorageController.phpundefined@@ -26,13 +26,13 @@ public function importCreate($name, Config$new_config, Config $old_config) {- * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importChange().+ * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importUpdate(). */- public function importChange($name, Config $new_config, Config$old_config) {+  public function importUpdate($name, Config$new_config, Config $old_config) { // Set a global value we can check in test code.$GLOBALS['hook_config_import'] = __METHOD__;-    return parent::importChange($name,$new_config, $old_config);+ return parent::importUpdate($name, $new_config,$old_config);

This is a bit weird to change here, but I like the new name better anyway.

### #52

 Status: needs review » needs work

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined@@ -96,15 +96,13 @@ public function get($name) {+ unset($this->cache[$cache_key]); I have probably missed something but if it's a reset, can it not just set$this->cache = array()?

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@++class ConfigImporter {

Docblock

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@+   * List of config entities in the managed by manifests in the source storage.

huh? :)

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@+   * Add changes to the changelist.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@+    $changes = array_diff($changes, $this->changelist[$op]);+    $this->changelist[$op] = array_merge($this->changelist[$op], $changes); Isn't it the same to += the two arrays together, as then only keys that aren't already set will be added? +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@+ if (substr($name, 0, 9) != 'manifest.') {

This would be changed to strpos($name, 'manifest.') === 0. It's all preference though I guess. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@+ if (count($this->getUnprocessed($op))) { Seems strange doing count there? Would we usually do !empty()? Either way, this will work just fine. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.phpundefined@@ -0,0 +1,48 @@+ foreach (array('delete', 'create', 'update') as$op) {+      foreach ($event->getConfigImporter()->getUnprocessed($op) as $name) { This snippet gets used a few times, not sure if it's worth trying to abstract this out at all? Probably not actually. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.phpundefined@@ -0,0 +1,48 @@+$events['config.importer.validate'][] = array('onConfigImporterValidate', 40);+    $events['config.installer.validate'][] = array('onConfigImporterValidate', 40);+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigSnapshotSubscriber.phpundefined@@ -0,0 +1,68 @@+$events['config.importer.import'][] = array('onConfigImporterImport', 40);

Maybe we should add some class constants for this event string?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined@@ -193,8 +202,26 @@ function testNameValidation() {+    catch (ConfigNameException $e) {+$this->pass($message);+ } What happens if an exception is not thrown? The tests will still pass. What we have done in tests before is set a variable and then evaluate after the try/catch. ### #53  Status: needs work » needs review Thanks for the reviews! In reply to @timplunkett #51 1. @timplunkett "This makes me wonder if there is another way to pass the string to the service" ... suggestions welcome :) 2. Fixed - did it this way due to @crell's last comment in #23 ... but that was before so many dependencies 3. Fixed 4. I had change importChange to importUpdate to match the$op's being stored in the changelists... create, delete and change just didn't seem right... have a look in ConfigImporter::importInvokeOwner()

1. Yep you missed that here we're clearing the static cache of a specific config object not all... see ConfigFactory::reset()
2. Fixed
3. Fixed
4. Fixed
5. Nope we have numeric keys so this doesn't have the desired effect of unique array values.
6. The strpos version is not quite equivalent... the current version is also only testing the first 9 characters of the config name
7. You can't use empty() to test function returns - see http://php.net/manual/en/function.empty.php
8. I tried but I agree probably not :)
9. The point here is that the events are based on the ConfigImporter instance's servicename... I don't think these should be class constatns
10. Added a $this->fail() if the exception is not thrown. Whilst adding the docblock for ConfigImporter (@damienkloip's point 2) I realised that the comparison of configuration stored in different config storage controllers is a different concern than actually doing the import. The attached patch implements a StorageComparerInterface and 2 classes that implement it ConfigComparer and CongirComparerManifest - the difference being that one uses manifests to decide what to do for config entities. An additional advantage of this approach can be seen in the ConfigSnapshotTest where comparing the snapshot storage to active and staging is very simple (contrast with the patch in #50). AttachmentSizeStatusTest resultOperations 50-53-interdiff.txt40.28 KBIgnored: Check issue status.NoneNone 1890784.53.drupal8.config-import.patch67.26 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,955 pass(es).View details | Re-test ### #54 And one more thing... +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@+ // Use an override free context for importing so that overrides to do not+ // pollute the imported data. The context is hard coded to ensure this is+ // the case.+$this->context = new FreeConfigContext($this->eventDispatcher); After chatting with @beejeebus in IRC he pointed out that injecting a config context here made no sense as it only makes sense to run the import with a FreeConfigContext. ### #55 +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@+ * @param \Drupal\Component\Plugin\PluginManagerInterface$entity_manager+   *   The entity plugin manager used to import config entities....+    $this->pluginManager =$plugin_manager;

I think entityManager is a better name here, and I think its wrong to typehint with an interface that doesn't have the method we want on it. Either we need an interface for EntityManager::getStorageController(), or we should just typehint with the class.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@+        if ($entity_type = config_get_entity_type_by_name($name)) {+          $old_config = new Config($name, $this->storageComparer->getTargetStorage(),$this->context);+          $old_config->load();++$data = $this->storageComparer->getSourceStorage()->read($name);+          $new_config = new Config($name, $this->storageComparer->getTargetStorage(),$this->context);+          if ($data !== FALSE) {+$new_config->setData($data);+ }++$method = 'import' . ucfirst($op);+$handled_by_module = $this->pluginManager->getStorageController($entity_type)->$method($name, $new_config,$old_config);+        }+        if (!empty($handled_by_module)) {+$this->setProcessed($op,$name);

This is way out of scope for this issue, but the coupling of CMI to Config Entities here always bothered me greatly. Is it possible that the ConfigEntity system could subscribe to the event instead (in a follow-up).

It would break the $handled_by_module part, but I'm still unsure if we need that. +++ b/core/lib/Drupal/Core/Config/StorageComparer.phpundefined@@ -0,0 +1,203 @@+ * {@inheritdoc}+ */+ public function __construct($source_storage, $target_storage) {+++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.phpundefined@@ -0,0 +1,130 @@+ /**+ * Constructs the Configuration storage comparer.+ *+ * @param \Drupal\Core\Config\StorageInterface$source_storage+   *   Storage controller object used to read configuration.+   * @param \Drupal\Core\Config\StorageInterface $target_storage+ * Storage controller object used to read configuration.+ */+ public function __construct($source_storage, $target_storage); Needs typehinting, and we should move it out of the interface (we don't usually put __construct in interfaces) +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigSnapshotSubscriber.phpundefined@@ -0,0 +1,68 @@+ static function getSubscribedEvents() { public? ### #56 @timplunkett / #55 1. Agreed and fixed 2. Yep we could add an event which always modules to do handle an import instead of the general process - definitely out of scope here. 3. Fixed 4. static function getSubscribedEvents() { is how all the other core events are... AttachmentSizeStatusTest resultOperations 1890784.56.drupal8.config-import.patch67.14 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,696 pass(es).View details | Re-test 53-56-interdiff.txt4.44 KBIgnored: Check issue status.NoneNone ### #57  Status: needs review » reviewed & tested by the community Issue tags: -Stalking Crell This looks awesome. Thanks so much @alexpott! ### #58  Status: reviewed & tested by the community » needs review alexpott asked me to give this a standards look. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined--- /dev/null+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@+ * @see \Drupal\Core\Config\StorageComparerInterface...+ * @see \Drupal\Core\Config\ConfigImporterEvent usually @see's are together at the end of the doc block, but iirc, they can also be placed inline if that helps communicate better. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@+ * Each instance of ConfigImporter has a service name which is used to construct+ * event names. The events fired during an import are:+ * - '{service name}.validate': Events listening can throw a+ * \Drupal\Core\Config\ConfigImporterException to prevent an import from small change in formatting for lists in comments. http://drupal.org/node/1354#lists +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@+ * The name of the ConfigImporter. Used to identify events....+ * The storage comparer used to discover configuration changes....+ * The event dispatcher used to notify subscribers. changed to be consistant. . Used ... -> used +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@+ * @var \Drupal\Core\Lock\LockBackendInterface$lock

taking out the $lock http://drupal.org/node/1354#var +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@+ * Changelists to check for changes. Defaults to all changelists, i.e.+ * array('delete', 'create', 'update')....+ public function hasChanges($changelists = array('delete', 'create', 'update')) {...+   * @param string $op+ * The change operation performed. Either create, change or delete....+ * @param$op+   *   The change operation to get the unprocessed list for. Either create,+   *   update or delete.

one missing type in @param.
also, one has change instead of update.

Making them match (and also match the order in the hasChanges definition:
hasChanges($changelists = array('delete', 'create', 'update')) ======= posting what I have so far. I can get back to this in a couple hours. AttachmentSizeStatusTest resultOperations 1890784.58.drupal8.config-import.patch66.94 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,988 pass(es).View details | Re-test interdiff-56-58.txt3.56 KBIgnored: Check issue status.NoneNone ### #59 +++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,347 @@+ /**+ * Checks if there are any unprocessed changes.+ *+ * @param array$changelists+   *   Changelists to check for changes. Defaults to all changelists, i.e.+   *   array('delete', 'create', 'update').+   *+   * @return bool+   *   TRUE if there are changes to process and FALSE if not.+   */+  public function hasChanges($changelists = array('delete', 'create', 'update')) {+ foreach ($changelists as $op) {+ if (count($this->getUnprocessed($op))) {+ return TRUE;+ }+ }+ return FALSE;+ }+++ b/core/lib/Drupal/Core/Config/StorageComparer.phpundefined@@ -0,0 +1,208 @@+ /**+ * Checks if there is a changelist with changes to process.+ *+ * @param array$ops+   *   Operation to check for changes. Defaults to all operations, i.e.+   *   array('delete', 'create', 'update').+   *+   * @return bool+   *   TRUE if there are changes to process and FALSE if not.+   */+  public function hasChanges($ops = array('delete', 'create', 'update')) {+ foreach ($ops as $op) {+ if (!empty($this->changelist[$op])) {+ return TRUE;+ }+ }+ return FALSE;+ }+++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.phpundefined@@ -0,0 +1,120 @@+ /**+ * Checks the changelist has any changes.+ *+ * Until the changelist has been calculated this will always be FALSE.+ *+ * @see \Drupal\Core\Config\StorageComparerInterface::createChangelist().+ *+ * @param array$changelists+   *   Changelists to check for changes. Defaults to all changelists, i.e.+   *   array('delete', 'create', 'update').+   *+   * @return bool+   *   TRUE if there are changes to process and FALSE if not.+   */+  public function hasChanges($ops = array('delete', 'create', 'update'));+ There is the config importer, comparer, and interface. I updated to be more consistant, I wonder if it can use a {@inheritdoc}. +++ b/core/lib/Drupal/Core/Config/StorageComparer.phpundefined@@ -0,0 +1,208 @@+ * @param \Drupal\Core\Config\StorageInterface$source_storage+   *   Storage controller object used to read configuration.+   * @param \Drupal\Core\Config\StorageInterface $target_storage+ * Storage controller object used to read configuration. updating target comment to be "to write". +++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.phpundefined@@ -0,0 +1,120 @@+ * Gets the configuration target storage.+ *+ * @return \Drupal\Core\Config\StorageInterface+ * Storage controller object used to read configuration.+ */+ public function getTargetStorage(); fixed copy and paste error. "to read" -> "to write". +++ b/core/modules/config/config.admin.incundefined@@ -21,7 +22,7 @@ * @param Drupal\Core\Config\StorageInterface$target_storage  *   The target storage to compare differences to.  */-function config_admin_sync_form(array &$form, array &$form_state, StorageInterface $source_storage, StorageInterface$target_storage) {+function config_admin_sync_form(array &$form, array &$form_state, StorageInterface $source_storage) { needed to delete the @param for target that was removed. this is also missing the @return. Maybe this is an unrelated change? +++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestStorageController.phpundefined@@ -26,13 +26,13 @@ public function importCreate($name, Config $new_config, Config$old_config) {-   * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importChange().+   * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importUpdate().

I think this can be an {@inheritdoc} but maybe this was left here to be part of cleaning up this later. So I'll leave it.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestData.phpundefined@@ -7,6 +7,8 @@+use Drupal\Core\Config\ConfigImporter;+use Drupal\Core\Config\StorageComparer; use Drupal\Core\Config\FileStorage;

I think these are usually alphabetical.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,347 @@+  /**+   * Invokes import* methods on configuration entity storage controllers.+   *+   * The changelist is modified as each change is processed.+   *+   * @todo Add support for other extension types; e.g., themes etc.+   */+  protected function importInvokeOwner() {+    // Allow modules to take over configuration change operations for+    // higher-level configuration data.+    // First pass deleted, then new, and lastly changed configuration, in order to+    // handle dependencies correctly.

The Allowed... sentence seems about the function in general so moved it up to the function doc block.

+++ b/core/lib/Drupal/Core/Config/ConfigImporterEvent.phpundefined@@ -0,0 +1,39 @@+    return $this->configImporter;+ }+} added a new line at the end of the class because, mostly we do, and also, other classes in this patch do. AttachmentSizeStatusTest resultOperations 1890784.59.drupal8.config-import.patch67.42 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,855 pass(es).View details | Re-test interdiff-58-59.txt8.81 KBIgnored: Check issue status.NoneNone ### #60 I made the small change about the doc block and changed it to an {@inheritdoc} like YesCT sugjested in comment #59. AttachmentSizeStatusTest resultOperations 1890784.60.drupal8.config-import.patch67.13 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,582 pass(es).View details | Re-test interdiff-59-60.txt749 bytesIgnored: Check issue status.NoneNone ### #61 Talking to alexpott and EllaTheHarpy in irc, we agree the hasChanges in ConfigImporter is different than the hasChanges in ConfigComparer, so renaming it with a better name: hasUnprocessedChanges. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigSnapshotSubscriber.phpundefined@@ -0,0 +1,68 @@diff --git a/core/modules/config/config.admin.inc b/core/modules/config/config.admin.inc+++ b/core/modules/config/config.admin.incundefined@@ -31,18 +33,20 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa+$config_import = Drupal::service('config.importer');+  if (!$config_import->hasChanges()) {+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined@@ -87,7 +97,7 @@ function testDeleted() {+$this->assertFalse($this->configImporter->hasChanges());@@ -138,7 +148,7 @@ function testNew() {+$this->assertFalse($this->configImporter->hasChanges());@@ -195,7 +205,7 @@ function testUpdated() {+$this->assertFalse($this->configImporter->hasChanges()); I dont see a hasChanges in any of the use classes. Why does this work? Is it because it's on an object of type ConfigImporter? Then it might should be the new name hasUnprocessedChanges() AttachmentSizeStatusTest resultOperations 1890784.61.drupal8.config-import.patch67.16 KBIdleFAILED: [[SimpleTest]]: [MySQL] 55,585 pass(es), 11 fail(s), and 4 exception(s).View details | Re-test interdiff-60-61.txt932 bytesIgnored: Check issue status.NoneNone ### #62  Status: needs review » needs work The last submitted patch, 1890784.61.drupal8.config-import.patch, failed testing. ### #63 +++ b/core/modules/config/config.admin.incundefined@@ -31,18 +33,20 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa+ if (!$config_import->hasChanges()) {+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined@@ -87,7 +97,7 @@ function testDeleted() {+    $this->assertFalse($this->configImporter->hasChanges());@@ -138,7 +148,7 @@ function testNew() {+    $this->assertFalse($this->configImporter->hasChanges());@@ -195,7 +205,7 @@ function testUpdated() {+    $this->assertFalse($this->configImporter->hasChanges());

I think these also needed switching?

### #64

 Assigned to: Anonymous » YesCT

yeah, I convinced myself, and the testbot confirms. I'll do it. (since I broke it *grin*)

### #65

 Status: needs work » needs review

Here it is.

AttachmentSizeStatusTest resultOperations
1890784.65.drupal8.config-import.patch67.2 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).View details | Re-test
interdiff-61-65.txt1.79 KBIgnored: Check issue status.NoneNone

### #66

Reviewing the improvements made by YesCT and EllaTheHarpy I discovered a mistake I made in the documentation... well at one point it was true but now it a LIE :)

AttachmentSizeStatusTest resultOperations
65-66-interdiff.txt867 bytesIgnored: Check issue status.NoneNone
1890784.66.drupal8.config-import.patch67.07 KBIdlePASSED: [[SimpleTest]]: [MySQL] 56,247 pass(es).View details | Re-test

### #67

 Status: needs review » reviewed & tested by the community

Great docs and naming improvements!
And the code still looks great.

### #68

looks good to me too.

### #69

Very minor

+++ b/core/includes/config.incundefined@@ -425,6 +195,21 @@ function config_typed() {+ * @param Drupal\Core\Config\StorageInterface $target_storage+ * The storage to synchronize configuration to.+ */+function config_import_create_snapshot(StorageInterface$source_storage, StorageInterface $snapshot_storage) {+$snapshot_storage->deleteAll();

param names don't match :(

+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigSnapshotSubscriber.phpundefined@@ -0,0 +1,68 @@+   * Create config snapshot.

Creates?

### #70

i think is ready to go.

rerolled for the doc only changes in #69.

AttachmentSizeStatusTest resultOperations
1890784-70-import-refactor.patch67.08 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,725 pass(es).View details | Re-test

### #71

 Issue tags: +Avoid commit conflicts

This is likely to have a conflict with #1987660: Convert config_sync() to a new style controller. This should be committed first as the other issue is just moving code hunks it isn't rtbc but just in case.

### #72

 Assigned to: YesCT » Anonymous

### #73

 Status: reviewed & tested by the community » needs review

Talking with @catch about this... he didn't like passing the service name in... and I think @timplunkett wasn't sure about it either.So I've implemented this using class constants.

AttachmentSizeStatusTest resultOperations
70-73-interdiff.txt6.4 KBIgnored: Check issue status.NoneNone
1890784-73-import-refactor.patch68.44 KBIdleFAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.View details | Re-test

### #74

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -220,9 +217,9 @@+      if (!$this->lock->acquire($this::SERVICE_NAME)) {...+        throw new ConfigImporterException(sprintf('Import service %s is already running', $this::SERVICE_NAME));@@ -230,7 +227,7 @@+$this->lock->release($this::SERVICE_NAME);@@ -318,7 +315,7 @@+$this->eventDispatcher->dispatch($this::SERVICE_NAME . '.' .$event_name, new ConfigImporterEvent($this));@@ -328,7 +325,7 @@+ return !$this->lock->lockMayBeAvailable($this::SERVICE_NAME);@@ -338,7 +335,7 @@+ return$this::SERVICE_NAME;

Shouldn't this be static::SERVICE_NAME? That's what we use elsewhere.

### #75

Yep... doh!

AttachmentSizeStatusTest resultOperations
73-75-interdiff.txt1.88 KBIgnored: Check issue status.NoneNone
1890784-75-import-refactor.patch68.45 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,898 pass(es).View details | Re-test

### #76

 Assigned to: Anonymous » catch Status: needs review » reviewed & tested by the community

RTBC if green! This feels much better.

### #77

<?php  public function import() {    if ($this->hasUnprocessedChanges()) { // Ensure that the changes have been validated.$this->validate();      $this->configFactory->enterContext($this->context);?>

does this mean that validation and import run against different config contexts? it hurts my head because i don't really know what that means, but my gut feeling is that is probably not a good idea.

### #78

@beejeebus I did consider this but the reason I did not do this is that I think it should the event listener's job to worry about context if it needs to... If it does it is probably incorrect :)... Because validators should be using the storagecontrollers attached to the storagecomparer to read config in. After all most validators are probably not going to look at what's there but what's coming and therefore will have to read from staging. Additionally what they are most likely to want to compare to active is stuff like machine names and uuid's and context messes will these... well... all bets are off.

### #79

Looks much better with the constants, a couple of things still confuse me bit:

Installing default config is sufficiently rare that I completely agree it doesn't deserve it's own service, however it's weird that there's a StorageComparer service and the class is also instantiated directly - does it need to be a service at all then, why not instantiate directly everywhere?

+++ b/core/includes/config.incundefined@@ -38,22 +31,19 @@ function config_install_default_config($type,$name) {+    $storage_comparer = new StorageComparer($source_storage, Drupal::service('config.storage'));+    // Only import new config. Changed config is from previous enables and

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,341 @@+ * Each instance of ConfigImporter has a service name which is used to construct

Class constant rather than a property on the instance.

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.phpundefined@@ -0,0 +1,25 @@+class ConfigInstaller extends ConfigImporter {++  /**+   * The name used to identify events and the lock.+   */+  const SERVICE_NAME = 'config.installer';+

This defines a service name, but it's not registered as a service. I don't think these are service names at all, but rather operation names or similar.

### #80

 Status: reviewed & tested by the community » needs review

Removed the services added by the patch... Catch is right these will not be used often enough to justify their existence.

Renamed SERVICE_NAME to ID as that's what it is an identifier.

AttachmentSizeStatusTest resultOperations
75-80-interdiff.txt17.49 KBIgnored: Check issue status.NoneNone
1890784.80.drupal8.import-refactor.patch70.95 KBIdlePASSED: [[SimpleTest]]: [MySQL] 55,596 pass(es).View details | Re-test

### #81

 Status: needs review » reviewed & tested by the community

Having them as a service allows the importing to be swapped out. I'm not certain enough of our choices to say that contrib won't have some better ideas.

However, I leave this to catch. I'm fine with either, just worried a bit.

### #82

I think if we allow the importing to be swapped out, we'd also want to allow the defaults installation to be swapped out as well. It's the mismatch rather than them being services that bothers me (although I can't see another reason than swapping the implementation for them to be such).

This feels like something we could change at any time but leaving this open for a bit longer in case people want to discuss.

### #83

 Status: reviewed & tested by the community » fixed

Went ahead and committed/pushed this - if we want to make these services that'll be a simple follow-up - they're only called a couple of places.