Problem/Motivation

Both #1613424: Allow a site to be installed from existing configuration and #1808248: Add a separate module install/uninstall step to the config import process require configuration to install from the staging directory during module installation instead of from a module's config directory.

Proposed resolution

The easiest way to achieve this will be to refactor the config_install_default_config() into a ConfigInstaller service to handle the task. The refactor does not make the source storage injectable or settable as the two issues above should explore how best to do this. It just makes it possible.

The procedural function is removed because it was added in the 8.x cycle so deprecating makes little sense.

It does not handle injection into the module handler as this class currently makes heavy use of \Drupal and doing this in this refactor is likely to blow up in very interesting ways.

Remaining tasks

Review

User interface changes

None

API changes

  • Replace config_install_default_config() with ConfigInstaller::installDefaultConfig()
Files: 
CommentFileSizeAuthor
#32 2166065.32.patch31.44 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,558 pass(es). View
#32 31-32-interdiff.txt684 bytesalexpott
#31 2166065.31.patch30.77 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#31 30-31-interdiff.txt3.75 KBalexpott
#22 2166065.22.patch32.04 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,449 pass(es). View
#22 20-22-interdiff.txt1.99 KBalexpott
#20 2166065.20.patch32.03 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 59,414 pass(es). View
#20 interdiff-12-20.txt1.55 KBmtift
#12 2166065.12.patch31.9 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 59,390 pass(es). View
#12 interdiff.txt1.85 KBmtift
#10 2166065.10.patch31.9 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,535 pass(es). View
#10 8-10-interdiff.txt5 KBalexpott
#8 2166065.8.patch29.05 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,502 pass(es), 11 fail(s), and 1 exception(s). View
#8 5-8-interdiff.txt1.61 KBalexpott
#5 2166065.5.patch27.61 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,328 pass(es), 17 fail(s), and 5 exception(s). View
#5 1-5-interdiff.txt2.46 KBalexpott
#1 2166065.1.patch29.18 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,388 pass(es), 16 fail(s), and 5 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs review
Issue tags: +beta blocker
Parent issue: » #2121751: [META] Making configuration synchronisation work
Related issues: +#1613424: Allow a site to be installed from existing configuration, +#1808248: Add a separate module install/uninstall step to the config import process
FileSize
29.18 KB
FAILED: [[SimpleTest]]: [MySQL] 59,388 pass(es), 16 fail(s), and 5 exception(s). View

This patch is critical since it is part of #2121751: [META] Making configuration synchronisation work

alexpott’s picture

Issue summary: View changes
yched’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -0,0 +1,155 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function uninstallDefaultConfig($type, $name){
+    $config_names = $this->activeStorage->listAll($name . '.');
+    foreach ($config_names as $config_name) {
+      $this->configFactory->get($config_name)->delete();
+    }
+  }

This removes the config owned by the module that gets uninstalled (i.e removes all views when Views is uninstalled), thus has actually nothing to do with "default config" ?

At least the method name / phpdoc is misleading, but does it really belong in the same class / interface ?

alexpott’s picture

@yched hmmm... I'm okay with removing the removal of config_uninstall_default_config() from this patch - I just felt the two belonged together. So we have two options:

  • rename to uninstallConfig()
  • remove this from the patch

I guess that having a service called ConfigInstaller handle uninstallation is pretty weird. And config uninstallation has it's own set of issues re dependencies so I'm going to remove that part of the change from the patch.

alexpott’s picture

Issue summary: View changes
FileSize
2.46 KB
27.61 KB
FAILED: [[SimpleTest]]: [MySQL] 59,328 pass(es), 17 fail(s), and 5 exception(s). View

Removing the changes to config_uninstall_default_config()

The last submitted patch, 1: 2166065.1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2166065.5.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
29.05 KB
FAILED: [[SimpleTest]]: [MySQL] 59,502 pass(es), 11 fail(s), and 1 exception(s). View

Another change this patch makes is to use the configuration system to get the list of enabled modules. The current procedural function uses the ModuleHandler()->getModuleList(). The previous patches were failing DrupalUnitTestBase (DUBT) tests since this environment does not maintain a system.module file when enabling modules.

The reason this patch changes this is that we're in a classic circular dependency. If we ever get round to injecting the dependencies into the ModuleHandler then we'll be injecting the ConfigInstaller therefore we should not make the ConfigInstaller require the ModuleHandler to be injected. One could argue that we should separate the installation tasks from the ModuleHandler and I'd agree but this kind of refactor will turn out to be massive and out of scope for this issue. The assumption that system.module contains a current list of enabled modules if the configuration system is available and writing to disk seems sound to me. We already do something similar for system.theme in DUBT.

Patch attached fixes DUBT to write the system.module configuration file.

Status: Needs review » Needs work

The last submitted patch, 8: 2166065.8.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5 KB
31.9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,535 pass(es). View

Fixed the test failures. Using the config factory so early in DrupalUnitTestBase proved problematic as this causes the event dispatcher to be instantiated without the events of modules that are about to be enabled!

Also I had to fix Drupal\field\Tests\FieldInfoTest::testInstanceDisabledEntityType() as it was not actually testing anything due to a missing save of the field instance. And module_uninstall totally relies on the system.module configuration file existing and having the correct list of modules - which it did not in a DUBT environment till now.

jibran’s picture

Yay!! we have a new service can we please have new unit tests as well.

+++ b/core/lib/Drupal/Core/Config/ConfigInstallerInterface.php
@@ -0,0 +1,40 @@
+   * When the Views module is installed after the Node module is already enabled,

more then 80 chars.

mtift’s picture

FileSize
1.85 KB
31.9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,390 pass(es). View

This looks good to me. It removes all instances of config_install_default_config(). Installing Drupal/modules still works fine with the patch applied. The attached patch just fixes a couple very minor spacing and language issues.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -0,0 +1,146 @@
+        if ($entity_type = config_get_entity_type_by_name($name)) {

@jibran adding a PHPUnit test is icky whilst we have this :) Although I guess we do the thing of defining the function in the test.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -0,0 +1,146 @@
+    $config_dir = drupal_get_path($type, $name) . '/config';

This also makes PHPUnit testing unnecessarily difficult.

I would like to move on with this patch so I can continue at least with #1808248: Add a separate module install/uninstall step to the config import process. Config installation has many many tests - nearly every test installs a module :) so we know the functionality here is well covered.

jibran’s picture

drupal_get_path we can define this function in local namespace but I agree that we don't need phpunit to test functionality.
As it is a CT(critical task) let's fix this asap and I think we can explore the option of adding phpunit test in follow up.
Because what I have seen in core till now that we should have Interface for almost all services and same for phpunit.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch and it seems to encapsulate the config installer code properly in the new service. Yay for proper service dependencies also. The resulting changes elsewhere are simple. Looks good to me.

larowlan’s picture

+ /**
+ * Constructs the configuration installer.
+ *
+ * @param ConfigFactory $config_factory
+ * The configuration factory.
+ * @param StorageInterface $active_storage
+ * The active configuration storage.
+ * @param TypedConfigManagerInterfa$typed_config
+ * The typed configuration manager.
+ * @param EntityManagerInterface $entity_manager
+ * The entity manager.
+ * @param EventDispatcherInterface $event_dispatcher
+ * The event dispatcher.
+ * @param UuidInterface $uuid_service
+ * The UUID generation

Shouldn't these use the fully qualified classes?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
mtift’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,399 pass(es). View

You are correct, sir

mtift’s picture

FileSize
1.55 KB
32.03 KB
PASSED: [[SimpleTest]]: [MySQL] 59,414 pass(es). View

Ooops, wrong patch. Please ignore #19.

sun’s picture

  1. +++ b/core/core.services.yml
    @@ -190,7 +193,7 @@ services:
       theme_handler:
    ...
    +    arguments: ['@config.factory', '@module_handler', '@cache.cache', '@info_parser', '@config.installer', '@router.builder']
    
    +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -633,7 +633,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +        \Drupal::service('config.installer')->installDefaultConfig('module', $module);
    

    theme_handler was adjusted to inject the dependency, but why wasn't module_handler adjusted accordingly as well?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -0,0 +1,146 @@
    +      $other_module_config = array_filter($default_storage->listAll(), function ($value) use ($name) {
    +        return !preg_match('/^' . $name . '\./', $value);
    +      });
    

    Any particular reason for using an expensive preg_match() instead of a simple strstr() or strtok() or just simply strpos() === 0 here?

    When sticking with the preg_match(), $name has to be preg_quote($name, '/').

  3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -0,0 +1,146 @@
    +      $other_module_config = array_filter($other_module_config, function ($config_name) use ($enabled_extensions) {
    +        $provider = Unicode::substr($config_name, 0, strpos($config_name, '.'));
    +        return in_array($provider, $enabled_extensions);
    +      });
    

    Valid extension names are defined by DRUPAL_PHP_FUNCTION_PATTERN. Given the subsequent in_array(), Unicode::substr() can be replaced with a simple substr()?

  4. +++ b/core/lib/Drupal/Core/Config/ConfigInstallerInterface.php
    @@ -0,0 +1,40 @@
    +   * When an extension is installed, it searches all the default configuration
    ...
    +   * Additionally, the default configuration directory for the extension being
    ...
    +  public function installDefaultConfig($type, $name);
    

    The existing/moved phpDoc documentation on its own clarifies that we're talking about two discrete and independent operations, no?

    First, install the module's own configuration.

    Second, install any possibly existing integration configuration provided by other modules.

    function install($type, $name) {
      $this->installOwn($type, $name);
      $this->installIntegration($type, $name);
    }
    

    Thoughts?

  5. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -95,16 +103,21 @@ class ThemeHandler implements ThemeHandlerInterface {
    +   * @param \Drupal\Core\Config\ConfigInstallerInterface $config_installer
    +   *   (optional) The config installer to install configuration. This optional
    +   *   to allow the theme handler to work before Drupal is installed and has a
    +   *   database.
    

    Hm.

    1. I'm not sure I see the point of making the dependency optional, if we can easily provide it in the minimal container being setup in install_begin_request()?

    2. If we really want to keep it optional, then I think the service definition should specify @?theme_handler, no?

    3. More fundamentally:

    While ThemeHandler does contain enable() and disable() methods, the corresponding ModuleHandler does not have install() and uninstall() methods.

    In the original patch that introduced ModuleHandler, we (sorta intentionally) did not include those methods, because the list of enabled extensions defines the base data and behavior of the *Handler. All other *Handler methods are operating on an sorta-injected list of available/enabled extensions.

    Yes, I agree and realize that this patch only advances on the existing ThemeHandler code, which happens to contain enable()/disable() already.

    However, I'm 90% sure that this breaks a clean separation of concerns and will only lead to further circular dependencies.

    I'm fairly sure that we need a:

    class Drupal\Core\Extension\Installer\ExtensionInstaller

    possibly, but ideally not, with sub-classes for extension types:

    class Drupal\Core\Extension\Installer\ModuleInstaller

  6. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.php
    @@ -33,6 +33,9 @@ public static function getInfo() {
       public function setUp() {
         parent::setUp();
         $this->installSchema('system', 'config_snapshot');
    +    // Update the config snapshot. This allows the parent::setUp() to write
    +    // configuration files.
    +    config_import_create_snapshot(\Drupal::service('config.storage'), \Drupal::service('config.storage.snapshot'));
       }
    

    Hm. Is this a hidden @todo, or is the parent::setUp() incomplete, or can we rewrite the comment to clarify why it is necessary now whereas it previously wasn't?

  7. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterUnitTest.php
    @@ -37,7 +37,7 @@ public static function getInfo() {
    -    config_install_default_config('module', 'system');
    +    $this->installConfig(array('system'));
    

    Unless there is one already (I vaguely remember to have raised the concern a year ago already), we need a follow-up issue to change DUTB::installConfig() to accept a $type as first argument, too.

  8. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -102,6 +102,13 @@ protected function setUp() {
    +    // Create a minimal system.module configuration file so that the list of
    +    // enabled modules can be maintained allowing
    +    // \Drupal\Core\Config\ConfigInstaller::installDefaultConfig() to work. Not
    +    // using configuration factory ensures that events registered by modules
    +    // during self::enableModules are registered.
    +    \Drupal::service('config.cachedstorage.storage')->write('system.module', array('enabled' => array()));
    
    @@ -268,9 +275,16 @@ protected function enableModules(array $modules) {
    +    // Write directly to active storage to avoid early instantiation of
    +    // services which can prevent modules from registering events.
    +    $active_storage =  \Drupal::service('config.cachedstorage.storage');
    

    I did not understand this comment. :-/

    1. DUTB tests are operating with memory backend implementations anyway.

    2. Somehow, the comment in setUp() seems to contradict the comment in enableModules()?

    3. At minimum, shouldn't this use $this->container?

    Lastly: eek @ the service name — a cached config storage appears to be the primary config.storage service that everyone should be using, but yet, we're exposing it under a bad-ass name :-( → Critical follow-up issue?

    All of that being said, apparently there is a config.storage service definition for the primary active store already — Why is that not used here?

alexpott’s picture

FileSize
1.99 KB
32.04 KB
PASSED: [[SimpleTest]]: [MySQL] 59,449 pass(es). View

1. Because nothing is injected into the module handler at the moment. Injecting this will take far too much refactoring. And is out of scope in my opinion.
2. This is a lift and shift with minimal changes
3. This is a lift and shift with minimal changes
4. This is a lift and shift with minimal changes
5. The theme handler is used in the installer pre having any knowledge or checking that the config system is writable. I agree that creating an extension installer would be desirable but it is far beyond the scope of this patch.
6. We now always have config in a DUBT test - this test is testing snapshot functionality this comment explains exactly why.
7. DUBT does not properly support enabling themes
8.1/8.2 Okay re-writing to make clearer.
8.3 Using $this->container in tests is annoying and makes for unnecessarily long code but if it will get this in quicker then okay. But for tests we are discourage use of $this->container because injectable test environments make no sense - you are not going to be testing your tests. \Drupal::blah is already heavily used in DrupalUnitTestBase. Over thinking things when i decided to use 'config.cachedstorage.storage' - using 'config.storage' now.

The only thing this patch is trying to achieve is a refactor to a service so we can start to work on these more complex issues AND swap out where default configuration comes from so we can make config synchronisation work. Please lets get this done.

Status: Needs review » Needs work

The last submitted patch, 22: 2166065.22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

22: 2166065.22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2166065.22.patch, failed testing.

The last submitted patch, 22: 2166065.22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

22: 2166065.22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2166065.22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

22: 2166065.22.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

The only thing this patch is trying to achieve is a refactor to a service so we can start to work on these more complex issues AND swap out where default configuration comes from so we can make config synchronisation work. Please lets get this done.

There is indeed lots to unblock, so let's move with this!

alexpott’s picture

FileSize
3.75 KB
30.77 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
alexpott’s picture

FileSize
684 bytes
31.44 KB
PASSED: [[SimpleTest]]: [MySQL] 59,558 pass(es). View

The last patch is gonna fail a test added by #2098119: Replace config context system with baked-in locale support and single-event based overrides. It uses config_install_default_config() which it removed by this patch. It shouldn't be using it at all - it also unnecessarily installs locale config even though locale is no longer required for language overrides :)

The last submitted patch, 31: 2166065.31.patch, failed testing.

Gábor Hojtsy’s picture

Good point :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Sutharsan’s picture

Related:
The current issue did not remove the 'config.installer.validate' event listener. I created #2177689: Removed deprecated event listeners.

Status: Fixed » Closed (fixed)

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