Installing modules using the ConfigImporter does not work

I've tried the dev version with this patch and the config_install profile but it doesn't seem to work. When the install is done no default content was added. When I uninstall my default content module and reinstall it then the content is present.

So ignoring the sync state is problematic even during installation as the default content that is being imported might depend on configuration (entities) to exist, which is not the case at the time hook_modules_installed() is called during configuration syncing.

I hit this with using files as default content with File Entity installed, as the file entities cannot be imported when the respective file entity bundles have not yet been imported.

What we can do instead is subscribe to the config import process and check if any modules were installed and explicitly call those then at that time. This worked for me locally.

Interesting idea, but how to get difference in config sync called from module and with config installer?
I guess a lot of contrib using sync and running import on each sync needs some kill switch

BTW why there's no priority for subscriber?

Note that this will only import default content from modules that were explicitly installed as part of the configuration synchronization. Those are precisely those that default_content_modules_installed() will ignore. If a module triggers a configuration synchronization this will only trigger if additional modules are installed as part of that synchronization as well, in which case you want it to trigger because - again - default_content_modules_installed() will skip those additional modules.

Maybe I'm missing something but I don't quite understand what problems you are anticipating maybe you can explain in a bit more detail.

No priority because there really is no need for one. I think we generally only add priorities when we explicitly want to run before or after another subscriber.

My main concerns is that I does not understand all details of config sync and it's strange that modules can be enabled in that process but hooks are not called. That looks very strange to me.

That looks more like core bug and missing hook_modules_installed, hook_modules_uninstalled and hook_module_preuninstall may have side effects

@tstoeckler please point me where to check that behavior looks nothing in ConfigInstaller

So the hook are called, but in default_content_modules_uninstalled() we explicitly bail out if we are in the configuration synchronization process. So there is so core bug, as far as I can tell. Modules can be installed as part of configuration synchronization if the core.extension.yml changed. It is important (as you correctly point out yourself) that core core.extension.yml not just the values are saved into configuration but that the respective modules are properly installed or uninstalled including all hooks, etc.

So the first question is: Why do we not import content during configuration synchronization?
If you look at ConfigInstaller starting at line 296:

      if (... && $entity_type = $this->configManager->getEntityTypeIdByName($name)) {
        // If we are syncing do not create configuration entities. Pluggable
        // configuration entities can have dependencies on modules that are
        // not yet enabled. This approach means that any code that expects
        // default configuration entities to exist will be unstable after the
        // module has been enabled and before the config entity has been
        // imported.
        if ($this->isSyncing()) {
          continue;
        }
...
          $entity->trustData()->save();

(I have removed some parts to emphasize the important parts.) This means that the configuration entities that a module ships in its config/install are not installed when the module is installed as part of a configuration synchronization. Instead, those configuration entities are expected to be part of the configuration set, so that they will be installed later during the synchronization. This means, however, that at the time of hook_install() and hook_modules_installed() you can not depend on configuration entities existing - at least not if \Drupal::isConfigSyncing() returns TRUE. And because content that we create with default content might depend on certain config entities to exist (primarily as bundle entities), we thus cannot import content of modules that are enabled during config synchronization.

The followup question then is: When can we import content of modules that are enabled during config synchronization?
Having said the above, the answer is simple: As soon as all configuration entities are guaranteed to exist, in other words as soon as the configuration synchronization is complete. Luckily the configuration system provides an event for this (see ConfigImporter::finish()) which the patch utilizes.

Hope that makes it clear.

Regarding tests, yeah I guess you're right, that it would be nice to test this as it's somewhat hard to wrap your head around and could easily be broken again in the future.

Last comment makes me think of #2361423: Add a step to config import to allow contrib to create content based on config dependencies

NW for #9

It looks that core has special event \Drupal\Core\Config\ConfigEvents::IMPORT_MISSING_CONTENT

I think we should keep both hook_modules_installed() and sync event but that should not break related work

So ConfigEvent::IMPORT_MISSING_CONTENT is for content that config depends on via:

dependencies:
  content:
    - node:article:6ba7b810-9dad-11d1-80b4-00c04fd430c8

This issue is about content that depends on configuration. I.e. if you have a news.module that ships a node.type.news node type and some default news content with $node->bundle() === 'news' then you need the news bundle to exist before importing the content.

I think we should add support for ConfigEvent::IMPORT_MISSING_CONTENT as well, but that's a little bit harder to solve, as we cannot import specific content (i.e. content with specific UUIDs) at the moment. Also we will then have to consider config that depends on content that in turn depends on config. I.e. with the example above, you have a view that shows a specific news item in its header. So the view requires the news item to exist, but the news item requires the news node type to exist.

Marking back to needs review for now, looking forward to your thoughts.

I tested the patch and it works with config installer but

+++ b/src/Config/DefaultContentConfigSubscriber.php
@@ -0,0 +1,52 @@
+    $modules = $event->getConfigImporter()->getExtensionChangelist('module', 'install');
+    foreach ($modules as $module) {
+      $this->defaultContentManager->importContent($module);

this fails on second config sync because content, already imported
To solve that #2698425: Do not reimport existing entities should be commited first

Support for ConfigEvent::IMPORT_MISSING_CONTENT is separate issue

I filed #2849128: Add handling of ConfigEvent::IMPORT_MISSING_CONTENT
Going to test it properly on custom project and add tests

Agree that #2698425: Do not reimport existing entities should come first, but happy with the patch above.

Re #12 / #14: Actually no, because the module actually is only installed once, so this will only be called once, unless the module is subsequently uninstalled and reinstalled. But this does not make the current situation any worse, IMO.
Don't mind waiting for #2698425: Do not reimport existing entities either, though, just wanted to clarify.

+++ b/src/Config/DefaultContentConfigSubscriber.php
@@ -0,0 +1,52 @@
+    $this->defaultContentManager = $default_content_manager;
...
+      $this->defaultContentManager->importContent($module);

importer after #2614644: Split DefaultContentManager into Exporter and Importer

This is not just about the Configuration Installer - it's about an install of a module via config import - even via the UI. And as such is not a feature but a bug. This module is supposed to guarantee the default content is create on module install - atm it doesn't. Nice fix @tstoeckler

#2698425: Do not reimport existing entities and #2849128: Add handling of ConfigEvent::IMPORT_MISSING_CONTENT also look sensible. I'm not sure about waiting on #2698425: Do not reimport existing entities as this does not exacerbate the existing problem.

Here's a reroll.

Added a test and fixed the patch.

Wow, that test looks sweet, thanks!

Looks good, a couple of minor nits that need not hold this up or be fixed on commit.

1. +++ b/src/Config/DefaultContentConfigSubscriber.php
   @@ -0,0 +1,53 @@
   +use Drupal\default_content\DefaultContentManagerInterface;
   

   unused?

2. +++ b/tests/src/Functional/DefaultContentTest.php
   @@ -51,4 +53,37 @@ class DefaultContentTest extends BrowserTestBase {
   +    $this->drupalLogin($this->drupalCreateUser(array_keys(\Drupal::moduleHandler()->invokeAll(('permission')))));
   

   Could use the 'admin role' argument instead?

Small refactor of the tests and fixing #23.

Thanks, looks good to go, will check in with @andypost with regards to commit order.

@alexpott Thanx for tests clean-up! Nice refactoring

After #2867579: Core 8.3 compatibility next alpha can be released