Add a separate module install/uninstall step to the config import process

Maybe better change title to - Allow configuration import to enable/disable modules :)

Yes, two passes sound sensible to me.

This shouldn't actually be very hard; I expect a ~10 lines patch here.

There's one complexity to this though:

1) Modules that need to be enabled need to be enabled before executing the import.

2) Modules that need to be disabled... are a different can of worms. Upfront or after? Will import hooks have to be executed or not? Dependencies?

3) Modules that need to be uninstalled... oh.my.holy.f/me runs.

And: Speaking of hooks in 1) and 2), I guess we actually need to run through module_enable() + module_disable().

Lastly, do we handle themes at the same time? Or separate issue? Or not at all? (I'd disagree with the last option.)

This rabbit hole is very deep. I suspect that we need to define an explicit dependency order between CMI objects (and have some sensible defaults, very likely building on the module dependencies) and import in that order and perhaps reboot Drupal (batch API? Just a big bad reset? You need to reset statics and rebuild the DIC, at least) between steps. It's a gigantic mess.

Think of modules, field types, entity types, entity bundles, fields, instances, actions and what dependencies they can have on each other.

Because it wasn't mentioned yet, it needs to be asked, since we briefly discussed the alternative option a while ago:

What if we'd apply a special behavior to extension handling in the config import process?

I.e., instead of enabling/disabling extensions the normal way, we just update the respective system lists, and if necessary, install the schema. That would exclude hook_install(), hook_enable(), hook_disable(), and installation of default config. We'd reset/reboot once after doing that, and then go ahead with the existing import process.

This alternative sounds scary to me, but mayhaps, it is less scary than the problem space of having to deal with interdependencies between extensions.

OTOH, dependencies in config should actually be off-topic here -- that's what #1605460: Implement dependencies and defer process for importing configuration is about. Dependencies between extensions are properly handled built-in already; that's what the second argument to module_enable() & co is for. Perhaps it makes most sense to focus and progress in a pragmatic way.

Sorry for the noise, just trying to clarify the issue title. Changes in the system lists are actually imported correctly already (just like any other config). The actual problem is that newly enabled extensions do not participate in the config staging/import process; i.e., their config import hooks are not invoked.

This is an important issue and one of the reasons why I started on #1890784: Refactor configuration import and sync functions

Another thing we need to consider apart from just the config_import hooks are any install / disable / schema hooks. At the moment these are not fired at all.

The attached patch (which will fail) shows that after a config_import that updates system.modules - the module does not exist on the service, the schema is not installed and default config is not installed.

I'm pretty sure we cannot release without this, so bumping to critical.

The latest patch in #15 looks a bit messy ;) but I can perfectly see that the required logic will be relatively ugly either way.

Given the discussion results of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed — I'd suggest to simply and strictly deny importing any config in case any module is disabled. Removing that exceptionally horrible edge-case from the equation will likely make the logic a lot easier.

Furthermore, do we want to bake this into the existing syncing functions, or do we perhaps want to split the functionality into completely separate functions? (and change the existing to ignore those special config object names) I'd assume that splitting it out would help us to make the code a little bit cleaner?

We also need to update this for the new module_handler service.

Looking at the code in #15, and realizing we don't have a current solution for this yet, I think it would make sense to postpone this on #1890784: Refactor configuration import and sync functions. It will be easier to tackle once that is done.

I do agree that this is a critical that needs to be fixed.

Also, if #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed is decided, it will make this more simple.

Patch attached adds

• adds a step to handle any change to the system module files
• adds a step to handle any change to the system theme files
• a prioritisation event to config import so modules can reorder to the list config to import

Removed prioritisation step as it is out-of-scope and untested...
Recalculating differences after enable / disable / uninstall...
Handling errors a bit better...

Fixes installer and ensure that during a module or theme enable we don't end up in some mad recursion...

And to fix the view installer...

So at the moment this recalculates the changelist after installing modules and themes as these actions create config in the active store and therefore stuff is going to have changed - @beejeebus thinks this is the way to go. Last night I said I wasn't sure and now thinking more about it I'm convinced and here is why...

Scenario 1 - with code in #30

Starting condition DEV and LIVE matching environments from configuration perspective...

On DEV

1. Enable taxonomy module which create views.view.taxonomy_term
2. Rename views.view.taxonomy_term views.view.my_special_flower
3. Export config

Then on LIVE import config

1. Enables taxonomy module
2. Which creates views.view.taxonomy_term
3. Recalculates changelist
4. Rename views.view.taxonomy_term views.view.my_special_flower

Scenario 2 - with code #30

Starting condition DEV and LIVE matching environments from configuration perspective...

On DEV

1. Enable taxonomy module which create views.view.taxonomy_term
2. Rename views.view.taxonomy_term views.view.myspcial_flower
3. Rename views.view.myspcial_flower views.view.my_special_flwer
4. Rename views.view.my_special_flwer views.view.my_special_flower
5. Export config

Then on LIVE import config

1. Enables taxonomy module
2. Which creates views.view.taxonomy_term
3. Recalculates changelist
4. Rename views.view.taxonomy_term views.view.my_special_flower

The point being we now have not followed exactly the same set of renames on LIVE as on DEV.

Scenario 2 - how I think it should work

Starting condition DEV and LIVE matching environments from configuration perspective...

On DEV

1. Enable taxonomy module which create views.view.taxonomy_term
2. Rename views.view.taxonomy_term views.view.myspcial_flower
3. Rename views.view.myspcial_flower views.view.my_special_flwer
4. Rename views.view.my_special_flwer views.view.my_special_flower
5. Export config

Then on LIVE import config

1. Enables taxonomy module
2. As this is an import module enable we ignore default config because we have a full config tree
3. Create views.view.my_special_flower

I think the given that two systems A and B start in exactly the same state. System A can take what ever path it likes to get to a point where it exports. On importing system B should take the shortest possible path to get to the expected config. As there is absolutely no way we can know the exact path the system A took to get to the point where it was when it exports its config.

... however this still does not get away from the fact we still have to implement renames. Because of this situation... it is just a different problem and out-of-scope for dealing with module and theme enables...

Scenario 3 - why renames are vital

Starting condition DEV and LIVE matching environments from configuration perspective...

On DEV

1. Enable taxonomy module which create views.view.taxonomy_term
2. Export config

Then on LIVE import config

1. Enables taxonomy module
2. Creates views.view.taxonomy_term

Then on DEV

1. Rename views.view.taxonomy_term views.view.my_special_flower
2. Export config

Then on LIVE import config

1. Enables taxonomy module
2. Renames views.view.taxonomy_term views.view.my_special_flower

#30: 1808248.30.config-import-enable.test-only.patch queued for re-testing.

Here's an updated patch. It follows more closely with "Scenario 2 - how I think it should work" in #31.
I've overridden the module handler to provide a config one, which is available during a config import. This allows us to bypass the config_install_default_config() call during a module enable, instead importing from the full config tree for the module that is being enabled.

To test this patch, what I was doing was getting a config export from a standard profile install, then doing a minimal installation, and trying to do a config import to take us from minimal > standard.

I've included an additional test that does just this, which should fail. This patch does the config import and manages to take us nearly all the way from minimal > standard, but there are some outstanding issues.

Post save hooks are a bit of an issue. Couple of examples -

Custom_block module. It creates a field by doing custom_block_add_body_field(). if this field is coming in later on in the config import (from another config file) it will break.
Contact module - references $this->original->id() (where original may not actually exist if we are doing a config import)

Also because we are calling theme_enable() this installs the default config (as config_install_default_config() is embedded in that function). What we would possibly need here is a themeHandler class (similar to the moduleHandler), where we could do a similar override (to the one i've implemented in the ConfigModuleHandler) when we are enabling a theme via a config import.

Since the node types conversion to CMI landed, here's an updated patch that includes those YAML files in the staging directory for the test minimal to standard config import.

This issue depends on #1969800: Add UUIDs to default configuration being committed.

Linking some more related issues here -

#1918926: Module dependencies are not respected when default configuration is imported
#2029771: Having installation profiles in system.module.yml causes config import to fail
#2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)
#2030073: Config cannot be imported in order for dependencies

Is there a reason this is postponed? It looks to me that disabled modules only come into play here via a few small lines of code in the handleModules() method.

Updated issue summary.

Allow me to express my doubts over the direct approach here. Shouldn't this work via some event? Of course it would require that modules CMI object get processed first -- as I expressed above, the CMI dependency is a problem.

@alexpott: are you working on this? Any feedback needed? :)

Patch has been rerolled.

Hate to be dropping this so late :-/, but shouldn't we defer the uninstalls to the end of the sync process ?
- install modules that need to be installed according to system.module.yml
- sync the config tree other than system.module.yml
- only then, uninstall modules that need to be uninstalled according to system.module.yml
?

For the same reason "enable first" is needed so that we can handle the creation of the config entities owned by the "new" modules, I'd think we want to "disable last" so as to keep "old" modules active until we're done dealing with their config entities ?

@yched good point! You're not late we're only just getting started with this issue :)

@alexpott suggested doing this after #2030073: Config cannot be imported in order for dependencies. See new parent issue for details. :)

Unpostponed!

Rerolled patch - we need to consider how to do this now that the Config Importer is batched. Patch attached just does all the module / theme installing and uninstalling in BatchConfigImporter::initialize() which is wrong.

So we need to ensure Extension storage has an up to date list of enabled modules and themes during regular default configuration import.

Fixed up some of the test fails.

settings.translation_sync issue will be resolved in #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

The views were being ordered inconsistently during import because the default configuration had inconsistent position values.

Fingers crossed this is green :)

@todo: implement sensible batching of extension install and uninstall during config import.

However the ConfigImportAll test passes!!!! This test installs the standard profile. Then installs all available modules. Exports the configuration to staging. Deletes all fields and then uninstalls all possible modules and then syncs the site using the exported config to restore everything..

This test installs the standard profile. Then installs all available modules. Exports the configuration to staging. Deletes all fields and then uninstalls all possible modules and then syncs the site using the exported config to restore everything..

Hm. No (new) test should rely on Standard profile.

cf. #1595028: Convert tests using Standard profile to use Testing profile instead

Any chance to convert that into DUTB + test the actual regression vectors instead?

I'm explicitly stating DUTB, because this test does not and should not care for anything else than installing, importing, and exporting configuration.

That said, it is not clear to me why such a test has to install all available configuration of all available modules in the first place... Tests should exclusively focus on possible test cases (possibly permutations) only. Installing a bunch of random modules that happen to have some random configuration does not cover any particular test case and only slows down the overall test suite performance. Those are the kind of tests that are guaranteed to get into your way, and everyone will avoid to run them at all costs ('cos a single test run takes ages to complete).

So, what are the actual test cases to cover?

@sun this explicitly tests that all non-required modules can be enabled through the config importer and that the default configuration created by a standard install can also by created reliably through the importer. If you do not want to rely on standard profile - what we could do is enable all the modules and then import the configuration from the standard profile. But in my mind this is still relying on the standard profile. Further more I'd like to add the converse test that all non-required modules and their configuration and config entities can be removed by the config importer.

CommentManager::addDefaultField needs to hide the comment field on any existing entity displays. Interdiff impossible so a real diff of the two patches attached.

Fixing the new test

rerolling this since, #2226823: Make default views displays configuration have correct "position" values got in.

This patch adds:

• sorting the modules in the correct order for install and uninstall
• changing the admin theme as well as the default - for the same reasons this is important - what happens if we're processing a batch and the admin theme is disabled?
• it makes the BatchConfigImporter and ConfigImporter use more of the same code for processing extensions
• it adds back the recalculation of the changelist after the extensions have been processed since processing extensions uses configuration from staging and not an extension's default config

Mind you this patch is very very ugly at this point :(

Patch attached implements a better way of dealing with default and admin theme changes after theme enables and (more importantly) before theme disables.

Refactored for #2228921: Consolidate system.module + system.theme + system.theme.disabled into core.extension config

Wow, this is pretty crazy...

Looked through it, found a bunch of nitpicks and some minor issues, but I don't have any glorious ideas to make it all much easier and nicer...

Nice test coverage!

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -131,13 +182,28 @@ public function getStorageComparer() {
    
   +  protected function getEmptyExtensionsProcessedList() {
   +    return array(
   

   insertMissingDocumentationNitpickHere()

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -187,8 +253,129 @@ protected function setProcessed($op, $name) {
   +   * @return void
   

   I don't think we do this.

3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -239,7 +429,8 @@ public function validate() {
          }
   -      $this->eventDispatcher->dispatch(ConfigEvents::IMPORT_VALIDATE, new ConfigImporterEvent($this));
   +      $importer_event = new ConfigImporterEvent($this);
   +      $this->eventDispatcher->dispatch(ConfigEvents::IMPORT_VALIDATE, $importer_event);
          $this->validated = TRUE;
        }
   

   Seems unrelated?

4. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -253,13 +444,58 @@ public function validate() {
   +      // Installing a module can cause a kernel boot therefore reinject all the
   +      // services.
   +      $this->reInjectMe();
   

   Aw.

5. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -341,4 +577,67 @@ public function alreadyImporting() {
   +   * @param $config_name
   +   *   The configuration object name.
   

   Missing type.

6. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -341,4 +577,67 @@ public function alreadyImporting() {
   +      // drupal_flush_all_caches();
   

   Left-over?

7. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -341,4 +577,67 @@ public function alreadyImporting() {
   +  }
   +
   +  protected function reInjectMe() {
   

   docs...

8. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -130,6 +144,18 @@ public function installDefaultConfig($type, $name) {
   +          // happening) will be unstable after the module has been enabled and
   +          // before the config entity has been imported.
   +          if ($this->isSyncing) {
   +            continue;
   +          }
   
   @@ -138,6 +164,9 @@ public function installDefaultConfig($type, $name) {
                $id = $entity_storage->getIDFromConfigName($name, $entity_storage->getEntityType()->getConfigPrefix());
                $entity = $entity_storage->load($id);
   +            if ($this->isSyncing) {
   +              $entity->setSyncing(TRUE);
   +            }
   

   Those two checks seem the conflict, the second one will never run then?

9. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -159,4 +188,48 @@ public function installDefaultConfig($type, $name) {
   +      // If using the the extension install storage class can not
   

   Cut off comment.

10. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportAllTest.php
    @@ -0,0 +1,109 @@
    +    // Delete every field on the site so all modules can be disabled. For
    

    uninstalled ;)

    Also, I *thought* we fixed this so that fields are uninstalled correctly, but it's possibly that there are some left-overs...

11. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportAllTest.php
    @@ -0,0 +1,109 @@
    +    \Drupal::moduleHandler()->uninstall(array_keys($all_modules));
    

    All modules aren't actually all modules anymore at this point, maybe use a different variable?

12. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -65,16 +69,60 @@ function testImport() {
    +    // during the install. Options and Text are there in ensure modules are
    +    // installed with the correct dependencies. Options depends on Text so Text
    +    // should be installed first.
    

    first sentence seems to be missing some words or so ;) Also, those two are now already enabled for the test? and are not in the list?

13. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -65,16 +69,60 @@ function testImport() {
    +    // Disable the Options and Text modules to ensure that dependencies are handled
    +    // correctly.
    +    \Drupal::moduleHandler()->uninstall(array('text', 'options'));
    

    Ah, here we go ;) Still, the comment above seems outdated.

14. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -88,6 +136,85 @@ function testImport() {
     
         // Verify the cache got cleared.
         $this->assertTrue(isset($GLOBALS['hook_cache_flush']));
    +
    

    Not related, but pretty sure that this assertion is completely bogus. this happens inside the page request, it doesn't affect our global? It works because we do a dfac() during test setup I guess, should also be green when checking before you import ;)

15. +++ b/core/modules/config/tests/config_import_test/lib/Drupal/config_import_test/EventSubscriber.php
    @@ -0,0 +1,101 @@
    +use Drupal\Core\KeyValueStore\StateInterface;
    +use Symfony\Component\EventDispatcher\EventSubscriberInterface;
    +
    +
    +/**
    

    two spaces :p

16. +++ b/core/modules/config/tests/config_import_test/lib/Drupal/config_import_test/EventSubscriber.php
    @@ -0,0 +1,101 @@
    +  public function onConfigSave(ConfigCrudEvent $event) {
    

    needs some inheritdocs on this and other methods.

17. +++ b/core/modules/config/tests/config_import_test/lib/Drupal/config_import_test/EventSubscriber.php
    @@ -0,0 +1,101 @@
    +    return $events;
    +  }
    +
    +}
    \ No newline at end of file
    

    .

18. +++ b/core/modules/content_translation/content_translation.install
    @@ -87,6 +87,19 @@ function content_translation_install() {
    +  $config_names = \Drupal::configFactory()->listAll('field.field.');
    +  foreach ($config_names as $name) {
    +    \Drupal::config($name)
    +      ->set('settings.translation_sync', FALSE)
    +      ->save();
    +  }
    +  $config_names = \Drupal::configFactory()->listAll('field.instance.');
    +  foreach ($config_names as $name) {
    +    \Drupal::config($name)
    +      ->set('settings.translation_sync', FALSE)
    +      ->save();
    

    I guess this is a workaround until that is in it's own files?

19. +++ b/core/modules/field/lib/Drupal/field/FieldInstanceConfigInterface.php
    index af5ebf2..1c7d8a4 100644
    --- a/core/modules/forum/config/field.field.forum.forum_container.yml
    
    --- a/core/modules/forum/config/field.field.forum.forum_container.yml
    +++ b/core/modules/forum/config/field.field.forum.forum_container.yml
    
    +++ b/core/modules/forum/config/field.field.forum.forum_container.yml
    +++ b/core/modules/forum/config/field.field.forum.forum_container.yml
    @@ -1,4 +1,4 @@
    
    @@ -1,4 +1,4 @@
    -id: taxonomy_term.forum_container
    +id: forum.forum_container
    

    The fix should be the other way round? entity_type.fieldname, and taxonomy_term is the entity type, not forum..

20. +++ b/core/modules/forum/forum.install
    @@ -16,72 +16,75 @@ function forum_install() {
    +  if (!\Drupal::service('config.installer')->isSyncing()) {
    +    // Create the 'taxonomy_forums' field if it doesn't already exist. If forum
    +    // is being enabled at the same time as taxonomy after both modules have been
    +    // enabled, the field might exist but still be marked inactive.
    

    All those checks and edge cases are becoming more and more complicated :(

21. +++ b/core/modules/node/node.install
    @@ -459,7 +459,9 @@ function node_uninstall() {
       $types = \Drupal::configFactory()->listAll('node.type.');
       foreach ($types as $config_name) {
         $type = \Drupal::config($config_name)->get('type');
    -    \Drupal::config('language.settings')->clear('node. ' . $type . '.language.default_configuration')->save();
    +    if (\Drupal::moduleHandler()->moduleExists('language')) {
    +      \Drupal::config('language.settings')->clear('node. ' . $type . '.language.default_configuration')->save();
    +    }
       }
    

    Unrelated but this could use hook_entity_bundle_delete() in language.module now, which is triggered by entity_module_preuninstall().

    There's also talk of moving those settings into the language field settings.

Thanks @Berdir.

1 - 17. All fixed
18. Yep :(
19. Yes... nice catch
20. Indeed
21. Agreed but out of scope :)

How about breaking the batch up into truly separate steps for extensions and configuration - the attached patch does this and I think it is easier to understand.

Been testing this patch by doing the following:

1. Apply the latest patch from #2232275: System menu blocks need to be able to depend on their menu entities
2. Install the standard profile
3. Copy the active config directory somewhere safe
4. Edit the copied version of core.extension.yml to use the minimal profile instead of standard
5. Destroy the site
6. Install the minimal profile
7. Copy the saved standard config to staging
8. Copy system.site.yml from active to staging
9. Run the import http://www.screencast.com/t/NXI1Xu5VQ3.

Fixed an interesting issue with recreates going in in the wrong order due to being added after the creates and deletes.

This looks great! Reviewed the patch/code (did not apply and/or test it, but #118 is more than sufficient proof for me :-))

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -90,6 +106,27 @@ class ConfigImporter extends DependencySerialization {
   +   * Flag set to import system.theme during processing theme enable and disables.
   

   s/system.theme/core.extension/ ?

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -131,13 +175,34 @@ public function getStorageComparer() {
      public function reset() {
   

   Shouldn't this also reset $processedSystemTheme?

3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -187,8 +252,139 @@ protected function setProcessed($op, $name) {
   +  public function getUnprocessedConfiguration($op) {
   

   In reviewing this patch, I found all the "Unprocessed" method + variable names very confusing — they sound as if they're about an unprocessed data/domain object.

   Would be great to rename all of them into "Pending" (or similar).

   Briefly discussed with @alexpott in IRC... let's do that in a follow-up issue.

4. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -208,19 +406,20 @@ public function import() {
   +      // Where to put this?
   +      $this->handleExtensions();
   

   Is this @todo still an open question?

5. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -253,13 +452,58 @@ public function validate() {
   +      // Installing a module can cause a kernel boot therefore reinject all the
   +      // services.
   +      $this->reInjectMe();
   +      // During the container is rebuilt and the module handler is called from
   +      // drupal_get_complete_schema(). This causes the container's instance of
   +      // the module handler not to have loaded.
   +      $this->moduleHandler->loadAll();
   

   Oh wow.

   Since the workarounds appear to work for now, let's create separate follow-up issues to investigate whether we can resolve this cleaner/differently?

6. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -130,6 +144,18 @@ public function installDefaultConfig($type, $name) {
   +          // If we are syncing do not create configuration entities. Pluggable
   +          // configuration entities can have dependencies on modules that are
   +          // not yet enabled. In the absence of dependency management for config
   +          // entities this is a good as we can do. The problem with this
   +          // approach is that any code that expects default configuration
   +          // entities to exist (even if there is code the prevents this from
   +          // happening) will be unstable after the module has been enabled and
   +          // before the config entity has been imported.
   +          if ($this->isSyncing) {
   +            continue;
   +          }
   

   This comment (at least the parts about an absence of dependencies) sounds obsolete, no?

7. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -118,11 +118,16 @@ public function getChangelist($op = NULL) {
   +  protected function addChangeList($op, array $changes, array $sort_order = NULL) {
   ...
   +    if (!empty($sort_order)) {
   

   There's a small mismatch here:

   If it's legit to pass an empty array for $sort_order, then the condition should be isset().

   Otherwise, $sort_order should probably default to an empty array.

8. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -582,6 +582,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +    /** @var \Drupal\Core\Config\ConfigInstaller $config_installer */
   +    $config_installer = \Drupal::service('config.installer');
   +    $sync_status = $config_installer->isSyncing();
   +    if ($sync_status) {
   +      $source_storage = $config_installer->getSourceStorage();
   +    }
   
   @@ -671,6 +677,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        $config_installer = \Drupal::service('config.installer');
   +        if ($sync_status) {
   +          $config_installer
   +            ->setSyncing(TRUE)
   +            ->setSourceStorage($source_storage);
   +        }
   +        else {
   +          // If we're not in a config synchronisation reset the source storage
   +          // so that the extension install storage will pick up the new
   +          // configuration.
   +          $config_installer->resetSourceStorage();
   +        }
   

   Hm. Doesn't the ConfigInstaller / ConfigImporter handle this already?

   Also, wondering why we're not simply tagging config.installer with 'persist' instead?

   Wouldn't that resolve all of the manual futzing with re-injecting the previous state into the config installer/importer...?

   Wasn't this in-process rebuild from ModuleHandler the exact reason for why we introduced the 'persist' tag for services?

9. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -146,6 +146,11 @@ public function enable(array $theme_list) {
   +      // If we're not in a config synchronisation reset the source storage so
   +      // that the extension install storage will pick up the new configuration.
   +      if (!$this->configInstaller->isSyncing()) {
   +        $this->configInstaller->resetSourceStorage();
   +      }
   

   Oh wow. I wonder why I didn't run into this problem in #1067408: Themes do not have an installation status ...?

   At least I assume that comment to actually try to say:

   "ExtensionInstallStorage only knows about the currently enabled list of themes, so it has to be reset in order to pick up the default config of the newly installed theme. However, do not reset the source storage when synchronizing configuration, since that would needlessly trigger a reload of the whole configuration to be imported."

   No? Wanna take over that as a replacement comment? :-)

10. +++ b/core/modules/config/tests/config_import_test/config_import_test.services.yml
    @@ -0,0 +1,6 @@
    \ No newline at end of file
    

    minor

Thanks @sun

1. Nope this is about change the default and admin themes at the right time - which, if we are disabling themes, is right before we do the disabling
2. Nice catch
3. Sure
4. Nope
5. Yeah this is difficult
6. Improved the comment
7. Fixed - improved doc and got a bit defensive as we do not want sorting to mangle the data
8. Please no persist :) it is doing this because it has to ensure that a kernel reboot and container rebuild does not change this. Persisting the config installer would mean that all the services it depends have to be persisted to - this is why I think it is a terrible idea.
9. Fixed
10. Fixed

Fixed the test fails. These where due the sorting in StorageComparer::addChangeList() making the array keys not start from 0.

OK... I don't really agree with the approach taken just to avoid 'persist', but I guess we should simply move forward with this.

+++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -119,14 +121,21 @@
+        throw \InvalidArgumentException(String::format('Sorting the @op changelist should not change its length.', array('@op' => $op)));

throw *new* ? :)

Doh!

So the other reason for not using persistent services is that it means that a module can not ever swap out the service. We'll just copy the old one over.

Thanks! — Yeah, OK, let's move forward here.

The topic of persist + kernel rebuilds is a super complex topic of its own and definitely shouldn't hold up this patch.

Improved batch progress message.

See http://www.screencast.com/t/NXI1Xu5VQ3 for incorrect message.

I didn't do an in depth review of the latest patches, but +1 from my side, can't spot anything that is wrong and if sun and beejebus are OK with it then I am too :)

Discussed some of the issues like the order of install/uninstall with Alex last week and have no issues.

Overall this looks great. I found some very minor nitpicks that we could handle in a follow-up, posting here rather than ignoring though. The container syncing issue is horrible but not at all the fault of this patch, we've got other issues open trying to figure that out.

1. +++ b/core/lib/Drupal/Core/Config/BatchConfigImporter.php
   @@ -37,49 +75,99 @@ public function initialize() {
   +  public function processConfigurationBatch(array &$context) {
   

   Missing docs.

2. +++ b/core/lib/Drupal/Core/Config/BatchConfigImporter.php
   @@ -37,49 +75,99 @@ public function initialize() {
   +  public function finishBatch(array &$context) {
   

   Also here.

3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -253,13 +453,58 @@ public function validate() {
   +      // During the container is rebuilt and the module handler is called from
   

   During a container rebuild the module handler is called? Couldn't parse this. This should be fixed by #2194785: [meta] Stop relying on database schema info at runtime so could add a @todo.

If I have time for another read through, I'll commit this a bit later today, shouldn't stop another maintainer committing it if they get back here first though :)

Fixed nits from #127

Committed/pushed to 8.x, thanks!

This issue seems to be a potential source for "random" failures: https://qa.drupal.org/pifr/test/764353 is one example but I have seen that elsewhere.
I wonder whether we could file a followup which don't enables all the modules at once. On top of that it would be great to exclude contrib modules in sites/*/modules or modules/

I'm going to open the follow-up for #98 "@todo: implement sensible batching of extension install and uninstall during config import." right now.
just putting the structure in place to track them in the issue summary.

--
Also, quickly noting re #131, #2233929: drupal_set_time_limit should not be able to change the time limit if it's already unlimited can deal with it (but the #98 follow-up might actually deal with the cause).

#2234159: Fix config importing through the UI and change ConfigImportAll test using the UI is that followup

that was already a child. not sure if it needs to be related also. but these new fields are still new... not sure how best to use them.

updating the issue summary to note they are follow-ups though.

Also seeing the random fail, opened #2240709: ConfigImportUITest::testImport fails when the module list changes

shouldn't we defer the uninstalls to the end of the sync process?

Was this implemented? If not, why not? If so, how? Looking at the code, I only see:

if ($this->totalExtensionsToProcess > 0) {
  $sync_steps[] = 'processExtensions';
}
$sync_steps[] = 'processConfigurations';

which from what I can tell, does installs and uninstalls in the same place, and before configs.

I'm curious, because #2198429: Make deleted fields work with config synch just landed, which front loads the purging to the beginning of the process, rather than between processConfigurations() and (the nonexistent) processUninstalls().

No config uninstalls are not deferred. This is because they could affect config creation - that's why uninstalls are done first before installs. The other thing you is the final environment the config export came from - it is important to get to the same code base as soon as possible. I'm going to open an issue to add documentation to the ConfigImporter class about priority and ordering of everything.

it is important to get to the same code base as soon as possible

That makes sense. Thanks.

Created #2258247: Remove useless Drupal\config\Tests\ConfigImportAllTest