Problem/Motivation

Blocking #2090115: Don't install a module when its default configuration has unmet dependencies

Our configuration entity dependency system only understands names, not capabilities. That is, we only check if names match to decide if something meets a dependency, not the capabilities of a config entity.

If a module ships foo.bar.config_entity, and that already exists in the active store, we have no way to know if it is safe to install the module. This is not to say things will break - we just have no idea, because we only use names. Fixing that is another discussion.

As such, we can't install a module unless all of it's default config entities are installed, because we can't see if an existing config entity with the same name provides the capabilities that the module requires.

We could also do on-the-fly renames during module install, but that would require a new code path for what is potentially a corner case.

This issue was created as a result of @yched's comment #2138559-17: Replace UUIDs in configuration entities with a creation ID

Proposed resolution

We should block install of any module where the config entity names match an existing config entity, and provide an error screen explaining why we blocked install and how the user can resolve the issue.

"Foo module could not be installed because some of it's default configuration can not be installed. Please rename "$foo.bar" so that installation is possible."

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
(after reroll) Create a patch to address points in #86 (and update verbiage?) and make an interdiff Instructions
Reroll the patch if it no longer applies. Instructions
(after reroll) Add automated tests Instructions
Embed before and after screenshots in the issue summary Novice Instructions
discuss UX (see #84)
when this is fixed, unpostpone #2090115: Don't install a module when its default configuration has unmet dependencies

User interface changes

A new warning message.

API changes

A new way in module installation can fail.

How to test and current screenshots

  • Open settings.php and at the end uncomment the lines to include settings.local.php
  • Open settings.local.php and make sure extension_discovery_scan_tests is uncommented - this allows you to enable test modules
  • Go to admin/modules, you can test following scenarios.
    1. Toggle 'Configuration test' and 'Configuration install fail test' - you should get a message after enabling on the modules page
    2. Toggle 'Configuration install fail test'. You should go to the confirm form and after that a message on the modules page
    3. Toggle 'Configuration install fail dependency test'. You should go to the confirm form and after that a message on the modules page
    4. Enable 'Configuration test' separately. After that enable 'Configuration install fail test'. You should go to admin/extend/config-clash and see information on the clashing configuration

Postponed until

#2224581: Delete forum data on uninstall

Files: 
CommentFileSizeAuthor
#115 2140511-2.115.patch43.97 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,999 pass(es). View
#115 113-115-interdiff.txt2.76 KBalexpott
#113 2140511-2.113.patch42.1 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,989 pass(es). View
#113 111-113-interdiff.txt1.23 KBalexpott
#111 2140511-2.111.patch42.06 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,993 pass(es). View
#111 109-111-interdiff.txt7.9 KBalexpott
#109 2140511-2.108.patch41.89 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,964 pass(es). View
#109 106-108-interdiff.txt5.23 KBalexpott
#106 2140511-2.106.patch37.49 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,960 pass(es), 1 fail(s), and 0 exception(s). View
#106 103-106-interdiff.txt37.47 KBalexpott
#103 2140511-2.103.patch38.7 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,949 pass(es), 1 fail(s), and 0 exception(s). View
#103 99-103-interdiff.txt10.78 KBalexpott
#101 interdiff-2140511-99-101.txt0 bytesbabruix
#101 2140511-101.patch0 bytesbabruix
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,566 pass(es). View
#99 interdiff-2140511-93-99.txt3.05 KBbabruix
#99 2140511-99.patch34.03 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,589 pass(es), 1 fail(s), and 0 exception(s). View
#94 Screenshot from 2014-12-12 10:54:59.png17.07 KBswentel
#94 Screenshot from 2014-12-12 10:53:41.png35.15 KBswentel
#93 interdiff.txt12.42 KBswentel
#93 2140511-new.93.patch35 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,064 pass(es). View
#89 2140511-new.89.patch28.46 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,060 pass(es). View
#89 84-89-interdiff.txt6.27 KBalexpott
#84 2140511-new.84.patch27.53 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,738 pass(es), 1 fail(s), and 0 exception(s). View
#84 81-84-interdiff.txt820 bytesalexpott
#81 2140511-new.81.patch28.03 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,747 pass(es), 1 fail(s), and 0 exception(s). View
#71 2140511-new.71.patch30.56 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,013 pass(es), 1 fail(s), and 0 exception(s). View
#64 2140511.64.patch26.39 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,803 pass(es), 55 fail(s), and 1 exception(s). View
#62 2140511.62.patch25.95 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,487 pass(es), 58 fail(s), and 1 exception(s). View
#62 59-62-interdiff.txt28.97 KBalexpott
#59 2140511.59.patch15.54 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,847 pass(es), 56 fail(s), and 1 exception(s). View
#57 2140511-57.patch17.23 KBohthehugemanatee
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2140511-57.patch. Unable to apply patch. See the log in the details link for more information. View
#54 2140511-53.patch13.57 KBohthehugemanatee
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#46 2140511.46.patch10.87 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,215 pass(es), 86 fail(s), and 9 exception(s). View
#46 19-46-interdiff.txt4.3 KBalexpott
#41 install-fail.png33.7 KBjessebeach
#20 2140511.19.patch7.13 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,224 pass(es), 191 fail(s), and 19 exception(s). View
#20 interdiff.txt2.82 KBGábor Hojtsy
#20 Extend | drupal8.local 2014-02-27 11-38-31 2014-02-27 11-38-36.jpg84.94 KBGábor Hojtsy
#18 interdiff.txt3.93 KBGábor Hojtsy
#18 2140511.18.patch7.14 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#15 Extend | drupal8.local 2014-02-27 10-30-13 2014-02-27 10-30-23.jpg79.95 KBGábor Hojtsy
#14 2140511.13.patch4.62 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,710 pass(es). View
#2 2140511.2.patch4.65 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2140511.2.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Status: Active » Needs review
FileSize
4.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2140511.2.patch. Unable to apply patch. See the log in the details link for more information. View

The attached patch creates a warning message when a config file can not be imported - 'Active configuration with the name %config_name already exists therefore can not install the default configuration with the same name from %extension_name.'

Also interestingly we have system.module.yml in system/config but it is never being used because this file is written by the installer before the system module is enabled! So this patch removes it because pretending we have default config when we don't is a nonsense.

alexpott’s picture

One issue is that we have different situations where this can occur and we might want to do different things in each - which will almost be impossible.

  1. A user can install the node module and create a node type called forum and then install the forum module.
  2. A user can install the forum module and uninstall the forum module and then install it again - at the moment the config entities own by other modules are not removed - and there are tests for this.
  3. Module A can contain default config for field.field.node.some_field and module B can contain default config for field.field.some_field

I'd argue that in scenario 1 we'd want to prevent forum from being installed and in scenario 3 we're want to prevent module B from being installed. But we also have specific tests around forum node type preservation after disabling the forum module! With the patch in #1 doing scenario 3 using drush will lead to the following output when enabling forum for the final time:

The following extensions will be enabled: forum
Do you really want to continue? (y/n): y
forum was enabled successfully.                                                                                                                                                                                      [ok]
forum defines the following permissions: administer forums
Active configuration with the name entity.display.taxonomy_term.forums.default already exists therefore can not install the default configuration with the same name from forum.                                     [warning]
Active configuration with the name entity.form_display.taxonomy_term.forums.default already exists therefore can not install the default configuration with the same name from forum.                                [warning]
Active configuration with the name node.type.forum already exists therefore can not install the default configuration with the same name from forum.                                                                 [warning]
Active configuration with the name taxonomy.vocabulary.forums already exists therefore can not install the default configuration with the same name from forum.
alexpott’s picture

Added the related issue where we discussed what to do with config entities owned by another module but provided as default configuration by the module that is being uninstalled. In other (simpler words) should the views.view.frontpage be uninstalled when the node module is uninstalled?

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 2140511.2.patch, failed testing.

xjm’s picture

So, scenario 1 in D7 is hilarious. You end up with one content type called forum that is a hybrid of both content types. So I don't think anyone is relying on a particular behavior there. ;)

alexpott’s picture

Drupal\system\Tests\Common\UrlTest does not fail locally going to retest.

alexpott’s picture

Status: Needs work » Needs review

2: 2140511.2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2140511.2.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

2: 2140511.2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2140511.2.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,710 pass(es). View

Rerolled to apply again.

Gábor Hojtsy’s picture

This is the output after copying node.type.page.yml into actions/config and trying to enable actions module.

Gábor Hojtsy’s picture

Title: Default configuration can be silently not imported » Configuration file name collisions silently ignored for default configuration
Priority: Major » Critical
Issue tags: +beta target

Closed #2205845: Configuration file name collisions silently ignored as duplicate. Applying same priority and beta target tag.

Gábor Hojtsy’s picture

Issue tags: +#lsdhack
Gábor Hojtsy’s picture

FileSize
7.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
3.93 KB

Here is a proof of concept of a more strict approach. This checks before installing a module whether the default config will overlap with anything in active config. If so, the module is not installed and all the module installation is stopped (because further modules may have requirement to install the prior one).

Status: Needs review » Needs work

The last submitted patch, 18: 2140511.18.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
84.94 KB
2.82 KB
7.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,224 pass(es), 191 fail(s), and 19 exception(s). View

The error message certainly needs work, and should not be a warning because it does stop the installation from happening. Here is an updated patch with that and some more minor fixes. Here is how it looks like:

Status: Needs review » Needs work

The last submitted patch, 20: 2140511.19.patch, failed testing.

Gábor Hojtsy’s picture

beejeebus’s picture

i don't agree with any 'prevent install of module' validation.

here's my logic, please correct me if i'm wrong:

- modules cannot rely on configuration entities. config entities can be arbitrarily deleted/renamed/updated so that the name is the same, but everything else about them is different.

- if a module can't safely be installed without it's default configuration entities, that is a bug with the module.

catch’s picture

modules cannot rely on configuration entities. config entities can be arbitrarily deleted/renamed/updated so that the name is the same, but everything else about them is different.

This isn't strictly true either from an API standpoint or the status quo in core. Forum module for example creates locked configuration entities:

id: taxonomy_term.forum_container
status: true
langcode: en
name: forum_container
type: list_boolean
settings:
  allowed_values:
    - ''
    - ''
  allowed_values_function: ''
module: options
entity_type: taxonomy_term
locked: true
cardinality: 1
translatable: false
indexes: {  }
Gábor Hojtsy’s picture

@beejeebus: the problem lies within dependencies in configuration, you don't even need to have any line of PHP code. Let's say your module ships with field_product which is an image field. It is included in one or more displays of your products_view, that you also ship with the module. Now in Drupal 8, let's say you already have a field_product which is an SKU text field. Your module will be installed, the new (image field) field_product will not be installed because it conflicts, but that is silently ignored. The view that has the image field specific configuration for the field that is now a text field will be. Now what?

alexpott’s picture

@swentel's point is relevant to this discussion - https://drupal.org/comment/8557879#comment-8557879

The idea is simple: default configuration entities (and maybe also just simple settings) simply don't come with a module that is also the provider, or in another module that would need the other as a dependency.

As a consequence,

  • 'default' configuration is just a 'lie' and something that simply doesn't exist ...
  • The only place to provide default config is in installation profiles or in 'default configuration modules', or 'feature' modules which you create yourself easily in projects etc.
  • our core modules would then become clean and have no idea whatsoever about 'default' configuration, which would make the code pasted above in node_add_body_field in the current patch obsolete, and imho also simply not done.

To me this feels almost like a natural evolution in the CMI process, not sure how others feel about that idea. Definitely not something to fix in this patch, but for a follow up, but this is the idea. so .. shoot :)

beejeebus’s picture

re #25 - or field_product could be an image field supplied by another module, and could work just fine.

or views is not installed when your module is installed, and the default view is not installed until a later date. or s/view/image/ etc etc

really, this is a bigger problem, and not at all confined to just names, and is only really solved by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall and any follow ups to that.

Gábor Hojtsy’s picture

@beejeebus I don't think that would solve it, the dependencies are by name, so if my view depends on field_product, that does not really help, since a different field_product may be there. If the dependency is by UUID then we can ensure the concrete config, but then the UUID is not a UUID because it would be the same on each site :D

beejeebus’s picture

so this seems like a broken pattern then.

if you really need to rely on something by name, this is going to break. a lot. i don't see how declarative is going to work - the module will need to run code, and find a name that can be used, else install is going to fail often.

Gábor Hojtsy’s picture

Well, one option is config names are namespaced. Eg. instead of field_product, it would be field_mymodule_product and field_othermodule_product. That would require for core to follow the best practice and ship with default fields like field_node_body, view.node_frontpage, etc.

alexpott’s picture

Actually #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall might prevent it since that patch only calculates dependencies for incoming config reasoning that if all config entity dependencies have to be supplied in defaults - in other words you can not rely on another modules config entities.

That said we would still need to check this prior to module install and the only reason this will fail is that an import a configuration entity will not occur because one with the same name already exists. So I think the solution here is acceptable - I can't even imagine an edge case that it's not going to help us prevent.

jessebeach’s picture

That said we would still need to check this prior to module install and the only reason this will fail is that an import a configuration entity will not occur because one with the same name already exists.

As a naive user, this part raises alarm bells. The error prevents catastrophic data loss, but it also prevents me from building a site. I don't know, even now, what the remediation would be. Should I rename configuration files? Is this still an open issue and I just haven't found that discussion yet?

xjm’s picture

Issue tags: +Naming things is hard
jessebeach’s picture

I'm trying to understand the problem better. So, here is a scenario:

  1. I create a View that exposes as a block the image field in the default Article node type.
  2. I export that View.
  3. I import that View into another site.
  4. Will that import fail because a field named image already exists? Will it fail before an Article node type already exists?
alexpott’s picture

@jessebeach this only concerns default configuration. Configuration export and subsequent import is a different issue and one that we've solved through only supporting a complete site configuration import - partial import is not supported - our assumption is that the user intends to import what is in the staging directory and has obtained this configuration from their own site.

The problem this issue is trying to address is where a module provides default configuration entities which depend on each other (eg. a view and some fields). And one of the default configuration entities has an ID exactly the same as a configuration entity that already exists.

An example in core of a module that does this is forum. If a field already existed with the machine name forum_container before the module was installed this would be very messy and the installation would create all the other default configuration and use the existing forum_container field even though nothing what-so-ever mandates that is is the same type of field as defined in the forum module's default configuration.

Gábor Hojtsy’s picture

@jeesebeach: I have a complete example with contrib modules in #25, please see that :) Happy to elaborate.

jessebeach’s picture

If a field already existed with the machine name forum_container before the module was installed this would be very messy and the installation would create all the other default configuration and use the existing forum_container field even though nothing what-so-ever mandates that is is the same type of field as defined in the forum module's default configuration.

I also reread #25 and realized I'm using the wrong words. I shouldn't have written import; rather, I meant install.

So, given the clarifications in #25 and #35, here's the scenario as I understand it. There is really only one use case we're trying to defend against.

The core Forum module is not installed. A contrib module is installed and creates a Config Entity called forum_container. The user then enables the core Forum module which fails in some way (doesn't install or uses the wrong forum_container Config Entity).

Would it be possible to prevent installation of a module that would create a Config Entity, if a Config Entity with the same name existed in any other module installed or not? Like even if just the code were in the site, but the other module is not yet enabled? Here's what would happen in this case.

Module developer Zed is creating AwesomeForums module. The Forum module is not enabled on their development site. This developer creates a Config Entity called forum_container in their new module. The developer attempts to enable the module and whamo, it won't enable. The error message on the install screen explains that their Config Entity forum_container has the same name as a Config Entity of the Forum module and could lead to conflict -- please namespace the Config Entity name.

This would prevent module developers from creating Config Entities that conflict with core Config Entities. Now what about contrib-to-contrib Config Entity name conflicts? The same restriction applies. Except in contrib, developers will just need to namespace their Config Entities with the module namespace to prevent collisions.

And we don't do anything more complex that this. In the case where a Config Entity being installed has dependencies to what it expects to be existing Config Entities (field_image or field_product for example), we can simply list any missing dependencies on an install confirmation screen (field_image was deleted!), state that the missing fields may cause behavior problems with the new module, and ask the site builder if they'd like to proceed.

Gábor Hojtsy’s picture

The "configuration depends on other configuration that may be deleted already" case is handled by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall. This issue is about depending on a configuration that *exists* but may or may not be as expected. When you ship with configuration but part of your shipped configuration is not installed so your other config may or may not use dependents that it expected to use.

jessebeach’s picture

When you ship with configuration but part of your shipped configuration is not installed...

Right, and there are three ways this could happen.

  1. A module developer uses a name for a Config Entity that a Core module would also use.
  2. A site builder creates a field with a name that a Core module would also use.
  3. A module developer uses a name for a Config Entity that a contrib module also uses.

If we can prevent (1) from happening by shutting down module enabling in a module under development and (2) by disallowing a field to be given a name that a Core module might also use, then we avoid the downstream conflicts for site builders who would enable a Core module like Forum and run into conflicts.

And these restrictions, although not preventing (3), would raise conflicts quickly and promote proper namespacing in contrib.

Sorry to harp on this, but I'm trying to find a solution that would simply prevent the conflict from arising in the first place by making it really difficult to post a contrib module to d.o. that defines Config Entities with names that Core might potentially use. We want the decision point to exist primarily at development time so that developers are forced to confront these issues, not at site building time, when a site builder, who may not have a deep knowledge of Drupal, would be confronted with some tough decisions and little in terms of information to make them correctly.

jessebeach’s picture

So, all that being said, I think the approach in this patch is fine for what it seeks to prevent. What I'm arguing is that we should strive to never let a site builder get to the point where this install error is raised.

jessebeach’s picture

FileSize
33.7 KB

With the patch in #20, the Standard profile can no longer be installed :)

 standard cannot be installed because default configuration named system.cron already exists in active configuration.

Gábor Hojtsy’s picture

Lol, that looks like a testament to how successful this is :D

Berdir’s picture

jessebeach’s picture

The following code can be removed from ConfigInstallWebTest::testInstallProfileConfigOverwrite(), which is added by #1986090: Profile config does not overwrite module default config on install (system.cron.yml).

// Turn on the test module, which will attempt to replace the
// configuration data. This attempt to replace the active configuration
// should be ignored.
\Drupal::moduleHandler()->install(array('config_override_test'));

// Verify that the test module has not been able to change the data.
$config = \Drupal::config($config_name);
$this->assertIdentical($config->get(), $expected_profile_data);

// Disable and uninstall the test module.
\Drupal::moduleHandler()->uninstall(array('config_override_test'));

// Verify that the data hasn't been altered by removing the test module.
$config = \Drupal::config($config_name);
$this->assertIdentical($config->get(), $expected_profile_data);

Instead, installing a module or theme that attempts to install a default configuration file that already exists in active configuration should fail with an error message.

jessebeach’s picture

So, when the profile is installed after the basic modules are installed, we run into fatal errors because the profile tries to apply its default configuration files. I think what should happen, given #1986090: Profile config does not overwrite module default config on install (system.cron.yml) will already have taken the Profiles default configuration into account, is that a profile should not attempt to install any default configuration; it should only be installed in the context of a module installing.

alexpott’s picture

Status: Needs work » Needs review
Related issues: +#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall
FileSize
4.3 KB
10.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,215 pass(es), 86 fail(s), and 9 exception(s). View

Immediate reaction to #45 was +1 but the problem here is that profiles are full modules and it is perfectly possible for them to have their own configuration. They also supply default configuration entities and these cannot be installed until after all the modules have been because there are instances when it is creating configuration with multiple dependencies. A good example is editor.editor.full_html - this configuration is dependent on both ckeditor and editor but it can not be created when editor is installed because ckeditor is not installed yet (it is dependent on editor) - fun eh?

I think we need to handle profiles in a special way. Removing the already existing configuration in ConfigInstaller::gatherDefaultConfig() seems one option.

Also this patch appears blocked on #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall since the reason Drupal\comment\Tests\CommentFieldsTest is failing is that it uninstalls the Comment module and then reinstalls it. This currently will leave the default configuration for configuration entities comment does not provide in the active configuration - for example: views.view.comments_recent

The fails in FieldImportDeleteTest and FieldImportChangeTest were due to the fact that the field configuration provided by field_test_config is imported by \Drupal\field\Tests\FieldUnitTestBase::setUp() when it installs field configuration - this has occurred since #2082117: Install default config only when the owner and provider modules are both enabled landed but in HEAD we silently ignore the duplicate config :)

jessebeach’s picture

Nice! I jus wrote the same-ish block of code in ConfigInstaller to deal with filtering out already installed configs.

// If this is a profile module, remove any configuration that has already
// been transferred to active configuration because a module that shares
// the default configuration was already installed.
if ($type === 'profile') {
  $config_already_installed = array();
  foreach ($config_to_install as $name) {
    if ($this->activeStorage->exists($name)) {
      $config_already_installed[] = $name;
    }
  }
  $config_to_install = array_diff($config_to_install, $config_already_installed);
}

Let's see where the bot ends up #46.

Status: Needs review » Needs work

The last submitted patch, 46: 2140511.46.patch, failed testing.

jessebeach’s picture

The patch in #46 applied on #1986090: Profile config does not overwrite module default config on install (system.cron.yml) now gets us through site installation with the default configs from the standard profile without any errors.

beejeebus’s picture

Issue summary: View changes
beejeebus’s picture

Issue summary: View changes
xjm’s picture

What does "update verbiage" mean in the summary? What verbiage where?

Gábor Hojtsy’s picture

I believe that was in reference to the error message.

ohthehugemanatee’s picture

FileSize
13.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

#1986090: Profile config does not overwrite module default config on install (system.cron.yml) is committed, but the patch from #46 didn't apply because of significant other changes since then. I tried to re-roll.

The resulting patch applies OK, but now I get an odd sort of WSOD with nothing in the logs, anywhere. It happens on line 1299 of core/modules/system/system.module , during system_rebuild_module_data. The offending line is

\Drupal::state()->set('system.module.files', $files);

But I don't know why! This causes a problem right at the beginning of moduleHandler->install(). I'll work more on this in the coming days, but any advice would be great.

I've attached the patch such as it is for now.

xjm’s picture

Status: Needs work » Needs review

Let's use testbot for some more insight.

Status: Needs review » Needs work

The last submitted patch, 54: 2140511-53.patch, failed testing.

ohthehugemanatee’s picture

FileSize
17.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2140511-57.patch. Unable to apply patch. See the log in the details link for more information. View

I tracked this down a bit further, but I still don't know why it's stopping like this. Since there's no error behavior from PHP, it might not even be an error. The values received by \Drupal::state()->set() seem just fine. It tries to set

key: system.module.files
value:

Array
(
    [action] => core/modules/action/action.info.yml
    [aggregator] => core/modules/aggregator/aggregator.info.yml
    [ban] => core/modules/ban/ban.info.yml
    [basic_auth] => core/modules/basic_auth/basic_auth.info.yml
    [block] => core/modules/block/block.info.yml
    [book] => core/modules/book/book.info.yml
    [breakpoint] => core/modules/breakpoint/breakpoint.info.yml
    [ckeditor] => core/modules/ckeditor/ckeditor.info.yml
    [color] => core/modules/color/color.info.yml
    [comment] => core/modules/comment/comment.info.yml
    [config] => core/modules/config/config.info.yml
    [config_translation] => core/modules/config_translation/config_translation.info.yml
    [contact] => core/modules/contact/contact.info.yml
    [content_translation] => core/modules/content_translation/content_translation.info.yml
    [contextual] => core/modules/contextual/contextual.info.yml
    [custom_block] => core/modules/block/custom_block/custom_block.info.yml
    [datetime] => core/modules/datetime/datetime.info.yml
    [dblog] => core/modules/dblog/dblog.info.yml
    [editor] => core/modules/editor/editor.info.yml
    [entity] => core/modules/entity/entity.info.yml
    [entity_reference] => core/modules/entity_reference/entity_reference.info.yml
    [field] => core/modules/field/field.info.yml
    [field_ui] => core/modules/field_ui/field_ui.info.yml
    [file] => core/modules/file/file.info.yml
    [filter] => core/modules/filter/filter.info.yml
    [forum] => core/modules/forum/forum.info.yml
    [hal] => core/modules/hal/hal.info.yml
    [help] => core/modules/help/help.info.yml
    [history] => core/modules/history/history.info.yml
    [image] => core/modules/image/image.info.yml
    [language] => core/modules/language/language.info.yml
    [link] => core/modules/link/link.info.yml
    [locale] => core/modules/locale/locale.info.yml
    [menu_link] => core/modules/menu_link/menu_link.info.yml
    [menu_ui] => core/modules/menu_ui/menu_ui.info.yml
    [migrate] => core/modules/migrate/migrate.info.yml
    [migrate_drupal] => core/modules/migrate_drupal/migrate_drupal.info.yml
    [minimal] => core/profiles/minimal/minimal.info.yml
    [node] => core/modules/node/node.info.yml
    [options] => core/modules/options/options.info.yml
    [path] => core/modules/path/path.info.yml
    [quickedit] => core/modules/quickedit/quickedit.info.yml
    [rdf] => core/modules/rdf/rdf.info.yml
    [responsive_image] => core/modules/responsive_image/responsive_image.info.yml
    [rest] => core/modules/rest/rest.info.yml
    [search] => core/modules/search/search.info.yml
    [serialization] => core/modules/serialization/serialization.info.yml
    [shortcut] => core/modules/shortcut/shortcut.info.yml
    [simpletest] => core/modules/simpletest/simpletest.info.yml
    [statistics] => core/modules/statistics/statistics.info.yml
    [syslog] => core/modules/syslog/syslog.info.yml
    [system] => core/modules/system/system.info.yml
    [taxonomy] => core/modules/taxonomy/taxonomy.info.yml
    [telephone] => core/modules/telephone/telephone.info.yml
    [text] => core/modules/text/text.info.yml
    [toolbar] => core/modules/toolbar/toolbar.info.yml
    [tour] => core/modules/tour/tour.info.yml
    [tracker] => core/modules/tracker/tracker.info.yml
    [update] => core/modules/update/update.info.yml
    [user] => core/modules/user/user.info.yml
    [views] => core/modules/views/views.info.yml
    [views_ui] => core/modules/views_ui/views_ui.info.yml
    [xmlrpc] => core/modules/xmlrpc/xmlrpc.info.yml
)

This looks totally normal and valid as far as I can tell. By this point in the Install process, we've already used this same function several times without issue. But when I call it inside ModuleHandler->install(), it dies on keyValueStore->set($key, $value); .

I don't understand why this should be a problem for this particular value in this particular place. In HEAD we call the system_rebuild_module_data() 1 line later, inside if ($enable_dependencies), which works fine for the installer because during install $enable_dependencies == FALSE. It seems like the keyValueStore, and therefore system_rebuild_module_data(), isn't useful this early in the install process.

We are using system_rebuild_module_data() to populate $module_data to skip already installed modules, and, specific to this patch, to run this key snippet:

        // Validate default configuration of this module. Bail if unable to
        // install. Should not continue installing more modules because those
        // may depend on this one.
        if (!\Drupal::service('config.installer')->validatePreInstall($module_data[$module]->getType(), $module)) {
          break;
        }

which is part of the point of this whole patch.

TL;DR: I'm not sure what to do here.

  • We don't want to run only when $enable_dependencies == TRUE. If the default configuration fails validation, we should break out of the install process, just like we would with the module enabling process.
  • Is there an alternative way to populate $module_data that might work this early in the install process?

I've attached an updated version of the patch which gets through the install process with the default profile. All I did is move $module_data = system_rebuild_module_data() and the validatePreInstall() into the $enable_dependencies == TRUE if statement, so they don't fire during Install. I don't think that's Right, but it works.

xjm’s picture

#2090115: Don't install a module when its default configuration has unmet dependencies is postponed on this issue since the validation step would be useful there.

alexpott’s picture

Status: Needs work » Needs review
FileSize
15.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,847 pass(es), 56 fail(s), and 1 exception(s). View

Rerolled patch to account for #2262861: Add concept of collections to config storages

We still validate the modules that will can installed on ModulesListConfirmForm - and if there is any pre existing configuration inform the user that this is the case and give them links to make it easy to delete or rename everything and prevent the user from installing the modules.

Also we'll need to change Drush to have an option to force overwriting configuration so that developers can easily uninstall and reinstall modules that provide configuration entities that do not have a dependency on the module.

@ohthehugemanatee #2262861: Add concept of collections to config storages had a big impact on the ConfigInstaller - the changes in #54 and #57 needed to reimplement the early functionality on top of those changes - not really a straight reroll :)

No interdiff because of PSR4 and the extensive changes.

Status: Needs review » Needs work

The last submitted patch, 59: 2140511.59.patch, failed testing.

Gábor Hojtsy’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstallerInterface.php
    @@ -84,4 +84,18 @@ public function setSyncing($status);
    +   * Validate default configuration before installation.
    

    Certainly is not doing any validation.

  2. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
    @@ -0,0 +1,68 @@
    + * An exception thrown in case of config already already existing errors.
    

    Sentence needs cleanup, eg. repetition.

  3. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
    @@ -0,0 +1,68 @@
    +   * The configuration names that already exists, keyed by collection.
    +   *
    +   * @var string
    +   */
    +  protected $configNames = array();
    

    Not a string.

  4. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
    @@ -0,0 +1,68 @@
    +   * Sets the extensions being installed.
    +   *
    +   * @param array $extensions
    +   *   The extensions being installed.
    ...
    +   * Gets the extensions being installed.
    +   *
    +   * @return array
    +   *   The extensions being installed.
    

    Would be good to document the structure of the array.

  5. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
    @@ -0,0 +1,68 @@
    +   * Sets the configuration names.
    +   *
    +   * @param array $config_names
    +   *   The configuration names keyed by collection.
    

    Is that a nested associative array?

  6. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
    @@ -0,0 +1,68 @@
    +   * Gets the configuration names keyed by collection.
    +   *
    +   * @return string
    +   *   The configuration names.
    

    Not a string.

  7. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -676,6 +677,20 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +      if (count($existing_configuration)) {
    +        $exception = new PreExistingConfigException();
    +        $exception->setConfigNames($existing_configuration);
    +        $exception->setExtensions($module_list);
    +        throw $exception;
    +      }
    

    How does this show up on the UI, what is the user feedback now?

  8. +++ b/core/modules/config/tests/config_install_fail/config_install_fail.module
    @@ -0,0 +1,6 @@
    + * Empty module to test providing a default config entity with the same name.
    

    With the same name as? :)

alexpott’s picture

Status: Needs work » Needs review
FileSize
28.97 KB
25.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,487 pass(es), 58 fail(s), and 1 exception(s). View

This issue is nasty. The new patch implements config clash detection when

  1. Fixed
  2. Fixed
  3. Fixed
  4. Fixed
  5. Fixed
  6. Fixed
  7. If you try to install a module with clashing configuration you get redirected to a new config clash screen.
  8. Fixed

Need to implement config clash page for themes. Need to add tests for the config clash pages and need to resolve what to do with NodeTypePersistenance and InstallUninstallTest because these tests become impossible / trickier now.

Status: Needs review » Needs work

The last submitted patch, 62: 2140511.62.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
26.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,803 pass(es), 55 fail(s), and 1 exception(s). View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 64: 2140511.64.patch, failed testing.

xjm’s picture

xjm’s picture

Issue summary: View changes

Per discussion with @alexpott and @larowlan, postponing on #2224581: Delete forum data on uninstall.

xjm’s picture

Status: Needs work » Postponed

Actually postponing. :)

catch’s picture

Issue tags: +D8 upgrade path
alexpott’s picture

Status: Postponed » Needs work
alexpott’s picture

Status: Needs work » Needs review
Related issues: +#2267453: Views plugins do not store additional dependencies
FileSize
30.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,013 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled - we have a serious issue with views because currently they do not even have dependencies on the modules that provide the entity type they are querying - see #2267453: Views plugins do not store additional dependencies

Status: Needs review » Needs work

The last submitted patch, 71: 2140511-new.71.patch, failed testing.

alexpott’s picture

Status: Needs work » Postponed

Postponing on #2267453: Views plugins do not store additional dependencies since doing this patch without view dependencies working is impossible due to how InstallUninstallTest works

alexpott’s picture

Status: Postponed » Needs work

Status: Needs work » Needs review

Berdir queued 71: 2140511-new.71.patch for re-testing.

The last submitted patch, 57: 2140511-57.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 71: 2140511-new.71.patch, failed testing.

alexpott’s picture

catch’s picture

Title: Configuration file name collisions silently ignored for default configuration » [PP-1] Configuration file name collisions silently ignored for default configuration
alexpott’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
28.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,747 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled on top of all the changes including the new ModuleInstaller - which nicely keeps all of this complexity out of the module handler.

alexpott’s picture

Title: [PP-1] Configuration file name collisions silently ignored for default configuration » Configuration file name collisions silently ignored for default configuration

Fixed issue title - this is no longer postponed.

Status: Needs review » Needs work

The last submitted patch, 81: 2140511-new.81.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +needs te
FileSize
820 bytes
27.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,738 pass(es), 1 fail(s), and 0 exception(s). View

This might be green - after that we need to fix up the patch, add tests and talk about the UX.

Status: Needs review » Needs work

The last submitted patch, 84: 2140511-new.84.patch, failed testing.

Wim Leers’s picture

Initial review.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -156,16 +157,17 @@ protected function listDefaultConfigCollection($collection, $type, $name, array
    +        // Ensure that we have not already discovered the config to install.
    

    Nit: Wouldn't something like "Ensure that the config to install isn't already installed." make more sense?
    After reading this hunk a few times, it made sense to me though.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -156,16 +157,17 @@ protected function listDefaultConfigCollection($collection, $type, $name, array
    +        // Ensure the configuration is provided by an enabled module
    

    Nits:

    1. s/configuration/needed configuration/
    2. Missing trailing period.
  3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -330,4 +332,32 @@ public function setSyncing($status) {
    +    // dependencies with ModuleHandler and ThemeHandler.
    

    Nit: s/with/on/

  4. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
    @@ -0,0 +1,14 @@
    + * An exception thrown if configuration already exists with the same name.
    

    Nit: s/configuration already exists with the same name/configuration with the same name already exists/

    ?

  5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -127,6 +130,19 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +        // Profiles can not have config clashes.
    +        if ($module != drupal_get_profile()) {
    

    I first read "profiles can not have config clashes" as meaning "ensure that profiles don't have config clashes", but apparently this is saying that we only need to deal with actual modules, and not with install profiles, which are treated internally as modules also but cannot have config clashes by definition.

  6. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -127,6 +130,19 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +          // Validate default configuration of this module. Bail if unable to
    +          // install. Should not continue installing more modules because those
    +          // may depend on this one.
    

    For the UX, it'd be ideal if this would be checked *before* actually kicking of the module installation logic, and warning the user immediately.

    Why is this necessary if ModulesListForm::submitForm() checks this already? I'm guessing it's necessary for e.g. Drush, i.e. when not using the UI, but the API, to install modules?

  7. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    @@ -0,0 +1,164 @@
    +   * @param \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface $key_value_expirable
    +   *   The key value expirable factory.
    +   * @param \Drupal\Core\Config\ConfigManagerInterface $config_manager
    +   *   The configuration manager.
    +   */
    +  public function __construct(KeyValueStoreExpirableInterface $key_value_expirable, ConfigManagerInterface $config_manager, ConfigInstallerInterface $config_installer) {
    

    Third parameter missing in docblock.

  8. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    @@ -0,0 +1,164 @@
    +            // Super weird.
    

    :)

  9. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -496,6 +508,19 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +    // Check if we have any pre existing configuration.
    

    Nit: s/pre existing/pre-existing/

    ?

  10. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -496,6 +508,19 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      $this->keyValueExpirable->setWithExpire($account, $modules, 60);
    

    This is weird, but I think this is part of the hack you mention in ConfigClashController::moduleReport(), which probably still needs to be resolved as you say in #85.

  11. +++ b/core/modules/system/system.routing.yml
    @@ -285,6 +285,14 @@ system.theme_install:
    +    _content: 'Drupal\system\Controller\ConfigClashController::moduleReport'
    

    _controller :)

xjm’s picture

Issue tags: -needs te +Needs tests, +Triaged D8 critical

needs te indeed.

YesCT’s picture

Issue summary: View changes
Issue tags: +blocker, +Needs reroll

adding blocker tag, since this blocks #2090115: Don't install a module when its default configuration has unmet dependencies, so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2090115.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.27 KB
28.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,060 pass(es). View

New patch - response to #86 on its way...

alexpott’s picture

re #86

  1. This comment is not about what is already installed this is about not adding duplicates to the list to install
  2. Fixed missing period - not sure about the other suggestion
  3. Fixed
  4. Fixed
  5. Improved the comment
  6. There are two reasons, yep Drush is one of them, the other is more tricky - configuration can be created programatically during an install - therefore when installing multiple modules we need to check again after each installation.
  7. Fixed
  8. This would occur if there was a simple config clash - I'm completely unsure of what to do here. This should no be possible but we have no way of preventing this. There is nothing we can do to stop Stupid Module from having a config file called system.site.yml in its default configuration - but we definitely need to prevent this module from installing.
  9. Fixed
  10. This is the same way we pass the list of modules to the module install confirm form
  11. Fixed
swentel’s picture

Issue tags: +Ghent DA sprint

Tagging - looking at the UI and tests.

dawehner’s picture

Status: Needs review » Needs work

Let's be honest.

swentel’s picture

Status: Needs work » Needs review
FileSize
35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,064 pass(es). View
12.42 KB

Did some cleanups and added tests in ConfigInstallWebTest.

The UI now also catches the exception and shows a basic message. Depending on how you toggle modules in the UI, different things happen:

  • Toggle config test and config install fail test: Then submitForm() in ModulesListForm is called and an exception is thrown and catched with config test enabled.
  • Toggle config install fail test: you are redirect to ModulesListConfirmForm because of the dependency and then submitForm() in this class installs the modules. Exceptions is thrown and catched, with config test enabled.
  • Toggle config install fail test when config test is already enabled: you go to the clash screen.

One consequence is that the check for preexisting configuration needs to happen in both submit methods which is kind of stupid right now.

Also should we check on pre existing configuration on new modules or not ?

swentel’s picture

Added test scenario's and screenshots to summary.

swentel’s picture

Issue summary: View changes
pfrenssen’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -7,6 +7,7 @@
    +use Drupal\Component\Utility\String;
     use Drupal\Component\Utility\Unicode;
    

    This is not actually used.

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -128,6 +131,20 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +        // Install profiles can not have config clashes as if they contain
    +        // configuration with the same name as a module it will override it.
    +        if ($module != drupal_get_profile()) {
    +          // Validate default configuration of this module. Bail if unable to
    +          // install. Should not continue installing more modules because those
    +          // may depend on this one.
    +          $existing_configuration = \Drupal::service('config.installer')->findPreExistingConfiguration('module', $module);
    +          if (count($existing_configuration)) {
    +            throw new PreExistingConfigException(String::format('Configuration @config_names provided by @extension already exist in active configuration',
    +              array('@config_names' => implode(',', $existing_configuration[StorageInterface::DEFAULT_COLLECTION]), '@extension' => $module)
    +            ));
    +          }
    +        }
    

    This documentation is initially a bit hard to understand, unless you already know that we are going to hunt for config clashes, and that this is the only thing that will cause configuration to be considered invalid.

    I also think !empty() would be better than count() here.

  3. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -18,9 +19,17 @@
    +  protected $admin_user;
    +
    

    According to coding standards, object properties should be lowercamelcased. This is not strictly followed in core though.

  4. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -29,7 +38,7 @@ protected function setUp() {
    -  function testIntegrationModuleReinstallation() {
    +  function _testIntegrationModuleReinstallation() {
    

    s/_//

  5. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -87,16 +106,16 @@ function testIntegrationModuleReinstallation() {
    -  function testInstallProfileConfigOverwrite() {
    +  function _testInstallProfileConfigOverwrite() {
    

    s/_//

  6. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -146,4 +165,25 @@ function testInstallProfileConfigOverwrite() {
    +  /**
    +   * Tests pre existing configuration detection.
    +   */
    

    "pre-existing" instead of "pre existing".

  7. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -146,4 +165,25 @@ function testInstallProfileConfigOverwrite() {
    +    // Try to install config_install_fail_test without config_test enabled.
    +    $this->drupalPostForm('admin/modules', array('modules[Testing][config_install_fail_test][enable]' => TRUE), t('Save configuration'));
    +    $this->drupalPostForm(NULL, array(), t('Continue'));
    +    $this->assertText('Configuration config_test.dynamic.dotted.default provided by config_install_fail_test already exist in active configuration');
    

    This can only work if config_install_fail_test depends on config_test. I looked this up in the info.yml file to verify an this is indeed the case. Maybe explain this here to avoid confusion?

  8. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -146,4 +165,25 @@ function testInstallProfileConfigOverwrite() {
    +    // Try to install config_install_fail_dependency_test.
    +    $this->drupalPostForm('admin/modules', array('modules[Testing][config_install_fail_dependency_test][enable]' => TRUE), t('Save configuration'));
    

    config_install_fail_dependency_test depends on config_install_fail_test which depends on config_test. This is well tested :)
    Also worth a line of documentation.

Got half way through the review. Chimay Blue is clouding my judgement so I better stop :P

dawehner’s picture

Status: Needs review » Needs work

Back to needs work.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -329,4 +331,32 @@ public function setSyncing($status) {
    +    $extension_config = $this->configFactory->get('core.extension');
    +    $enabled_extensions = array_keys((array) $extension_config->get('module'));
    +    $enabled_extensions += array_keys((array) $extension_config->get('theme'));
    +    // Add the extensions that will be enabled to the list of enabled
    +    // extensions.
    

    Don't we have to take into account install profiles here as well?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -110,6 +110,19 @@ public function getEntityTypeIdByName($name) {
    +  public function loadConfigEntityByName($name) {
    +    $entity_type_id = $this->getEntityTypeIdByName($name);
    +    if ($entity_type_id) {
    +      $entity_type = $this->entityManager->getDefinition($entity_type_id);
    +      $id = substr($name, strlen($entity_type->getConfigPrefix()) + 1);
    +      return $this->entityManager->getStorage($entity_type_id)->load($id);
    +    }
    +    return NULL;
    +  }
    +
    

    Note: This code is pretty much exactly the same as in ::findConfigEntityDependentsAsEntities() so I would argue we should better replace the usage there. The only difference is that the other implementation supports multi-loading.

  3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -128,6 +131,20 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +          $existing_configuration = \Drupal::service('config.installer')->findPreExistingConfiguration('module', $module);
    

    Note: In that context you certainly have $config_installer available as variable.

  4. +++ b/core/modules/system/system.routing.yml
    @@ -285,6 +285,14 @@ system.theme_install:
    +system.modules_config_clash:
    +  path: 'admin/modules/config-clash'
    +  defaults:
    +    _controller: 'Drupal\system\Controller\ConfigClashController::moduleReport'
    +    _title: 'Configuration clash'
    +  requirements:
    +    _permission: 'administer modules'
    +
    

    It is a little bit odd that this is not part of the config module, but well, it will otherwise require the config module to solve some of those conflicts.

babruix’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
34.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,589 pass(es), 1 fail(s), and 0 exception(s). View
3.05 KB

Rerolled patch from #93.
Applied proposed changes from comments #96 and #98.

Status: Needs review » Needs work

The last submitted patch, 99: 2140511-99.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,566 pass(es). View
0 bytes

Added back try-catch block around $this->moduleInstaller->install.

webchick’s picture

Status: Needs review » Needs work

Aw, shoot. It's a 0 byte patch. :( Any chance you've still got the changes laying around?

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.78 KB
38.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,949 pass(es), 1 fail(s), and 0 exception(s). View

Improved the exception object so we can do a translated version in the UI and improved the test.

We also need to worry about themes :(

I've also checked that the comments in #96 and #98 (thank you @pfrenssen and @dawehner) where addressed in #99 and they are. The interdiff in #99 does not show the full scope of the changes made.

Status: Needs review » Needs work

The last submitted patch, 103: 2140511-2.103.patch, failed testing.

Wim Leers’s picture

Overall, this looks great!

Code review

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -329,4 +330,32 @@ public function setSyncing($status) {
    +    // Add the extensions that will be enabled to the list of enabled
    +    // extensions.
    +    $enabled_extensions[] = $name;
    

    s/extensions/extension/

  2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -110,6 +110,19 @@ public function getEntityTypeIdByName($name) {
    +  public function loadConfigEntityByName($name) {
    +    $entity_type_id = $this->getEntityTypeIdByName($name);
    +    if ($entity_type_id) {
    +      $entity_type = $this->entityManager->getDefinition($entity_type_id);
    +      $id = substr($name, strlen($entity_type->getConfigPrefix()) + 1);
    +      return $this->entityManager->getStorage($entity_type_id)->load($id);
    +    }
    +    return NULL;
    +  }
    

    Elegant :)

  3. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -24,6 +24,17 @@
    +   * Loads a configuration entity from the configuration name.
    

    s/from/by/, to match the function name?

  4. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
    @@ -0,0 +1,73 @@
    +  protected $module;
    ...
    +  public function getModule() {
    ...
    +   * Creates an exception for a module name and a list of configuration objects.
    ...
    +      array('@config_names' => implode(', ', $config_objects), '@extension' => $module)
    

    s/module/extension/

  5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -149,6 +151,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +        // Install profiles can not have config clashes as if they contain
    

    s/as if/if

  6. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -149,6 +151,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +        // configuration with the same name as a module it will override it.
    

    s/as a module it will override it/because it would be overridden/

    ?

  7. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -149,4 +167,53 @@ function testInstallProfileConfigOverwrite() {
    +    // Try to install config_install_fail_test and config_test. This installs
    

    "This" === config_install_fail_test. Not as clear as it could be in the comment.

    EDIT: it becomes clear 12 lines further though.

  8. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -149,4 +167,53 @@ function testInstallProfileConfigOverwrite() {
    +    // configuration clash before installation so the user will be taken to...
    

    "to..." -> unfinished?

  9. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -149,4 +167,53 @@ function testInstallProfileConfigOverwrite() {
    +  }
     }
    

    Should have a blank line in between them.

  10. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    @@ -0,0 +1,176 @@
    +  /**
    +   * The module handler service.
    +   *
    +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
    +   */
    +  protected $moduleHandler;
    ...
    +  /**
    +   * An associative list of modules to enable or disable.
    +   *
    +   * @var array
    +   */
    +  protected $modules = array();
    

    Unused; probably copy/paste leftover.

  11. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    @@ -0,0 +1,176 @@
    +   * @param \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface $key_value_expirable
    +   *   The key value expirable factory.
    

    This doesn't match; you don't get the factory, but a specific store.

    It's correct in the class property docs.

  12. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    @@ -0,0 +1,176 @@
    +            // @todo
    

    ?

  13. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    index 24f1b4b..d7234f4 100644
    --- a/core/modules/system/src/Form/ModulesListConfirmForm.php
    
    +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
    index 2486ca4..8347543 100644
    --- a/core/modules/system/src/Form/ModulesListForm.php
    

    There's a lot of code duplication between these two. Can we change it so it's DRY? Or why don't we want that? Because it's out of scope?

  14. +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
    @@ -151,13 +164,37 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      $this->keyValueExpirable->setWithExpire($account, $this->modules, 60);
    
    +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -496,6 +509,19 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      $this->keyValueExpirable->setWithExpire($account, $modules, 60);
    

    Why 60 seconds? Why not 5, why not 300?

  15. +++ b/core/modules/system/system.module
    @@ -145,6 +145,9 @@ function system_help($route_name, RouteMatchInterface $route_match) {
    +      return '<p>' . t('Here you can find a list of configuration that already exists. You need to either rename it or delete it before installing modules which have configuration with the same name.') . '</p>';
    

    "list of configuration" versus "it"

Manual testing

Having tried this, I now understand the 60-second caching (upon trying to install a module and a config clash being detected, the user is redirected to admin/modules/config-clash); but I don't understand why you did it this way. Why not make it part of the configuration form's output? Why a separate route? Because that's the only reason why there's this (IMHO awkward) 60-second caching AFAICT.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
37.47 KB
37.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,960 pass(es), 1 fail(s), and 0 exception(s). View

I've removed the UI completely since deciding what to show people for configuration overrides that clash is impossible. We might consider implementing a UI in a folllow-up issue. Doing that fixes quite a few of the points raised by @Wim Leers in #105. All the relevant feedback is also addressed in the attached patch.

I've implemented the check for themes too.

There are extensive tests for this change in ConfigInstallWebTest and ConfigInstallTest.

Status: Needs review » Needs work

The last submitted patch, 106: 2140511-2.106.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

The TourTest fail is interesting. We inject the active configuration into the ExtensionInstallStorage. If we use the ConfigInstaller to find pre-existing configuration first then to an install and there are collections then the wrong active configuration injected.

alexpott’s picture

FileSize
5.23 KB
41.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,964 pass(es). View

Now for the patch :)

Wim Leers’s picture

Status: Needs review » Needs work

#108: great find! And simple fix in #109.

I can only find nitpicks this time.

  1. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -175,45 +179,51 @@
    +    // The user is shown a confirm form becuase the config_test module is a
    

    s/becuause/because/

  2. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -175,45 +179,51 @@
    +      ->set('label', 'Je suis Charlie')
    

    :)

  3. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -175,45 +179,51 @@
    +    $this->assertRaw('Unable to install Configuration install fail test, <em class="placeholder">config_test.dynamic.dotted.default, language/fr/config_test.dynamic.dotted.default</em> already exist in active configuration.');
    

    Great expansion of this patch! Very valuable information indeed.

  4. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -175,45 +179,51 @@
    +    // Test installing a theme through UI that has existing configuration.
    

    s/through UI/through the UI/

  5. +++ b/core/modules/config/tests/config_collection_clash_install_test/config/install/another_collection/config_collection_install_test.test.yml
    @@ -0,0 +1 @@
    \ No newline at end of file
    
    +++ b/core/modules/config/tests/config_collection_clash_install_test/config/install/collection/test1/config_collection_install_test.test.yml
    @@ -0,0 +1 @@
    \ No newline at end of file
    
    +++ b/core/modules/config/tests/config_collection_clash_install_test/config/install/collection/test2/config_collection_install_test.test.yml
    @@ -0,0 +1 @@
    \ No newline at end of file
    

    .

  6. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -517,7 +518,24 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    diff --git a/core/modules/system/src/Tests/Module/InstallUninstallTest.php b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
    
    diff --git a/core/modules/system/src/Tests/Module/InstallUninstallTest.php b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
    index 125b754..a2c8cd1 100644
    
    index 125b754..a2c8cd1 100644
    --- a/core/modules/system/src/Tests/Module/InstallUninstallTest.php
    
    --- a/core/modules/system/src/Tests/Module/InstallUninstallTest.php
    +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
    
    +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
    +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
    @@ -7,7 +7,10 @@
    
    @@ -7,7 +7,10 @@
     
     namespace Drupal\system\Tests\Module;
     
    +use Drupal\Core\Entity\Entity\EntityViewMode;
    +use Drupal\Core\Field\Entity\BaseFieldOverride;
     use Drupal\Core\Logger\RfcLogLevel;
    +use Drupal\node\Entity\NodeType;
     
     /**
      * Install/uninstall core module and confirm table creation/deletion.
    diff --git a/core/modules/system/system.module b/core/modules/system/system.module
    
    diff --git a/core/modules/system/system.module b/core/modules/system/system.module
    index 255449e..5b30997 100644
    
    index 255449e..5b30997 100644
    --- a/core/modules/system/system.module
    
    --- a/core/modules/system/system.module
    +++ b/core/modules/system/system.module
    
    +++ b/core/modules/system/system.module
    +++ b/core/modules/system/system.module
    @@ -145,6 +145,7 @@ function system_help($route_name, RouteMatchInterface $route_match) {
    
    @@ -145,6 +145,7 @@ function system_help($route_name, RouteMatchInterface $route_match) {
     
         case 'system.status':
           return '<p>' . t("Here you can find a short overview of your site's parameters as well as any problems detected with your installation. It may be useful to copy and paste this information into support requests filed on drupal.org's support forums and project issue queues. Before filing a support request, ensure that your web server meets the <a href=\"@system-requirements\">system requirements.</a>", array('@system-requirements' => 'http://drupal.org/requirements')) . '</p>';
    +
       }
     }
    

    These are now unnecessary/unintentional changes.

  7. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
    @@ -149,4 +171,59 @@ function testInstallProfileConfigOverwrite() {
    +    // Try to install config_install_fail_test without selecting config_test.
    +    // The user is shown a confirm form becuase the config_test module is a
    +    // dependency.
    ...
    +    // Test that collection configuration clashes are reported correctly.
    ...
    +    // Test installing a theme through UI that has existing configuration.
    ...
    +    // Theme install through the API.
    

    Ideally the symmetry here would also be reflected in the comments.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.9 KB
42.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,993 pass(es). View

Thanks @Wim Leers

  1. Fixed
  2. :)
  3. Yep
  4. Fixed
  5. Fixed
  6. Fixed
  7. Fixed

Also spotted some unnecessary testing in ConfigInstallWebTest::testInstallProfileConfigOverwrite()

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Went over it one more time. Found two very very silly nitpicks, that can easily be fixed upon commit. Patch looks great :)

  1. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
    @@ -0,0 +1,102 @@
    +  public static function create($extension, array $config_objects) {
    +
    +    $message = String::format('Configuration objects (@config_names) provided by @extension already exist in active configuration',
    

    Unnecessary blank line.

  2. +++ b/core/modules/config/tests/config_clash_test_theme/config_clash_test_theme.info.yml
    @@ -0,0 +1,11 @@
    +#
    

    This is a copy/paste error; the other test extensions' info files don't have a comment. Can be deleted.

alexpott’s picture

FileSize
1.23 KB
42.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,989 pass(es). View
catch’s picture

Status: Reviewed & tested by the community » Needs review

No major complaints but two questions on things that confused me a bit.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -79,7 +79,7 @@ class ConfigInstaller implements ConfigInstallerInterface {
    @@ -155,16 +155,17 @@ protected function listDefaultConfigCollection($collection, $type, $name, array
    

    There's a lot of code shared between this and ConfigInstaller::installCollectionDefaultConfig().

    And ConfigInstaller::findPreExistingConfiguration() als does some similar things as installCollectionDefaultConfig(). Any way some of that code can be factored out?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -329,4 +330,31 @@ public function setSyncing($status) {
    +  public function findPreExistingConfiguration($type, $name) {
    

    So just to confirm findPreExistingConfiguration() includes any configuration shipped by install profiles?

  3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -149,6 +151,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +          $existing_configuration = $config_installer->findPreExistingConfiguration('module', $module);
    

    OK so install profiles are skipped before this code runs. But install profiles are included in findPreExistingConfiguration().

    I'm not really clear from this what happens in the following cases:

    1. Install profile and module are installed at the same time.

    2. Install profile is installed first, then module is enabled later.

    3. Install profile is installed with module at the same time. Module is uninstalled. Module is re-installed.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.76 KB
43.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,999 pass(es). View
  1. I don't think there is that much code that is shared between them all and they have different tasks:
    • For a given extension findPreExistingConfiguration looks in the active storage and compares the do what will be installed
    • For a given extension listDefaultConfigCollection lists default configuration for an extension that is available to install
    • For a given collection installCollectionDefaultConfig installs all of the available configuration in enabled extensions' config/install directories

    Yes they look similar but there are subtle differences that make refactoring to share code harder work and more verbose. I've renamed listDefaultConfigCollection to be listDefaultConfigToInstall since that more accurately describes what it does.

  2. Install profiles can contain clashing configuration this is okay because install profiles can only be installed by the installer. During installation if the profile contains a matching config file to something that is being installed that we use the configuration provided by the install profile. This is how we use Standard's system.theme and system.cron configuration. Once installation is complete and we do regular module install not during a module install then the module's default configuration is used regardless of what is in the install profile.
  3. Answers to @catch's scenarios
    1. The profile's config is used.
    2. A profile should not contain configuration for a module it does not depend on - but if it did then the module's config would be used.
    3. The profile's config is used during install. It will be removed by the uninstall. The module's will be used during the re-install.

    Some of this is tested in ConfigInstallWebTest::testInstallProfileConfigOverwrite() but it could be improved - this is functionality is not introduced by this issue so I think this is a separate issue.

Tentatively setting back to rtbc because all I've done is rename a method.

webchick’s picture

Assigned: Unassigned » catch

Boing!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Hmm OK. I'd somewhat assumed that profile overrides would continue to work if a module is uninstalled and re-installed, but quite happy this isn't the case.

Committed/pushed to 8.0.x, thanks!

  • catch committed 6efd90c on 8.0.x
    Issue #2140511 by alexpott, Gábor Hojtsy, swentel, babruix,...

Status: Fixed » Closed (fixed)

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

jhedstrom’s picture