Updated: Comment #0

Suggested comment message

Issue #2082117 by alexpott, xjm, tim.plunkett, heyrocker, beejeebus, sun: Fixed Install default config only when the owner and provider modules are both enabled.

Since this was split from the meta issue.

Problem/Motivation

Based on various IRC discussions with alexpott, xjm, mtift, and others, this is a sub-issue of #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API that focuses exclusively on what happens to configuration entities when modules are installed. Many of the problems are discussed in that other issues. The options are basically to install default config or not.

Please note that #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall covers the topic of uninstallation.

Proposed resolution

One suggestion is to only install default config when both the providing module and the config entity defining modules are (or will be) installed. The proposal is to create a new ExtensionInstallStorage class that extends InstallStorage and that can be used in config_install_default_config() to determine if an extension provides default configuration for any other enabled extensions. This is the approach used in #1776830-75: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API.

Remaining tasks

Decide what to do when a module is installed that provides default configuration that uses another module's API

User interface changes

None. However, this patch might enable changes to the UI during module installation.

API changes

TBD

#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
#1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API
#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

This is currently blocked by #2082499: Uninstalling action module removes actions created by the user module since whilst action config entities are owned by the action module things just break eg. Drupal\user\Tests\UserAdminTest

alexpott’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Status: Active » Needs review
FileSize
66.89 KB

Adding a patch that includes the fix from #2082499-6: Uninstalling action module removes actions created by the user module to get testbot to see where I'm at.

alexpott’s picture

FileSize
22.5 KB

Some extra change to ./core/vendor crept into the last patch cause I was playing around with composer :)

Here is a cleaned up patch

alexpott’s picture

FileSize
15.28 KB
Anonymous’s picture

Status: Needs review » Needs work

missing whitespace...

+        return !preg_match('/^'.$name.'\./', $value);

too much whitespace...

+    $enabled_extensions +=  array_keys(array_filter(list_themes(), function ($theme) {return $theme->status;}));

otherwise, this looks good to go.

Anonymous’s picture

oh, i forgot to ask - what's up with the commented out bits?

alexpott’s picture

Status: Needs work » Needs review
FileSize
14.68 KB
1.86 KB

Points addressed - that test is now removed since this is now impossible as we search default configuration directories for the name plus a period.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php
@@ -182,17 +182,19 @@ function testNameValidation() {
+//    // Verify an exception is thrown when importing configuration with an
+//    // invalid name (missing a namespace).
+//    $message = 'Expected ConfigNameException was thrown when attempting to install invalid configuration.';
+//    try {
+//      $this->enableModules(array('config_test_invalid_name'));
+//      $this->installConfig(array('config_test_invalid_name'));
+//      $this->fail($message);
+//    }
+//    catch (ConfigNameException $e) {
+//      $this->pass($message);
+//    }

Removed

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

nice work.

yched’s picture

+++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
@@ -0,0 +1,64 @@
+      // Don't think we need to worry about profile because it is an enabled
+      // module and any modules it has will also be enabled or not.
+      // $this->folders = $this->getComponentNames('profile', array(drupal_get_profile()));

This doesn't look completely polished yet ? :-)
Also, there are phpdocs in this class that look like they should be {@inheritdoc}

yched’s picture

More nitpicks :-)

  1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOtherModuleTest.php
    @@ -0,0 +1,67 @@
    +      'name' => 'Default configuration other modules',
    

    name looks weird ?

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOtherModuleTest.php
    @@ -0,0 +1,67 @@
    +  public function testInstallOtherModule() {
    

    Misses phpdoc

  3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOtherModuleTest.php
    @@ -0,0 +1,67 @@
    +    // We can not use this entity system because the config_test entity type
    

    I might be wrong, but s/this/the/ ?
    + Could use an empty line and a "Check that the config entity doesn't exist before the config_test module is enabled" beforehands.

  4. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOtherModuleTest.php
    @@ -0,0 +1,67 @@
    +    $other_module_config_entity->set('style', "The piano ain't got no wrong notes.");
    +    $other_module_config_entity->save();
    

    This is why the last part of the method makes sense, but this could use a word of explanation here.

  5. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOtherModuleTest.php
    @@ -0,0 +1,67 @@
    +
    

    Extra white line

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
15.42 KB
5.61 KB

Thanks @yched - why is reviewing your own code is so difficult?

I've also improved the ConfigOtherModuleTest to test what happens when you uninstall the config entity provider. Whilst that's not actually changed by this patch it's good to have coverage of the current behaviour.

Status: Needs review » Needs work

The last submitted patch, 2082117.11.patch, failed testing.

yched’s picture

why is reviewing your own code is so difficult?

Don't tell me about it :-)

Gone into annoying / überpicky mode, sorry...

  1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOtherModuleTest.php
    @@ -21,7 +21,7 @@
    -      'name' => 'Default configuration other modules',
    +      'name' => 'Default configuration provider for config_test',
    

    I still wouldn't make sense of that test name if I saw it in the test listing page (but maybe it's just me, feel free to ignore)

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOtherModuleTest.php
    @@ -34,34 +34,50 @@
    +    // is enabled. We can not use the entity system because the config_test
    

    "cannot" ?

  3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOtherModuleTest.php
    @@ -34,34 +34,50 @@
    +    // Update the default configuration. To test that the changes are preserved
    +    // if the module that provides the default configuration is uninstalled.
    

    This sounds like one single sentence ?

alexpott’s picture

Status: Needs work » Needs review
FileSize
15.21 KB
2.07 KB

The reason the test fail is that I left a spurious assert in :(

Hopefully have fixed everything from yched's review.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good

catch’s picture

Status: Reviewed & tested by the community » Needs review

Installing default config for other modules only when the other module is installed makes complete sense.

However the patch currently feels like it'd be easy to get inconsistent behaviour.

So taking default views for example, seems like with the same module being enabled, two things can happen depending on whether Views is enabled before or after:

1. Views isn't installed. Module foo is enabled. No default views are imported. Views is enabled. Still no default views.

2. Views is installed. Module foo is enabled. Default views are imported.

I don't see anything in the patch that deals with that, so I'm either missing something or we likely need a new issue to discuss that - I think it can be a follow-up but still might need to be critical.

Anonymous’s picture

yep, #16 is a good point, and can be a follow up i reckon.

catch’s picture

Alex pointed out in irc that this is actually covered by the patch, it's just easy to miss. Asked for a better comment.

Anonymous’s picture

huh, yep, i had no idea InstallStorage did that. ignore me.

i really, really don't like the layout of these file storage objects.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

oh yeah, i guess that means we're back at RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Still needs the improved comment.

yched’s picture

+++ b/core/includes/config.inc
@@ -22,10 +24,33 @@
 function config_install_default_config($type, $name) {
+  // Get all default configuration owned by this extension.
+  $source_storage = new ExtensionInstallStorage();
+  $config_to_install = $source_storage->listAll($name . '.');

Yeah, that part confused me as well when reviewing, I had to take a good look at the code to realize that this was the "collect the config in all other, already enabled modules, that hadn't been installed so far and can now be installed" part, and that the code indeed implemented the behavior advertized in the issue. The second half of the method is more akin to what you would expect, but that 1st part is easy to miss.

Maybe this just needs some explicit higher level comment explaining what config_install_default_config() does (and maybe starting with the second part makes things a bit easier to grasp):
- among the config items provided by the newly enabled module, install the ones that can be installed (those that are owned by modules that are available)
- among the config items provided by all other modules, install the ones that can now be installed (the ones owned by the newly enabled module)
1st part feels fairly intuitive, 2nd part is "a natural counterpart of the 1st part if you think about it"

alexpott’s picture

FileSize
16.15 KB
3.15 KB

Here's the new and improved comment, tests and some coding standards things I spotted :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Works for me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2082117.17.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
16.14 KB
790 bytes

Oh that was fun - the system.theme config was being cache in the factory because we where calling list_themes(TRUE) during a theme_enable().

catch’s picture

It'd be good to add the additional comment yched mentioned in |#22 to config_install_default_config(), with that I think it'll be clear what's happening, the new comment should be enough with that extra context, but I'm still not sure I could figure it out scanning through the code not knowing what it does already.

yched’s picture

well, +1 on #27 then ;-)...
since that comment would be about "what the function does" rather than "how it does it", it might be more helpful in the phpdoc ?
(the patch actually makes the current function description stale anyway)

Status: Needs review » Needs work

The last submitted patch, 2082117.26.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
17.88 KB
2.57 KB

Yep moved the documentation into the function doc block and worked on the text with @xjm

Also hopefully fixed the test failures. Basically we need to ensure that the theme_list() is correct when calling config_install_default_config().

catch’s picture

Status: Needs review » Reviewed & tested by the community

That's great.

alexpott’s picture

FileSize
17.92 KB
2.42 KB

A couple of minor coding standard issues noticed by @xjm

webchick’s picture

Assigned: Unassigned » catch

I think I'd sooner this be committed by catch than me.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Add a suggested commit message