Refactor configuration import and sync functions

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

Rerolled

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.

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

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

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?

Some general comments:

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

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

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

1) Lazy-loading code.
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:

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

Invoking https://twitter.com/chx/status/242326777538695168/photo/1 , trying to imagine what stof would do with this API.

What about

$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();

I think this is RTBC but as sun written this , he should review it.

#33: 1890784.33.drupal8.config-import.patch queued for re-testing.

This one has gone a bit quiet. Do we still want it?

I nIRC, @alexpott confirms that we still want this and he offerred to reroll tomorrow.

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.

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

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

Adds

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

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

This looks awesome. Thanks so much @alexpott!

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.

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

I made the small change about the doc block and changed it to an {@inheritdoc} like YesCT sugjested in comment #59.

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

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

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

Here it is.

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

looks good to me too.

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?

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.

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

RTBC if green! This feels much better.

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.

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.

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.

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.

Committed, so no conflicts to avoid anymore.