Problem/Motivation

Proposed resolution

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

Remaining tasks

  • needs validation of approach
  • needs tests for Symfony events
Files: 
CommentFileSizeAuthor
#80 75-80-interdiff.txt17.49 KBalexpott
#80 1890784.80.drupal8.import-refactor.patch70.95 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,596 pass(es).
[ View ]
#75 73-75-interdiff.txt1.88 KBalexpott
#75 1890784-75-import-refactor.patch68.45 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,898 pass(es).
[ View ]
#73 70-73-interdiff.txt6.4 KBalexpott
#73 1890784-73-import-refactor.patch68.44 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#70 1890784-70-import-refactor.patch67.08 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 55,725 pass(es).
[ View ]
#66 65-66-interdiff.txt867 bytesalexpott
#66 1890784.66.drupal8.config-import.patch67.07 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,247 pass(es).
[ View ]
#65 1890784.65.drupal8.config-import.patch67.2 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).
[ View ]
#65 interdiff-61-65.txt1.79 KBYesCT
#61 1890784.61.drupal8.config-import.patch67.16 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 55,585 pass(es), 11 fail(s), and 4 exception(s).
[ View ]
#61 interdiff-60-61.txt932 bytesYesCT
#60 1890784.60.drupal8.config-import.patch67.13 KBEllaTheHarpy
PASSED: [[SimpleTest]]: [MySQL] 55,582 pass(es).
[ View ]
#60 interdiff-59-60.txt749 bytesEllaTheHarpy
#59 1890784.59.drupal8.config-import.patch67.42 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 55,855 pass(es).
[ View ]
#59 interdiff-58-59.txt8.81 KBYesCT
#58 1890784.58.drupal8.config-import.patch66.94 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 55,988 pass(es).
[ View ]
#58 interdiff-56-58.txt3.56 KBYesCT
#56 1890784.56.drupal8.config-import.patch67.14 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,696 pass(es).
[ View ]
#56 53-56-interdiff.txt4.44 KBalexpott
#53 50-53-interdiff.txt40.28 KBalexpott
#53 1890784.53.drupal8.config-import.patch67.26 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,955 pass(es).
[ View ]
#50 47-50-interdiff.txt3.24 KBalexpott
#50 1890784.50.drupal8.config-import.patch62.5 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,752 pass(es).
[ View ]
#47 1890784.47.drupal8.config-import.patch61.49 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 55,649 pass(es), 9 fail(s), and 45 exception(s).
[ View ]
#45 44-45-interdiff.txt15.63 KBalexpott
#45 1890784.45.drupal8.config-import.patch61.49 KBalexpott
FAILED: [[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 ]
#44 1890784.44.drupal8.config-import.patch52.78 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 55,841 pass(es), 7 fail(s), and 45 exception(s).
[ View ]
#40 38-40-interdiff.txt2.33 KBalexpott
#40 1890784.40.drupal8.config-import.patch52.58 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,387 pass(es).
[ View ]
#38 1890784.38.drupal8.config-import.patch51.49 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 53,098 pass(es), 117 fail(s), and 387 exception(s).
[ View ]
#33 30-33-interdiff.txt13.55 KBalexpott
#33 1890784.33.drupal8.config-import.patch43.24 KBalexpott
FAILED: [[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 ]
#30 28-30-interdiff.txt8.04 KBalexpott
#30 1890784.30.drupal8.config-import.patch42.79 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 48,779 pass(es), 98 fail(s), and 0 exception(s).
[ View ]
#29 24-28-interdiff.txt16.29 KBalexpott
#28 1890784.28.drupal8.config-import.patch42.41 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,234 pass(es).
[ View ]
#24 1890784.24.drupal8.config-sync.patch34.1 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 49,042 pass(es).
[ View ]
#20 1890784.19.drupal8.config-import.patch32.05 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 49,371 pass(es).
[ View ]
#20 15-19-interdiff.txt34.74 KBalexpott
#15 1890784-config-15.patch25.28 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,813 pass(es).
[ View ]
#10 1890784-refactor-import-10.patch14.11 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 1-4-interdiff.txt1.99 KBalexpott
#4 1890784.4.drupal8.config-sync.patch25.13 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1890784.4.drupal8.config-sync.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
1-3-interdiff.txt25.25 KBalexpott
1890784.3.drupal8.config-sync.patch2.32 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1890784.3.drupal8.config-sync.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1890784.drupal8.config-sync.patch24.54 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 50,894 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

alexpott’s picture

Status:Active» Needs review
StatusFileSize
new24.54 KB
FAILED: [[SimpleTest]]: [MySQL] 50,894 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

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

alexpott’s picture

StatusFileSize
new25.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1890784.4.drupal8.config-sync.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.99 KB

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.

Status:Needs review» Needs work

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

alexpott’s picture

Doing a retest as install works locally for me.

alexpott’s picture

Status:Needs work» Needs review
Issue tags:-Configuration system

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

moshe weitzman’s picture

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

Status:Needs review» Needs work
Issue tags:+Configuration system

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

heyrocker’s picture

StatusFileSize
new14.11 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Rerolled

heyrocker’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

heyrocker’s picture

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.

heyrocker’s picture

Status:Needs work» Needs review

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

tim.plunkett’s picture

StatusFileSize
new25.28 KB
PASSED: [[SimpleTest]]: [MySQL] 50,813 pass(es).
[ View ]
heyrocker’s picture

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.

beejeebus’s picture

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?

alexpott’s picture

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.

beejeebus’s picture

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?

alexpott’s picture

StatusFileSize
new34.74 KB
new32.05 KB
PASSED: [[SimpleTest]]: [MySQL] 49,371 pass(es).
[ View ]

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.

sun’s picture

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

alexpott’s picture

Crell’s picture

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:

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

alexpott’s picture

StatusFileSize
new34.1 KB
PASSED: [[SimpleTest]]: [MySQL] 49,042 pass(es).
[ View ]

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.

Status:Needs review» Needs work

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

alexpott’s picture

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.

alexpott’s picture

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

alexpott’s picture

StatusFileSize
new42.41 KB
PASSED: [[SimpleTest]]: [MySQL] 52,234 pass(es).
[ View ]
alexpott’s picture

StatusFileSize
new16.29 KB

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

alexpott’s picture

StatusFileSize
new42.79 KB
FAILED: [[SimpleTest]]: [MySQL] 48,779 pass(es), 98 fail(s), and 0 exception(s).
[ View ]
new8.04 KB
+++ 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.

chx’s picture

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

Status:Needs review» Needs work

The last submitted patch, 1890784.30.drupal8.config-import.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new43.24 KB
FAILED: [[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 ]
new13.55 KB

Thanks for the review @chx

chx’s picture

Assigned:Unassigned» sun

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

Crell’s picture

Status:Needs review» Needs work
Issue tags:+Configuration system, +Stalking Crell

The last submitted patch, 1890784.33.drupal8.config-import.patch, failed testing.

alexpott’s picture

Assigned:sun» alexpott

Working on a reroll... Changes due to the config context patch :)

alexpott’s picture

Assigned:alexpott» Unassigned
Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new51.49 KB
FAILED: [[SimpleTest]]: [MySQL] 53,098 pass(es), 117 fail(s), and 387 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1890784.38.drupal8.config-import.patch, failed testing.

alexpott’s picture

StatusFileSize
new52.58 KB
PASSED: [[SimpleTest]]: [MySQL] 53,387 pass(es).
[ View ]
new2.33 KB

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.

alexpott’s picture

Status:Needs work» Needs review
moshe weitzman’s picture

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

moshe weitzman’s picture

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

alexpott’s picture

StatusFileSize
new52.78 KB
FAILED: [[SimpleTest]]: [MySQL] 55,841 pass(es), 7 fail(s), and 45 exception(s).
[ View ]

Rerolled...

alexpott’s picture

StatusFileSize
new61.49 KB
FAILED: [[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 ]
new15.63 KB

After chatting to @timplunkett in IRC decided to remove the config_import function so everything is contained with the ConfigImporter class.

Status:Needs review» Needs work

The last submitted patch, 1890784.45.drupal8.config-import.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new61.49 KB
FAILED: [[SimpleTest]]: [MySQL] 55,649 pass(es), 9 fail(s), and 45 exception(s).
[ View ]

Rerolled...

tim.plunkett’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 1890784.47.drupal8.config-import.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new62.5 KB
PASSED: [[SimpleTest]]: [MySQL] 55,752 pass(es).
[ View ]
new3.24 KB

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.

tim.plunkett’s picture

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

damiankloip’s picture

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.

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.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new67.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,955 pass(es).
[ View ]
new40.28 KB

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

In reply to @damienkloip #52
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).

alexpott’s picture

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.

tim.plunkett’s picture

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

alexpott’s picture

StatusFileSize
new4.44 KB
new67.14 KB
PASSED: [[SimpleTest]]: [MySQL] 55,696 pass(es).
[ View ]

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Stalking Crell

This looks awesome. Thanks so much @alexpott!

YesCT’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.56 KB
new66.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,988 pass(es).
[ View ]

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.

YesCT’s picture

StatusFileSize
new8.81 KB
new67.42 KB
PASSED: [[SimpleTest]]: [MySQL] 55,855 pass(es).
[ View ]
+++ 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.

EllaTheHarpy’s picture

StatusFileSize
new749 bytes
new67.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,582 pass(es).
[ View ]

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

YesCT’s picture

StatusFileSize
new932 bytes
new67.16 KB
FAILED: [[SimpleTest]]: [MySQL] 55,585 pass(es), 11 fail(s), and 4 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1890784.61.drupal8.config-import.patch, failed testing.

tim.plunkett’s picture

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

YesCT’s picture

Assigned:Unassigned» YesCT

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

YesCT’s picture

Status:Needs work» Needs review
StatusFileSize
new1.79 KB
new67.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).
[ View ]

Here it is.

alexpott’s picture

StatusFileSize
new67.07 KB
PASSED: [[SimpleTest]]: [MySQL] 56,247 pass(es).
[ View ]
new867 bytes

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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

YesCT’s picture

looks good to me too.

aspilicious’s picture

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?

beejeebus’s picture

StatusFileSize
new67.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,725 pass(es).
[ View ]

i think is ready to go.

rerolled for the doc only changes in #69.

marcingy’s picture

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.

YesCT’s picture

Assigned:YesCT» Unassigned
alexpott’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new68.44 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new6.4 KB

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.

tim.plunkett’s picture

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

alexpott’s picture

StatusFileSize
new68.45 KB
PASSED: [[SimpleTest]]: [MySQL] 55,898 pass(es).
[ View ]
new1.88 KB

Yep... doh!

tim.plunkett’s picture

Assigned:Unassigned» catch
Status:Needs review» Reviewed & tested by the community

RTBC if green! This feels much better.

beejeebus’s picture

it hurts my head to ask this question, but:

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

alexpott’s picture

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

catch’s picture

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.

alexpott’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new70.95 KB
PASSED: [[SimpleTest]]: [MySQL] 55,596 pass(es).
[ View ]
new17.49 KB

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.

tim.plunkett’s picture

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.

catch’s picture

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.

catch’s picture

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.

effulgentsia’s picture

Committed, so no conflicts to avoid anymore.

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

Anonymous’s picture

Issue summary:View changes

Add issue that is postponed on this.