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 content or 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 content or config entities' soft dependencies and use this to present a confirm form on module uninstall

Files: 
CommentFileSizeAuthor
#32 30-32-interdiff.txt2.42 KBalexpott
#32 2082117.32.patch17.92 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,973 pass(es).
[ View ]
#30 26-30-interdiff.txt2.57 KBalexpott
#30 2082117.30.patch17.88 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,229 pass(es).
[ View ]
#26 23-26-interdiff.txt790 bytesalexpott
#26 2082117.26.patch16.14 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,193 pass(es), 11 fail(s), and 32 exception(s).
[ View ]
#23 14-17-interdiff.txt3.15 KBalexpott
#23 2082117.17.patch16.15 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 58,788 pass(es), 16 fail(s), and 2 exception(s).
[ View ]
#14 7-14-interdiff.txt2.07 KBalexpott
#14 2082117.14.patch15.21 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,550 pass(es).
[ View ]
#11 7-11-interdiff.txt5.61 KBalexpott
#11 2082117.11.patch15.42 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 58,849 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#7 4-7-interdiff.txt1.86 KBalexpott
#7 2082117.7.patch14.68 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,430 pass(es).
[ View ]
#4 2082117.4.patch15.28 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,938 pass(es).
[ View ]
#3 2082117.3.patch22.5 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,835 pass(es).
[ View ]
#2 2082117.2.patch66.89 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,733 pass(es).
[ View ]

Comments

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

Issue summary:View changes

Updated issue summary.

Status:Active» Needs review
StatusFileSize
new66.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,733 pass(es).
[ View ]

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.

StatusFileSize
new22.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,835 pass(es).
[ View ]

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

Here is a cleaned up patch

StatusFileSize
new15.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,938 pass(es).
[ View ]

Status:Needs review» Needs work

missing whitespace...

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

too much whitespace...

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

otherwise, this looks good to go.

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

Status:Needs work» Needs review
StatusFileSize
new14.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,430 pass(es).
[ View ]
new1.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

Status:Needs review» Reviewed & tested by the community

nice work.

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

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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new15.42 KB
FAILED: [[SimpleTest]]: [MySQL] 58,849 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new5.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.

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 ?

Status:Needs work» Needs review
StatusFileSize
new15.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,550 pass(es).
[ View ]
new2.07 KB

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

Hopefully have fixed everything from yched's review.

Status:Needs review» Reviewed & tested by the community

Looks good

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.

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

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

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

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs review

Still needs the improved comment.

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

StatusFileSize
new16.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,788 pass(es), 16 fail(s), and 2 exception(s).
[ View ]
new3.15 KB

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

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.

Status:Needs work» Needs review
StatusFileSize
new16.14 KB
FAILED: [[SimpleTest]]: [MySQL] 59,193 pass(es), 11 fail(s), and 32 exception(s).
[ View ]
new790 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().

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.

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.

Status:Needs work» Needs review
StatusFileSize
new17.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,229 pass(es).
[ View ]
new2.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().

Status:Needs review» Reviewed & tested by the community

That's great.

StatusFileSize
new17.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,973 pass(es).
[ View ]
new2.42 KB

A couple of minor coding standard issues noticed by @xjm

Assigned:Unassigned» catch

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

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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

Issue summary:View changes

Add a suggested commit message