Problem/Motivation

Currently default_content_modules_installed() only runs the default content import only if a module is enabled outside of a configuration sync.
This means that an approach like Configuration Installer, which allows you to install a fresh site based on existing config, doesn't work. Or installing a module via a configuration deployment and drush config-import or the Config UI.

Proposed resolution

Add an additional condition to allow the default content import if the config sync occurs during the drupal installation. We can use drupal_installation_attempted() to achieve this.

Remaining tasks

Reviews needed.

User interface changes

None

API changes

None

Comments

das-peter created an issue. See original summary.

das-peter’s picture

Don't mind me just updating the embarrassingly wrong condition in the last patch.
Lets see how many times I can get the condition wrong... :P

hatznie’s picture

Status: Needs review » Needs 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.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.39 KB

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.

andypost’s picture

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?

tstoeckler’s picture

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.

andypost’s picture

Issue tags: +Needs tests

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

tstoeckler’s picture

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.

moshe weitzman’s picture

andypost’s picture

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

tstoeckler’s picture

Status: Needs work » Needs review

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.

andypost’s picture

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

andypost’s picture

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

larowlan’s picture

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

tstoeckler’s picture

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.

andypost’s picture

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

alexpott’s picture

Category: Feature request » Bug report

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.

alexpott’s picture

Here's a reroll.

Status: Needs review » Needs work

The last submitted patch, 18: 2833562-18.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.78 KB
4.53 KB

Added a test and fixed the patch.

alexpott’s picture

Title: Compatibility with Configuration Installer » Installing modules using the ConfigImporter does not work
Issue summary: View changes
tstoeckler’s picture

Wow, that test looks sweet, thanks!

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

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?

larowlan’s picture

alexpott’s picture

Small refactor of the tests and fixing #23.

larowlan’s picture

Assigned: Unassigned » andypost

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

  • andypost committed 9d068fa on 8.x-1.x authored by alexpott
    Issue #2833562 by alexpott, das-peter, tstoeckler, andypost, larowlan,...
andypost’s picture

Assigned: andypost » Unassigned
Status: Reviewed & tested by the community » Fixed
Related issues: +#2867579: Core 8.3 compatibility, +#2849128: Add handling of ConfigEvent::IMPORT_MISSING_CONTENT

@alexpott Thanx for tests clean-up! Nice refactoring

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

Status: Fixed » Closed (fixed)

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