Problem/Motivation

Default configuration entities could depend on a module that is not installed.

Proposed resolution

The patch attached takes the following approach:

  1. All configuration in config/install has to have its dependencies met during install - if it does not then you get an unmet dependencies error.
  2. Optional configuration is moved to config/optional. During an extension's installation all configuration objects, whose dependencies can be met, are installed. This is a change from HEAD where we assume that config in config/install is optional if the config entity provider is not installed.
  3. When you install an extension that provides a config entity type (eg. views), all config/optional directories are searched for potential configuration to create. For example, when you install views, all views contained any installed extension's config/optional directories whose dependencies can be met are created.

As a result of these changes we can simplify a lot of the logic in ConfigInstaller because there is less magic detection of configuration to install. Also we get a nice performance boost in the installer because we are only searching optional configuration once per install rather than searching all the config/install directories for all installed modules during every module installation.

Steps to test

  1. Install minimal
  2. Uninstall the node module
  3. If testing the patch: move config/optional/views.view.frontpage.yml to config/install/views.view.frontpage.yml
  4. Change the views.view.frontpage.yml (provided by node module) to have the following dependencies key:
    dependencies:
      module:
        - node
        - user
      enforced:
        module:
          - telephone
    

    This creates an unmet dependency

  5. Install node - With the patch, this should fail and an error message appear indicating that a dependency is missing for the view. With HEAD this will work
  6. Install views - this should work.
  7. Install node - With the patch, this should fail and an error message appear indicating that a dependency is missing for the view. With HEAD this will work and create a view that has unmet dependencies.
  8. Install telephone
  9. Install node - With the patch, this is work fine.

Remaining tasks

Review patch
Commit

User interface changes

Message to user about missing dependencies for configuration.

API changes

  • Scan EXTENSION/config/optional for optional configuration to install during module installation
  • Change ConfigInstaller::findPreExistingConfiguration() to be the more generic => ConfigInstaller::checkConfigurationToInstall()
  • Add a new install step that installs an install profile (install_install_profile). This is so that themes can installed before the profile so that the profile can provide configuration that depends in the theme.
  • Add ConfigInstaller::installOptionalConfig() to enable installation of optional configuration.

Postponed until

#2140511: Configuration file name collisions silently ignored for default configuration

Original report by @catch

Found by alexpott on https://drupal.org/node/2003966#comment-7866263

Problem
- Module A provides some default configuration, to be enabled when module B is enabled.

- The default configuration references a plugin owned by module C

- Module C is not enabled when either module A or module B are

- The default configuration is installed, and is immediately broken - in Views it'd be a broken handler, some other systems can't handle it at all.

We should probably not install the configuration, if it's going to be invalid as soon as it's installed. That'd make this one a counterpart to #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall - certainly the validation in there is going to be necessary for whatever happens here.

A couple of options then:

- skip the configuration install silently. This brings up the question of whether that configuration will ever get installed (i.e. if module C is enabled we're not going to look for configuration files in module A just in case they have a plugin owned by module C.

- prompt the person enabling the module to see if they'd also like to enable module C at the same time, then they can answer yes or know and we forget about this config file after that.

CommentFileSizeAuthor
#117 2090115-opt.117.patch100.38 KBalexpott
#117 114-117-interdiff.txt4.48 KBalexpott
#114 2090115-opt.114.patch97.25 KBalexpott
#114 111-114-interdiff.txt851 bytesalexpott
#111 2090115-opt.111.patch97.22 KBalexpott
#111 108-111-interdiff.txt1.37 KBalexpott
#108 2090115-opt.108.patch95.56 KBalexpott
#108 107-108-interdiff.txt1.34 KBalexpott
#107 2090115-opt.107.patch95.64 KBalexpott
#107 105-107-interdiff.txt10.33 KBalexpott
#105 2090115-opt.105.patch87.84 KBalexpott
#105 103-105-interdiff.txt1.98 KBalexpott
#36 2090115.36.patch26.77 KBalexpott
#39 36-39-interdiff.txt6.99 KBalexpott
#39 2090115.39.patch31.57 KBalexpott
#40 39-40-interdiff.txt1.19 KBalexpott
#40 2090115.40.patch30.88 KBalexpott
#43 40-43-interdiff.txt3.31 KBalexpott
#43 2090115.43.patch31.54 KBalexpott
#45 43-45-interdiff.txt507 bytesalexpott
#45 2090115.45.patch32.04 KBalexpott
#48 45-48-interdiff.txt9.4 KBalexpott
#48 2090115.48.patch38.9 KBalexpott
#50 48-50-interdiff.txt1.25 KBalexpott
#50 2090115.50.patch39.27 KBalexpott
#53 install.png89.08 KBcafuego
#53 modules.png158.04 KBcafuego
#53 views.png54.13 KBcafuego
#53 front.png82.52 KBcafuego
#63 50-63-interdiff.txt7.61 KBalexpott
#63 2090115.63.patch40.92 KBalexpott
#65 63-65-interdiff.txt451 bytesalexpott
#65 2090115.65.patch41.36 KBalexpott
#74 anode-first.patch42.78 KBxjm
#74 anode-first-interdiff.txt1.42 KBxjm
#74 anode-last.patch42.79 KBxjm
#74 anode-last-interdiff.txt1.43 KBxjm
#74 znode-first.patch42.79 KBxjm
#74 znode-first-interdiff.txt1.43 KBxjm
#74 znode-last.patch42.78 KBxjm
#74 znode-last-interdiff.txt1.42 KBxjm
#85 2090115-opt.85.patch69.3 KBalexpott
#87 85-87-interdiff.txt4.88 KBalexpott
#87 2090115-opt.87.patch69 KBalexpott
#89 87-89-interdiff.txt2.31 KBalexpott
#89 2090115-opt.89.patch66.93 KBalexpott
#95 2090115-opt.95.patch70.13 KBalexpott
#95 89-95-interdiff.txt8.86 KBalexpott
#98 95-98-interdiff.txt27.58 KBalexpott
#98 2090115-opt.98.patch73.6 KBalexpott
#103 98-103-interdiff.txt18.37 KBalexpott
#103 2090115-opt.103.patch87.74 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Postponed » Active

This doesn't have a hard-dependency on the other issue, but it has a hard-dependency on the API. We might want to open a new issue for adding the dependency API and implementing it, then the install and uninstall work could happen in parallel based on that. Un-postponing for now.

catch’s picture

Status: Active » Postponed
Issue tags: +blocked

OK we slightly re-purposed that issue to explicitly include the API and one implementation, so this does have a hard dependency now...

catch’s picture

Issue summary: View changes
Issue tags: +beta blocker
xjm’s picture

Issue tags: +Configuration system
xjm’s picture

jessebeach’s picture

Title: Don't install configuration when it has a soft-dependency on a module that's not installed » [META-2] Don't install configuration when it has a soft-dependency on a module that's not installed
Issue summary: View changes
jessebeach’s picture

Title: [META-2] Don't install configuration when it has a soft-dependency on a module that's not installed » [PP-2] Don't install configuration when it has a soft-dependency on a module that's not installed
jessebeach’s picture

Title: [PP-2] Don't install configuration when it has a soft-dependency on a module that's not installed » [PP-1] Don't install configuration when it has a soft-dependency on a module that's not installed

One postponing issue down. The other postponer is no longer, itself, postponed.

jessebeach’s picture

Status: Postponed » Active

Unpostponed!

jessebeach’s picture

Title: [PP-1] Don't install configuration when it has a soft-dependency on a module that's not installed » Don't install configuration when it has a soft-dependency on a module that's not installed
xjm’s picture

Issue tags: +VDC
alexpott’s picture

I'm not 100% certain that we should actually provide general support for optional dependencies - Views has its own system but how each config entity handles this will could be different. Without views handling of optional dependencies then and if we take the approach proposed in the issue summary

We should probably not install the configuration, if it's going to be invalid as soon as it's installed.

then we'd never install the content management view since this has an optional dependency on the content_tranlsation module.

I think we should just let each config entity do its own thing. If a module provides a default config entity that has dependencies on multiple modules and it will not work without those dependencies being met then the module needs to declare those dependencies in it's info.yml file. For example, if a custom module provides a view that also depends on the node module then the module needs to depend on node. Which means if views is installed then the default view will be installed and will work.

catch’s picture

then we'd never install the content management view since this has an optional dependency on the content_tranlsation module.

That seems like the right behaviour. I think it's the view that's wrong here. Or in this case, Views would have to remove content_translation from the dependencies stored with the view itself, then handle the 'optional dependency' itself. But the default behaviour would be to check those dependencies and tough if they're not met.

xjm’s picture

I disagree with #13; it's a design behavior in Views, and valuable. The views concept of optional handlers allows us to ship with support for translation that doesn't break your view when you don't have a multilingual site. Unless I'm misunderstanding?

xjm’s picture

catch’s picture

#13 wasn't very well put. I'm a bit concerned about optional handlers in general, because of #2212081: Remove views optional handler handling but that's a bit different.

In the case of this issue, I think we should enforce that you can't have unmet soft-dependencies in active configuration, and it should be up to Views to implement optional handlers as an exception to that rule.

So a view with no optional handlers, but with dependencies on missing modules/configuration entities should not be installed at all. That could either mean throwing an exception or just not installing - not entirely sure where I stand on the latter choice.

xjm’s picture

xjm’s picture

Issue tags: -blocked
alexpott’s picture

If a module wants to provide configuration that depends on modules it does not itself depend on then it should just create a sub module that does to provide the configuration.

If we want we can add validation to the module installer after #2140511: Configuration file name collisions silently ignored for default configuration goes in since that adds a config validation step to the module install process. As this does not change APIs or data structures de-betablockering and postponing on said issue. Making a beta target as we might decide that the additional validation will help module developers.

xjm’s picture

Title: Don't install configuration when it has a soft-dependency on a module that's not installed » Don't install a module when its default configuration has unmet dependencies
Issue summary: View changes
Issue tags: +Needs issue summary update

Discussed with @catch @alexpott @effulgentsia and @tim.plunkett. We agreed that we are not going to support soft dependencies -- either you declare a hard dependency, or you don't have one.

#2140511: Configuration file name collisions silently ignored for default configuration adds a validation step to the module install that we can use, so we are postponing this issue on that.

Berdir’s picture

Hm, interesting, while I can see that it wouldn't really work anyway, I am wondering how to support use cases like the one we will have in the Monitoring (https://drupal.org/project/monitoring) project:

In 7.x, we had had hooks to define sensors and a lot of integration with core and contrib modules, we had our own pattern to implement hooks for them, similar to how rules/token and other modules do it. They were only called if the module was enabled and the sensors we provided for it automatically existed.

In 8.x, we're working on converting them to a plugin/config entity approach (GSoC project) to be able to manage/create/edit them in more flexible ways, like the UI.

So the question is, how are we going to provide our integration with third party modules now? I can see that just putting them in the config/install folder now is not a solution either because they will only be installed if the module is already installed and won't be picked up later. So yes, that wouldn't work anyway. But what can work?

If a module wants to provide configuration that depends on modules it does not itself depend on then it should just create a sub module that does to provide the configuration.

Yeah, that, but based on a quick look at our hooks file, that would result in 10+ submodules already, containing nothing more but a bunch of default configuration? Maybe less so in 8.x, but count($modules) is still a performance factor and it's also more complicated for users as they will have to remember to enable all those modules. I'd rather come up with a custom pattern to look into a config/$module folder in hook_install/hook_modules_installed() and copy to active than providing so many submodules.

We do officially support this for plugins for example, if you set provider = 'other_module', then it will respect that and only expose that plugin of that module is installed.

Any suggestions?

catch’s picture

Yeah, that, but based on a quick look at our hooks file, that would result in 10+ submodules already, containing nothing more but a bunch of default configuration? Maybe less so in 8.x, but count($modules) is still a performance factor and it's also more complicated for users as they will have to remember to enable all those modules.

Now that we don't require .module files, if we do #2233261: Deprecate hook_hook_info() then I think we could possibly optimize the module handler to not bother looking for hook implementations in those modules. Wouldn't that be most of the performance implication from lots of modules gone? There's still initial discovery of course.

I'd rather come up with a custom pattern to look into a config/$module folder in hook_install/hook_modules_installed() and copy to active than providing so many

Something like that sounds OK too, although you could have configuration with multiple other dependencies, not sure it works for that.

catch’s picture

Issue tags: +D8 upgrade path
catch’s picture

Title: Don't install a module when its default configuration has unmet dependencies » [PP-2] Don't install a module when its default configuration has unmet dependencies
alexpott’s picture

Title: [PP-2] Don't install a module when its default configuration has unmet dependencies » [PP-1] Don't install a module when its default configuration has unmet dependencies

not anymore

xjm’s picture

Issue tags: +Triaged D8 critical
YesCT’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Re #21, #22: the answer to default config that depends on other modules has been to add a submodule in every other issue I have seen. The active configuration is not supposed to contain config that does not fully have owners (with all its components). I agree with @catch that by making the existence of these modules less of a problem sounds better than trying to work around this in config because this has been the assumption in config for the whole time.

Berdir’s picture

I still think that this is a bad idea and would result in bad DX and UX.

Take tour.module as an example. Are you really suggesting that people have to add a mymodule_tour submodule, just to provide a default tour for their UI? And expect that users enable 20 random tour submodules, so they have tours for their site?

Another example, imagine the help config entity issue would have happened, so hook_help() would have become default config entities. That would mean that modules are required to provide a mymodule_help submodule to provide the default help that we require them to do ?

We have many, many config entities now, and a lot of them are going to be frequently used for optional integration with other modules.

We have the provider filter system so that modules can optionally provide plugin implementations based on the existence of others. Obviously modules can implement hooks for modules that are not installed, they can also provide plugin implementations for plugin types defined by optional modules, modules can even define routes and have them depend on the existence of another module. The only non-trivial thing right now are services, which require a ServiceProvider class.

Why not support a system for optional default configuration as well?

I've implemented what I talked about in #21 for monitoring, see http://cgit.drupalcode.org/monitoring/tree/monitoring.module?h=8.x-1.x#n377 and http://cgit.drupalcode.org/monitoring/tree/config/optional?h=8.x-1.x. I'm sure it is incomplete, but so far, it is working quit well. I guess one problem is the special case that I have there right now in case monitoring itself is installed, but we can bake that into ConfigInstaller I think, just like we already look for config that starts with that module name in other modules.

alexpott’s picture

re #29 I was never thinking that this issue would require a sub module for a tour. this case is already covered by the installer.

For me this issue was always about what if my module provides a view that depends on my module, views and some other module that my module does not depend on. If views is installed when I install my module and the other module is not - what happens then?

Berdir’s picture

Ok, we talked a bit about this in IRC.

So according to @alexpott, support for provider-based optional config is not supposed to be removed. We would only validate configuration that we expect to be installable.

Which means that a part of the default configuration is going to be silently ignored anyway. And conceptually, there is not much difference between default configuration that has a dependency through the provider prefix or 1-n module dependencies (I'm not sure about config dependencies, though, those are... different). So, I think there would not be a problem to silently skip default configuration where at least one dependency is not fulfilled.

The difference/problem is that it is much harder to find not yet installed default config that we could now maybe install with the new module if that dependency is hidden in the dependency list in the file as opposed to the convenient provider prefix (which we already support and do exactly that). If we can find a way to do that that is not super-slow ( does not need to be super-fast either, because install).

There are some ways to do that, starting with my simple approach that only supports a single module dependency, over parsing default config that we don't know yet and look for what is needed by the new module and has no additional dependency, or even maintaining some sort of storage that keeps track of default config with missing dependencies in an efficient way.

We could also consider that such optional default configuration needs to live in a separate folder even when not going with my implementation, simply to make it clear that code may not depend on those configurations, as it might or might not exist. And we should probably prevent the installation in case we have a missing entity dependency, because keeping track of those would be too complex and probably not desirable.

alexpott’s picture

Title: [PP-1] Don't install a module when its default configuration has unmet dependencies » Don't install a module when its default configuration has unmet dependencies
Status: Postponed » Active
alexpott’s picture

An interesting scenario is this. If you enable views it will install all the views provided by any of the currently installed modules. What happens if one of the views provided by one of this other modules has an unmet dependency? Stopping views from being installed feels wrong. Creating the view is wrong.

Gábor Hojtsy’s picture

IMHO it should skip importing that view. If that view has one display that requires a 3rd party (uninstalled) module, it will be a pretty broken view. I know its too much to ask, but ideally on each module install config would go through shipped config files and see if there are config files that were not imported and are now satisfied. Of course then you may delete something it would be imported again. So to protect from that, the system could check that in this case it only finds config that does require the new component, not just flat out new :) OTOH I think that sounds too much to expect maybe. Lots of dependency magic :)

Berdir’s picture

That is pretty much what I wrote, I think :)

Going to work a bit on this.

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
26.77 KB

oops @Berdir - I started some preliminary work on this last night - I should have posted a comment - my bad.

Status: Needs review » Needs work

The last submitted patch, 36: 2090115.36.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -221,6 +243,13 @@ protected function createConfiguration($collection, array $config_to_install) {
+        // All the configuration that is provided by the extension will have
+        // been dependency checked already. But configuration provided by other
+        // extensions won't have. If this fails just ignore the configuration
+        // and continue.
+        if (!$this->validateDependencies($data[$name], $enabled_extensions, $all_config)) {
+          continue;
+        }

For fixing the test fails, it might be helpful to have a debug('skipped $bla') here :)

I'm still wondering what to do about config entity dependencies. As mentioned, for modules, we might implement to find config that we can now install because it was just missing this module before, but I don't think we will ever do that for config entities. So maybe we should warn or fail for things that depend on missing config entities.

But, this is too late to abort the installation, the module is already installed by now.

A bit confused now if we skip, like the ConfigInstaller apparently does, or abort, which we seem to do, but not always?

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.99 KB
31.57 KB

We abort if the config with the missing dependencies is in the config/install directory of the extension we are installing. We skip if it provided by another extension.

Fixed some of the fails.

alexpott’s picture

FileSize
1.19 KB
30.88 KB

Some unnecessary change in #39

The last submitted patch, 39: 2090115.39.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: 2090115.40.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
31.54 KB

Some more fixes.

Status: Needs review » Needs work

The last submitted patch, 43: 2090115.43.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
507 bytes
32.04 KB

Okay fixed all the tests. Not sure why head is not failing with this atm. But there are several bug fixes to ConfigInstaller in this patch which have uncovered this.

Now to add explicit tests...

alexpott’s picture

Issue tags: +SprintWeekend2015
dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -149,30 +134,57 @@ public function installDefaultConfig($type, $name) {
    +    if ($install_profile = Settings::get('install_profile')) {
    +      if ($name !== $install_profile) {
    

    Can't we just remove the IF? The second if will never be TRUE if the install profile is empty.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -221,6 +238,13 @@ protected function createConfiguration($collection, array $config_to_install) {
    +        // All the configuration that is provided by the extension will have
    +        // been dependency checked already. But configuration provided by other
    +        // extensions won't have. If this fails just ignore the configuration
    +        // and continue.
    +        if (!$this->validateDependencies($name, $data[$name], $enabled_extensions, $all_config)) {
    +          continue;
    +        }
    

    So if those configuration are not installed they simply will never be installed. I guess its fine as nothing is "broken", there is just potential functionality missing.
    It would have been nice if we would at least tell people why we nevertheless still installed

  3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -361,4 +393,161 @@ public function findPreExistingConfiguration($type, $name) {
    +    if ($name != drupal_get_profile()) {
    

    Previously we used Settings::get() is there a reason we can't here?

  4. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -361,4 +393,161 @@ public function findPreExistingConfiguration($type, $name) {
    +    // All the migrate tests will fail if we check since they install the
    +    // migrate_drupal module but only set up the dependencies for the single
    +    // migration they are testing.
    +    if (strpos($config_name, 'migrate.migration.') === 0) {
    +      return TRUE;
    +    }
    

    Urgs

  5. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -361,4 +393,161 @@ public function findPreExistingConfiguration($type, $name) {
    +    // Core can provide configuration.
    +    $enabled_extensions['core'] = 'core';
    

    Do we plan to include core into the configuration file at some point? Might be interesting for weight problems.

  6. +++ b/core/lib/Drupal/Core/Config/UnmetDependenciesException.php
    @@ -0,0 +1,75 @@
    +    $message = String::format('Configuration objects (@config_names) provided by @extension have unmet dependencies',
    

    Helpful exception message!

  7. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -151,17 +151,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    -        // Install profiles can not have config clashes. Configuration that
    -        // has the same name as a module's configuration will be used instead.
    -        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 = $config_installer->findPreExistingConfiguration('module', $module);
    -          if (!empty($existing_configuration)) {
    -            throw PreExistingConfigException::create($module, $existing_configuration);
    -          }
    -        }
    +        // Check the validity of the default configuration. This will throw
    +        // exceptions if the configuration is not valid.
    +        $config_installer->checkConfigurationToInstall('module', $module);
    

    I really like this change!

  8. +++ b/core/modules/book/config/install/core.entity_view_mode.node.print.yml
    @@ -5,4 +5,8 @@ cache: true
    \ No newline at end of file
    

    meh

alexpott’s picture

Issue tags: -Needs tests
FileSize
9.4 KB
38.9 KB

@dawehner thanks for the review...

  1. Actually we can completely remove this - that is no longer necessary since ExtensionInstallStorage has whether or not to check profile configuration depending whether we are installing. Once we're not installing Drupal profile default configuration is (and should be ignored.
  2. Added a logger to at least log something - we have no way to put a message on screen really.
  3. Yep fixed
  4. Indeed. More reason migrations should not be config entities
  5. Separate, but interesting, issue.
  6. :)
  7. :)
  8. Fixed.

Added tests as well. And rerolled following #2406543: Remove ConfigFactory::setOverrideState and ConfigFactory::getOverrideState()

Status: Needs review » Needs work

The last submitted patch, 48: 2090115.48.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
39.27 KB

re #48.3 turns out there is. Settings.php does not have to have the install profile written - as shown by InstallerExistingSettingsTest. Whether we want to allow this is a different issue and one for a follow up. ExtensionInstallStorage is already aware if we are doing an installation as opposed to a regular module install so no change is necessary there.

larowlan’s picture

Issue tags: +Critical Office Hours

Manual testing for office hours:
install minimal
install node
install views
check the frontpage

cafuego’s picture

Assigned: Unassigned » cafuego
cafuego’s picture

Assigned: cafuego » Unassigned
FileSize
89.08 KB
158.04 KB
54.13 KB
82.52 KB

Manually tested. Installed minimal (which already includes node). Then enabled views and visited /node (as the default front page location wasn't changed)
Screenshots below:

Install screen

Enable views

Check views UI

Front page view

alexpott’s picture

Issue summary: View changes

Cleaning up IS.

alexpott’s picture

Added a small CR to document the new install task (install_install_profile) and the fact that the install task install_profile_modules no longer installs the install profile https://www.drupal.org/node/2413003

Berdir’s picture

Started testing this.

monitoring tests still work fine with my custom code, but I did found one problem in search_api: #2414629: Fix incorrect config dependencies in search_api.index.database_search_index. Was a bit hard to find, because it silently skipped the index due to the invalid config dependencies.

Berdir’s picture

Took me quite some time, but managed to install my install profile again.

Had to shuffle things around quite a bit, found a lot of stale and wrong dependencies, and mismatches in config entity ID's and filenames. The error messages were useful in debugging what was wrong, although what was missing was the information which file was missing which dependencies. Had to add my own var_dump() for that.

There is one problem with install profile overrides, but apparently they have a free pass (it ignored missing dependencies there), although I didn't fully understand how that works:
I'm overriding a view display for a module that provides one by default (file_entity). I also have additional fields, that are using additional widgets. Those have additional dependencies. Now, when file_entity is installed, it is using the config from the install profile instead and then doesn't find those dependencies, obviously. I guess I would have to move the additional part of those view displays to my install hook, if that would change.

In short, +1 from me in general, although I'd still love to find a general solution to support optional config with real dependencies on modules the same we support them based on the provider.

cilefen’s picture

I manually tested #50 with the process in the issue summary, with and without the path. Without the patch, node is allowed to install with the unmet dependency.

With the patch, I get "Unable to install Node, views.view.frontpage has unmet dependencies." in the GUI and in drush:

"exception 'Drupal\Core\Config\UnmetDependenciesException' with message 'Configuration objects (views.view.frontpage) provided by node have unmet dependencies

alexpott queued 50: 2090115.50.patch for re-testing.

effulgentsia’s picture

Given #54, I wanted to remove the "Needs issue summary update" tag, but a few things I don't understand from the current IS:

Default configuration entities could depend on a module that is not a dependencies on the modules that has the default configuration.

Huh? In addition to needing some grammar cleanup, which might clarify the meaning a bit, I think this could be expanded to really clarify what the problem is.

Install node - this should work
Install views - this should work - the front page will not exist.
...
Install views
Install node - this should fail

Why is the proposed resolution of this issue for the behavior to be different depending on order? If this is actually what is desired, the summary should explain why. If this is just the best we can do with current architecture, then that should be explained as well, perhaps with a link to a followup issue to improve that if appropriate.

alexpott’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Answered #60 by updating the issue summary.

larowlan’s picture

  1. +++ b/core/core.services.yml
    @@ -139,7 +139,7 @@ services:
    +    arguments: ['@config.factory', '@config.storage', '@config.typed', '@config.manager', '@event_dispatcher', '@logger.channel.default']
    

    6 injected services is getting on the high side. Not sure if there is any scope for cleanup here. Similar issues were seen in #2372507: Remove _system_path from $request->attributes - perhaps there's scope for a 'ConfigProxy' service that just includes the other four?

  2. +++ b/core/includes/install.core.inc
    @@ -1563,10 +1561,7 @@ function install_profile_modules(&$install_state) {
    -  // Although the profile module is marked as required, it needs to go after
    -  // every dependency, including non-required ones. So clear its required
    -  // flag for now to allow it to install late.
    -  $files[$install_state['parameters']['profile']]->info['required'] = FALSE;
    

    <3 seeing this hack go by the wayside

  3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -355,4 +389,173 @@ public function findPreExistingConfiguration($type, $name) {
    +      // Validate default configuration of this module. Bail if unable to
    

    As much as I love the phrase 'Bail', not sure it meets our standards. Perhaps 'Throw an execption'

  4. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -355,4 +389,173 @@ public function findPreExistingConfiguration($type, $name) {
    +    // All the migrate tests will fail if we check since they install the
    +    // migrate_drupal module but only set up the dependencies for the single
    +    // migration they are testing.
    +    if (strpos($config_name, 'migrate.migration.') === 0) {
    

    ouch. This kind of smells like code that has to be aware of simpletest. Do we really want to bake this in? Perhaps make this configurable via settings/container parameter/something in case something else in contrib wants this affordance? Or we just make the call that migrate needs to sort its stuff out (which would be a follow up)

  5. +++ b/core/lib/Drupal/Core/Config/UnmetDependenciesException.php
    @@ -0,0 +1,75 @@
    +    $e = new static($message);
    +    $e->configObjects = $config_objects;
    +    $e->extension = $extension;
    

    lovely use of php scope

  6. +++ b/core/modules/block_content/tests/modules/block_content_test/block_content_test.info.yml
    --- a/core/modules/block_content/tests/modules/block_content_test/config/install/block.block.foobargorilla.yml
    +++ b/core/modules/block_content/tests/modules/block_content_test/config/install/block.block.foobargorilla.yml
    

    sounds like something I'd have come up with :-P

  7. +++ b/core/modules/book/config/install/core.entity_view_mode.node.print.yml
    @@ -5,4 +5,8 @@ cache: true
    +      - book
    

    <3

  8. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -144,6 +145,20 @@ public function install(Request $request) {
    +      catch (UnmetDependenciesException $e) {
    +        $config_objects = $e->getConfigObjects();
    +        drupal_set_message(
    +          $this->formatPlural(
    +            count($config_objects),
    +            'Unable to install @extension, %config_names has unmet dependencies.',
    +            'Unable to install @extension, %config_names have unmet dependencies.',
    +            array(
    +              '%config_names' => implode(', ', $config_objects),
    +              '@extension' => $theme,
    +            )),
    +          'error'
    +        );
    
    +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
    @@ -176,6 +177,21 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      catch (UnmetDependenciesException $e) {
    +        $config_objects = $e->getConfigObjects();
    +        drupal_set_message(
    +          $this->formatPlural(
    +            count($config_objects),
    +            'Unable to install @extension, %config_names has unmet dependencies.',
    +            'Unable to install @extension, %config_names have unmet dependencies.',
    +            array(
    +              '%config_names' => implode(', ', $config_objects),
    +              '@extension' => $this->modules['install'][$e->getExtension()],
    +            )),
    +          'error'
    +        );
    +        return;
    
    +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -536,6 +537,21 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      catch (UnmetDependenciesException $e) {
    +        $config_objects = $e->getConfigObjects();
    +        drupal_set_message(
    +          $this->formatPlural(
    +            count($config_objects),
    +            'Unable to install @extension, %config_names has unmet dependencies.',
    +            'Unable to install @extension, %config_names have unmet dependencies.',
    +            array(
    +              '%config_names' => implode(', ', $config_objects),
    +              '@extension' => $modules['install'][$e->getExtension()],
    +            )),
    +          'error'
    +        );
    

    We have this hunk three times, would it make sense to have a method on UnmetDependenciesException to generate this message? UnmetDependenciesException->getDependencyMessage()? or similar?

alexpott’s picture

FileSize
7.61 KB
40.92 KB

Thanks @larowlan

  1. I'm not too concerned about this in installer code. ConfigManager actually has a most of these services we could make this a proxy for them. I'd prefer to handle this in a separate issue if we really want to do that.
  2. Me too
  3. Comment rewritten
  4. I don't think any other configuration entities should get this exception. Migration testing is kinda special and migration configuration entities are too. Several times I've tried to convince people that they should not be configuration entities - have not managed to do that yet.
  5. :)
  6. :)
  7. :)
  8. Looked at the potx module - looks like it will support this - added a method and will create another issue up to do the same for PreExistingConfigException

Had to fix ConfigInstallTest because config_schema_test now provides a config_test default config entity and with this change and the weirdness of the KernelTestBase environment (modules enabled without config being created at the same time).

Status: Needs review » Needs work

The last submitted patch, 63: 2090115.63.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
451 bytes
41.36 KB

More KernelTestBase weirdness for the same reason.

xjm’s picture

Did a first round of manual testing just using the steps in the summary. There are three things I found unintuitive:

  1. In step 5 of the testing steps in the summary, the message being logged only to watchdog, with no notification on the screen, seems weird. I understand that the installation of Views should not be blocked on node's view having a missing dependency -- that's not Views' fault -- and so that that might be the reason for the different behavior. But I'm not 100% about the decision to only log it to watchdog. Part of this might be out of scope -- we have the magic that installing views goes and finds all your views and installs them if it can, but no record of that in (e.g.) watchdog, nor an indication to the user.

     

  2. In step 8, it's unintuitive that installing node first and then views should work, but installing views first and then node should fail. I guess we are adding the expectation that a dependency in a default configuration file is a dependency of the module, but this seems utterly contradictory to the fact that the config entity also requires Views! So we silently skip the default config if the module that owns the API for it is not there, but fail hard if the same config includes a different dependency. That is seriously WTFy, especially to a site builder or a dev who doesn't know the history of how config entities have evolved in D8.

    The difference between Views and Book in terms of the modified frontpage view here is really subtle for anyone who hasn't had their head in this for years. They are both required for the view. Differently required, but still required. Why should Node throw up its hands when one is missing, but happily skip the View when the other is missing?

     

  3. Also in step 8, the error message I receive when I try to install node is:

    Unable to install Node, views.view.frontpage has unmet dependencies.

    Leaving aside for a moment the comma splice. :P This is a site builder-facing message, but it doesn't tell the site builder how to resolve it. At a minimum, we need to tell the user what the unmet dependencies are (i.e., "Hey friend, enable book first").

Next I'll test some different permutations, and synch.

xjm’s picture

To re-frame #66 point 2 a little: Either installing default configuration for a module is a required part of installing the module, or it's an optional step that happens if possible. With the fix here (which is certainly an improvement on "silently broken", of course), we are doing one thing sometimes and the other sometimes, depending on what order the site builder remembered to enable their modules in.

xjm’s picture

Issue summary: View changes

Updated the testing steps to clarify what's different between HEAD and the patch.

alexpott’s picture

The thing is if a module provides configuration with unmet dependencies this is a bug. It is not something that should ever occur in regular site building. And if it does a module should have a bug report filed against it to add the correct dependencies. Alternatively, a sub module that has the correct dependencies can be added. But in the case of installing configuration that is provided by other modules we can't fail the installation of the module because it has done nothing wrong.

I believe the current way is the only way to proceed here without adding unnecessary layers of complexity. It is up to the module developer to ensure that the requirements of their provided configuration can be met regardless of installation order.

I do, agree, that adding a visual indication that configuration has been skipped is important. Since this will help get bug reports filed and provide an element of consistency. The only way we can do this is by putting a drupal_set_message in ConfigInstaller which feels icky - but in this case necessary.

Berdir’s picture

1) Yes, the problem is our only options there are watchdog() or drupal_set_message(), and drupal_set_message() there could be problematic, but I guess we can try (problematic: We might display those message in unexpected situtations / to wrong users, who knows how it will be used).

3) There's not that much we can do (we don't always know the module, it could also be a missing config entity that we don't know who should provide it), but I agree that we should display the actual missing dependencies there. I was trying to say the same in the second paragraph in #57, but hidden in a lot of text.

Speaking of that, I'd love to know more about the third paragraph in #57, because I don't fully understand why that works yet, and I'd love to ;)

xjm’s picture

@alexpott:

The thing is if a module provides configuration with unmet dependencies this is a bug. It is not something that should ever occur in regular site building

So you are saying, in the test case, Node itself should declare a dependency on Book? If so, we should get that into the exception message somehow, and think more about how we can communicate this to module developers in our dependency documentation. It's still weird though that we allow you to not declare a dependency on Views, and that's hunkey dorey (edit: to be clear, I know why we do this; I advocated for continuing to support it earlier in this issue), but we require you to declare the internal dependencies of individual views. #67 still applies I think.

@berdir re: point 3:
Yeah, I thought about that too (modules vs. config entities) but we can still easily tell you what the missing dependencies are. and when it's a module dependency it's pretty simple to understand.

xjm’s picture

Oh, looking at #57 from @berdir:

although I'd still love to find a general solution to support optional config with real dependencies on modules the same we support them based on the provider.

This is related to what I'm getting at in #67. Edit: Also, @effulgentsia has also given similar feedback in #60.

In terms of the principles of configuration management, at a high level, do we want to state that default configuration is an optional part of a module (as it is with the provider) or that it is required (as with the internal dependency of a particular entity)? I recognize that supporting @berdir's suggestion would be a ton of overhead vs. the owner and provider checks, and that's okay to divert to a followup issue in order to sooner prevent the data integrity/silent failure/unrecoverable state bugs that could result at present. However, while I understand the technical reasons that Views and Book are different in terms of views.view.frontpage.yml, that doesn't mean it's not worth discussing the conceptual problem.

xjm’s picture

I tested installing Standard with the patch and the modified frontpage view. It follows the behavior of #5 in the summary, not #8 -- that is, both node and views are installed without any problem, but the frontpage shows "Page not found" and the unmet dependency message is logged to watchdog (as above, without indicating what the unmet dependency was).

However, when I moved the frontpage view to a znode module at the end of the standard profile dependency list, installation threw an exception with the unmet dependency error (the expected behavior for step #8). If I move znode to right after node, it just installs without installing the frontpage view.

I.e., this installation succeeds, does not create the frontpage view, and logs the message to watchdog:

  - node
  - znode
  - history
  - block
  - breakpoint
  - ckeditor
  - color
  - config
  - comment
  - contextual
  - contact
  - menu_link_content
  - datetime
  - block_content
  - quickedit
  - editor
  - entity_reference
  - help
  - image
  - menu_ui
  - options
  - path
  - taxonomy
  - dblog
  - search
  - shortcut
  - toolbar
  - field_ui
  - file
  - rdf
  - views
  - views_ui
  - tour

Whereas this installation fails with an exception:

  - node
  - history
  - block
  - breakpoint
  - ckeditor
  - color
  - config
  - comment
  - contextual
  - contact
  - menu_link_content
  - datetime
  - block_content
  - quickedit
  - editor
  - entity_reference
  - help
  - image
  - menu_ui
  - options
  - path
  - taxonomy
  - dblog
  - search
  - shortcut
  - toolbar
  - field_ui
  - file
  - rdf
  - views
  - views_ui
  - tour
  - znode

If I call the module anode instead of znode, either order works. So there is something :( going on with the ordering of dependencies, and this ordering inconsistency also means we have an unreliable behavior installing what is the same code because of the different responses to a missing owner module vs. missing internal dependency.

xjm’s picture

I tested #73 three times, but attaching patches to make sure I'm not crazy and expose whether this happens on not-my-laptop as well. Edit: the anode-first and anode-last patches are named the wrong way around but same difference. And of course these patches are going to fail anything that actually tests the frontpage view. ;) Just want to confirm that they fail differently...

xjm’s picture

Status: Needs work » Needs review

Okay Drupal\standard\Tests\StandardTest fails on all four with the exception... whereas locally (Edit: both with Drush and in the UI) I can install standard cleanly with the first three, just not the fourth. Something spooky is going on.

Edit: StandardTest also fails for me locally with the exception, even in the three patches that install with no exception.

I'm running MAMP with PHP 5.5.10.

xjm’s picture

@Berdir, maybe your mysterious third paragraph in #57 has something to do with the ordering strangeness? Can you reproduce the same results I get with the patches in #74?

alexpott’s picture

So the "spooky" thing @xjm is seeing wrt to the order is not introduced by the patch - but the patch exposes some new problems. Modules are ordered by dependency but within the tree we don't enforce any other ordering so if modules have the same dependencies then the order that they are installed in can depend on your php version (fun eh).

Reading through this issue, thinking about all of the dependency work throughout the cycle, and the complexity of installing config entities provided by other modules - I'm leaning towards changing the ability to provide optional configuration in a module's config/install directory. If configuration in that directory can not created when a module is installed then the module should not be installed.

So how should we provide optional configuration? I think we should stick it in a directory called optional. If the this configuration can be created (it's dependencies are met) when the module is installed it should be created. If not then it's not. Then we can explore in a followup adding a UI that lists optional configuration that can be created.

Doing this should lead to a simpler config installer - we only have to read two directories and both from the module being installed and is easier to explain. That is to say, when you install a module only configuration contained within the module is created.

alexpott’s picture

@xjm also should have said in #81 the module ordering randomness due to the dependency graph is a great catch.

xjm’s picture

So this should probably go in whatever other issue, but if we make "optional" config a separate install folder, how do we support both Minimal and Standard? I.e., in Minimal, we install Node with no frontpage view. In Standard, we install Node and Views and the frontpage view. Who owns the view? Surely it can't be Standard...

xjm’s picture

Or is "optional" config installation another separate and final step in installation and synch?

alexpott’s picture

Issue tags: +Needs issue summary update
FileSize
69.3 KB

The points raised in #67 and #60 by @xjm and @effulgentsia led me to investigate a different approach to the problem of how an extension provides optional configuration. In HEAD, an extension has no way to indicate that it relies on being able to create a particular piece of configuration in its config/install directory. If node is installed without views, the views in node/config/install are not created. This does not matter because node does not have any hard dependencies on them. When views is installed it searches all the enables extensions config/install directories and create the views. This patch, would silently ignore them if the dependencies can not be met. If views was installed first, before the module providing the view with unmet dependencies, then that module would error during installation.

The behaviours of HEAD, the patch in #65 and @xjm's discoveries about the module dependency ordering are telling us we have the config installation architecture wrong. We need to know when a piece of configuration is required and when it is optional.

The patch attached takes the following approach:

  • All configuration in config/install has to have its dependencies met during install - if it does not then you get the unmet dependencies error.
  • Optional configuration is stored in config/optional. During an extension's installation all configuration objects, whose dependencies can be met, are installed.
  • When you install a config entity type provider (eg. views) all config/optional directories are searched for views and those views, whose dependencies can be met, are installed.

To me this approach is great because the developer can then include all sorts of things that work when different extensions are installed but know that it does not interfere with the basic configuration installation for the module. The developer's intent is obvious from the config directory structure. What's more is that is allows for the config installer to become a bit less cumbersome because we're no longer search every extension's config/install directory on every module install. In further iterations of the patch I explore only searching every extension's config/optional directory when the extension being installed provides a config entity type.

I think that rather than proceeding with this patch and then #2431275: Move optional configuration in config/install to config/optional we should do it all in this issue because, together, they are the complete solution to the unmet dependencies problem. I'll wait before updating the issue summary to get consensus that this is the way to go. (No interdiff because too much change)

Berdir’s picture

That sounds good to me, and I'm +1 on merging it into this issue.

The current directory name will conflict with my current implementation in monitoring, which is however a bit different and groups them by module. I'll have to at least rename that for now. No problem, should have picked a less-obvious name for me ;)

The optional installation for this issue is still going to be limited by provider, which is fine for now, we're doing enough here already. We could open a follow-up (feature? task?) to support enabling optional configuration later on, when those dependencies become available (e.g. a module is being installed). That could be a manual process with a UI, or maybe something automatic, but then I guess we to store metadata somehow so that we can find those dependencies easily. As I said, in a follow-up :)

alexpott’s picture

New patch with a couple of improvements - only scans all optional directories if the extension being installed provides a config entity type and now that the createConfiguration method no longer accesses source storage we don't need the is installing stuff.

alexpott’s picture

Re #86 so the problem with automating the installing of optional configuration when dependencies are met is what do you do about stuff that the site owner has deleted?

alexpott’s picture

FileSize
2.31 KB
66.93 KB

Bye logging service.

Berdir’s picture

I assume we would limit to installation of optional configuration to things that become available again by enabling a specific module. So if you have an optional view that depends on taxonomy, then we'd only attempt to install that view if taxonomy is now available and wasn't before. I don't know if we should or even can support config entities here as well, likely only modules. Having optional configuration based on some other optional configuration seems weird (unless that is also default configuration of a module).

Or, we'd keep track of a list optional-not-yet-installed configuration, that is only written when an extension is installed. Possibly keyed by the missing dependency (ies), so we have a fast way of finding those optional things. Then it also shouldn't be an issue, because we'd only install something once.

PS: I'm remote and they blocked IRC :(

alexpott’s picture

#90 sounds extremely complex to do in core - but yep as you pointed out in #86 this is good material for a followup.

The other thing that is really nice about this patch is that optional config won't install over pre-existing configuration or fail because of it. So if you installed minimal profile, installed views, uninstalled node (which deletes views.view.frontpage), created a views.view.frontpage that listed users, and then installed node, it won't prevent you from installing node (as HEAD would) and your view will still be there.

effulgentsia’s picture

I haven't reviewed the patch itself, but +1 on the new direction as explained in #85 and subsequent comments.

swentel’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -289,10 +340,11 @@ public function resetSourceStorage() {
         if (!isset($this->sourceStorage)) {
    +      throw new ConfigException('This should not happen');
           // Default to using the ExtensionInstallStorage which searches extension's
           // config directories for default configuration. Only include the profile
           // configuration during Drupal installation.
    -      $this->sourceStorage = new ExtensionInstallStorage($this->getActiveStorages(StorageInterface::DEFAULT_COLLECTION), InstallStorage::CONFIG_INSTALL_DIRECTORY, $collection, drupal_installation_attempted());
    +      // $this->sourceStorage = new ExtensionInstallStorage($this->getActiveStorages(StorageInterface::DEFAULT_COLLECTION), InstallStorage::CONFIG_INSTALL_DIRECTORY, $collection, drupal_installation_attempted());
         }
    

    So this check can be removed completely ?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -358,4 +415,184 @@ public function findPreExistingConfiguration($type, $name) {
    +      // that already exits. Additionally, can not continue installing more
    

    exits => exists

  3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -96,34 +98,106 @@ public function installDefaultConfig($type, $name) {
    +      foreach ($config_to_install as $config_name => $data) {
    +        // Ensure that the configuration does not exist already, that it is a
    +        // configuration entity, and that its dependencies can be met.
    +        if (in_array($config_name, $existing_config) ||
    +          !$this->configManager->getEntityTypeIdByName($config_name) ||
    +          !$this->validateDependencies($config_name, $data, $enabled_extensions, $all_config)) {
    +          unset($config_to_install[$config_name]);
    +        }
    

    Should we log something in case someone accidentally puts a simple configuration file in the optional folder ?

    I've tested it adding a simple config file to an optional directory and it doesn't get installed, which is the intention, but maybe developers might get confused here ?

  4. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -358,4 +415,184 @@ public function findPreExistingConfiguration($type, $name) {
    +   *   The name of the installation profile or NULL if no installation profile is
    

    80 chars

  5. +++ b/core/modules/config/src/Tests/ConfigImportRecreateTest.php
    @@ -93,8 +93,8 @@ public function testRecreateEntity() {
    +    $this->assertEqual(5, count($creates), 'There are 4 configuration items to create.');
    +    $this->assertEqual(5, count($deletes), 'There are 4 configuration items to delete.');
    

    4 = 5

  6. +++ b/core/modules/views/src/Tests/ViewExecutableTest.php
    @@ -83,7 +84,9 @@ protected function setUpFixtures() {
    +    // Clear the views data cache so it updated with the new entity information.
    

    it's ?

swentel’s picture

re: 3 - ignore that idea. Sometimes, we don't even know if the file is for a config entity or a simple configuration anyway.

alexpott’s picture

FileSize
70.13 KB
8.86 KB
  1. Yep - fixed. Also made resetSourceStorage protected since we now do this as part of install default config.
  2. Fixed
  3. See #94
  4. Fixed
  5. Fixed - we now have 5 items to delete because we have the teaser view mode from the node module
  6. Fixed
Berdir’s picture

Uff, long review below. Looks overall very good, comments are mostly about details, naming and questions.

  1. +++ b/core/includes/install.core.inc
    @@ -1634,6 +1629,19 @@ function install_profile_themes(&$install_state) {
    +  // Install all available optional config.
    +  // @todo discuss whether this is the desired approach?
    +  \Drupal::service('config.installer')->installAllOptionalConfig();
    

    What exactly is the purpose of this?

    My guess is a final attempt to find and install any optional configuration that we now can, now that all modules are installed that we'll install as part of the installation, correct?

    Sounds useful to me, just needs a bit more documentation why and what it's doing exactly.

    I guess we can also make this available to users later, possibly a contrib module, although I assume we will need to make it a more manual process, where we show config that we could install, due to the issues you mentioned (deleted configuration and so on).

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -96,34 +96,106 @@ public function installDefaultConfig($type, $name) {
    +    if (!$this->isSyncing() && !drupal_installation_attempted()) {
    

    Oh, so in the installer, the call above is actually the only place where we install config.

    What is the reason to do it like this exactly? A comment here would be useful.

    I'm wondering if this could be an issue but I think not. modules shouldn't rely on optional config, and neither should modules rely on optional config from other modules, so it shouldn't matter if we install it piecemeal or at the end.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -96,34 +96,106 @@ public function installDefaultConfig($type, $name) {
    +    if ($this->isSyncing()) {
    +      return;
    +    }
    

    Do we need to call this here? it's protected and only called if we're not syncing. And I guess installAllOptionalConfig() isn't going to be called during sync and if, we should check it earlier there?

  4. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -96,34 +96,106 @@ public function installDefaultConfig($type, $name) {
    +    foreach ($collection_info->getCollectionNames(TRUE) as $collection) {
    +      $config_entity_support = $this->configManager->supportsConfigurationEntities($collection);
    +      if (!$config_entity_support) {
    

    true is the default argument, do we need to specify it here? I don't think it really makes it clearer, you have to look up what it actually means anyway, and it makes it look like we're doing something special when it actually is the standard case.

    Also, supportsConfigurationEntity only returns TRUE if it is the default collection. So why are we looping exactly? Is it to support a different config manager implementation that would support config entities in non-standard collections? is that even possible?

    Lastly, the local variable seems unecessary, we could just do a if (!$this->configManager->supportsConfigurationEntities($collection)) ?

  5. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -96,34 +96,106 @@ public function installDefaultConfig($type, $name) {
    +        // Ensure that the configuration does not exist already, that it is a
    +        // configuration entity, and that its dependencies can be met.
    +        if (in_array($config_name, $existing_config) ||
    +          !$this->configManager->getEntityTypeIdByName($config_name) ||
    

    The comment actually says the opposite of what we are doing, in a way, because we do negative checks and exclude if any condition doesn't apply. Would it help to write the comment in the same way, like "Exclude any configuration that exists already, ..." ?

  6. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -133,43 +205,23 @@ public function installDefaultConfig($type, $name) {
    +  protected function getDefaultConfigToInstall($collection, $name = NULL) {
    

    Is the documentation on this method still accurate?

    It starts with Lists, when the method no longer starts with list, and it explicitly mentions config/install, when it is actually depending on where the storage is pointed to.

    Do we even need "Default" in the name? would just getConfigToInstall() be enough as a name? not exactly sure what Default now means in this context, where it can be optional configuration as well (is optional configuration also default configuration?)

  7. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -133,43 +205,23 @@ public function installDefaultConfig($type, $name) {
    +    $storage = $this->getSourceStorage($collection);
    

    Wondering if it would be clearer if we would pass in the storage explicitly to this method. This might actually make it easier to understand and we might remove some of the documentation of where we are looking with something like "in the passed in storage" or so?

  8. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -177,12 +229,11 @@ protected function listDefaultConfigToInstall($type, $name, $collection, array $
    -  protected function createConfiguration($collection, array $config_to_install) {
    +  protected function createConfiguration($collection, array $data) {
         // Order the configuration to install in the order of dependencies.
    -    $data = $this->getSourceStorage($collection)->readMultiple($config_to_install);
         $config_entity_support = $this->configManager->supportsConfigurationEntities($collection);
         if ($config_entity_support) {
    

    Was wondering about the argument rename, but I get it now, it's the same variable names as before, we just inverted it from passing in the names and loading it to passing in the data and then getting the names internally.

    Neither $data nor $config_to_install are overly clear about what they contain, but I don't have better suggestions and changing that would not be long in this issue anyway.

  9. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -246,16 +296,15 @@ protected function createConfiguration($collection, array $config_to_install) {
    +    $storage = new ExtensionInstallStorage($this->getActiveStorages(StorageInterface::DEFAULT_COLLECTION), InstallStorage::CONFIG_INSTALL_DIRECTORY, $collection, drupal_installation_attempted());
    

    Is the drupal_installation_attempted() argument changing when profile overrides are applied compared to HEAD?

    I noticed that you can even have overrides (not sure if that is the right word) for modules that installed later on and it will pick those from the install profile. If I understand this correctly, then this will change that behavior?

    Not necessarily a bad thing, just trying to understand.

  10. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -246,16 +296,15 @@ protected function createConfiguration($collection, array $config_to_install) {
    +    $config_to_install = array_filter($storage->listAll(), function ($config_name) use ($enabled_extensions) {
           $provider = Unicode::substr($config_name, 0, strpos($config_name, '.'));
           return in_array($provider, $enabled_extensions);
         });
    

    Let's say I have a optional view in my module and I'm providing a translation for it.

    Where would I put that translation override? Is it even possible to be put in config/optional/languages/de/, for example, or would it have to be in config/default? Do we care about installing overrides when the config they override doesn't exist? I guess collections aren't limited to being overrides, technically.

  11. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -284,17 +340,12 @@ public function resetSourceStorage() {
    -      $this->sourceStorage = new ExtensionInstallStorage($this->getActiveStorages(StorageInterface::DEFAULT_COLLECTION), InstallStorage::CONFIG_INSTALL_DIRECTORY, $collection, drupal_installation_attempted());
    

    Looks like that line is just moving, then my assumption about profile config override must be wrong, but why does it work now the way it does?

  12. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -333,22 +384,27 @@ public function isSyncing() {
    +   * @return array
    +   *   Array of configuration objects that already exist keyed by collection.
    

    I guess this documentation is just moved, is it worth clarifying that it returns configuration object *names*?

  13. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -358,4 +414,184 @@ public function findPreExistingConfiguration($type, $name) {
    +      // Configuration is assumed to already be checked by the config importer
    +      // validation events
    

    nitpick: missing . at the end.

  14. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -358,4 +414,184 @@ public function findPreExistingConfiguration($type, $name) {
    +   * @param array $data
    +   *   Configuration data.
    

    this is why $data is kind of a bad name in the other method IMHO. there it is multiple objects keyed by name, here it is the data of a single one, which I think is the more common thing for "config data". But well, english has no plural for data, so... :)

  15. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -358,4 +414,184 @@ public function findPreExistingConfiguration($type, $name) {
    +    // All the migrate tests will fail if we check since they install the
    +    // migrate_drupal module but only set up the dependencies for the single
    +    // migration they are testing.
    +    if (strpos($config_name, 'migrate.migration.') === 0) {
    

    migration... config entity... I've seen you have the same opinion ;)

  16. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -358,4 +414,184 @@ public function findPreExistingConfiguration($type, $name) {
    +      if (isset($all_dependencies['enforced'])) {
    +        $all_dependencies = array_merge($all_dependencies, $data['dependencies']['enforced']);
    +      }
    

    is it worth unsetting enforced after merging it, so that we don't have to loop over it below?

  17. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -358,4 +414,184 @@ public function findPreExistingConfiguration($type, $name) {
    +  protected function drupalGetPath($type, $name) {
    +    return drupal_get_path($type, $name);
    +  }
    

    I guess we do this for unit tests, wondering why we don't need it for drupal_installation_attempted(), I guess that code path is not covered with unit tests (which probably makes sense, as it is reading files).

  18. +++ b/core/lib/Drupal/Core/Config/UnmetDependenciesException.php
    @@ -0,0 +1,96 @@
    +  public static function create($extension, array $config_objects) {
    

    I think in other cases we just have custom constructor arguments, but this is fine as well.

  19. +++ b/core/modules/config/src/Tests/ConfigOtherModuleTest.php
    @@ -59,18 +60,10 @@ public function testInstallOtherModuleFirst() {
    -    // Re-enable module to test that pre-existing default configuration throws
    ...
    +    // Re-enable module to test that pre-existing optional configuration does
    

    Interesting, here the comment changes from default to optional configuration, which kind of supports my point about the method name above :)

  20. +++ b/core/modules/config/src/Tests/ConfigOtherModuleTest.php
    @@ -97,6 +90,25 @@ public function testUninstall() {
    +    try {
    +      $this->installModule('config_install_dependency_test');
    +      $this->fail('Expected UnmetDependenciesException not thrown.');
    +    }
    +    catch (UnmetDependenciesException $e) {
    +      $this->assertEqual($e->getExtension(), 'config_install_dependency_test');
    +      $this->assertEqual($e->getConfigObjects(), ['config_test.dynamic.other_module_test_with_dependency']);
    +      $this->assertEqual($e->getMessage(), 'Configuration objects (config_test.dynamic.other_module_test_with_dependency) provided by config_install_dependency_test have unmet dependencies');
    +    }
    +    $this->installModule('config_test');
    +    $this->installModule('config_other_module_config_test');
    +    $this->installModule('config_install_dependency_test');
    

    I think this is the API test for the UI test above. why are we in the UI installing config_test first and documenting that as important and here the order is different?

  21. +++ b/core/modules/views/src/Tests/ViewExecutableTest.php
    @@ -83,7 +84,10 @@ protected function setUpFixtures() {
    +    $this->installConfig(array('system', 'field', 'node', 'comment'));
    +    // Clear the views data cache so it's updated with the new entity
    +    // information.
    +    \Drupal::service('views.views_data')->clear();
    

    #2443485: Remove extension:views cache tag and other views related cache improvements should remove the need for ->clear() :)

jibran’s picture

Status: Needs review » Needs work

NW as per #96

alexpott’s picture

Status: Needs work » Needs review
FileSize
27.58 KB
73.6 KB

Awesome review @Berdir - thanks!

Re #96

  1. I added this so that we can be sure that the variable module order during the installer exposed by @xjm's test's does not catch us out. Added comment to detail this. Regarding usage to install optional configuration later by contrib you're right we need to be able to install specifically chosen configuration. The current API does not make this easy enough. Removed InstallAllOptionalConfig and replaced it with InstallOptionalConfig that accepts a storage and a prefix so it can be used to install single pieces of optional configuration or configuration from alternative directories.
  2. Yep comment added. This is is also a performance boost - we only scan all the optional folders once during installation.
  3. You're right - removed.
  4. Removed the TRUE and the local variable. The original idea behind the supportsConfigurationEntity() was that any collection should be able to support everything that the default collection does. But I'm less enamoured by that idea now. I think collections should only be used for storage overrides. But removing this is a separate issue. Shall we open a followup?
  5. Fixed
  6. No it's not accurate at all. Updated and completely removed the prefix stuff. This should be handled elsewhere. Now it's a two line method with no logic.
  7. I think we should reconsider getSourceStorage() and replace it with an override source storage method. I've tried to do this but the problem is the container rebuild that occurs during a module install. I'd like to open a followup to refactor this and move the overriding into the module installer because this is not rebuilt during a module install and then we can pass an overriden source storage into ConfigInstaller::installDefaultConfig() and clean up everything so the config installer has less state. This also makes it easier to avoid nasty issues due to the fact that the config installer is re-entrant - installCollectionDefaultConfig is called on saving a language config entity. The patch removes all the source storage setting inside ConfigInstaller to make the followup simple.
  8. okay
  9. This is all because we only use profile configuration during installation and not during module install. Profile overrides of default configuration provided by other modules only work during install afaik.
  10. Good question. I think we should have a followup to consider making non default collections override only and then we can only create overrides if configuration exists.
  11. See 9.
  12. Fixed
  13. Fixed
  14. Yeah tricky... not done anything here. Renamed $data to $config_to_create in createConfiguration
  15. Yep :)
  16. Yep - makes sense.
  17. I've done this for drupal_installation_attempted too - afaik there are no unit tests for the ConfigInstaller but I'd love to get to the point where we can have some.
  18. Okay
  19. Yep - fixed.
  20. I've moved this test to Drupal\config\Tests\ConfigInstallTest since the equivalent UI test is in Drupal\config\Tests\ConfigInstallWebTest and made the order the same for the UI and API tests.
  21. Yep removed.
alexpott’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
alexpott’s picture

Issue summary: View changes
Berdir’s picture

4. Follow-up for this sounds fine.
7. Sounds good, I'm not exactly sure how much of that you already did now, didn't fully read through the interdiff yet.
9. I'm pretty sure that I recently installed a module and it picked up config from the install profile. will try again.
10. Follow sounds good, again :)

Was testing it again and found a problem with install profile configuration, which it was trying to install twice. The fix for that caused the problem that I expected to see in #57 paragraph 3, that dependencies are checked for config overrides and then it failed to install those.

Discussing that, we found two ways to move forward.
1. Accept that. Whoever does overrides like that will have to figure out an alternative approach, like changing the necessary configuration in a hook_install() or convince the maintainer to make that configuration optional (luckily, I maintain the port of the problematic module, so I only have to convince myself)
2. Change the way overrides are installed, by skipping them first and only installing them later on. That caused some problems in my case because I'm overriding the anon/authenticated user roles to give them additional permissions and was relying on default config in another case. I can work around both those problems.

We're not quite sure yet which option is better, but @alexpott tends toward 1 I think. Which works for me, I'll find a way to make it work for me.

alexpott’s picture

I think given the issue summary:

All configuration in config/install has to have its dependencies met during install - if it does not then you get an unmet dependencies error.

We have to go for option 1 - which means that profiles can profile overrides just as long as they don't introduce dependencies that can not be met at the time of install. The mitigation is that if the module does not actually depend on it moving it the configuration to optional the profile can override it and add dependencies because optional configuration is only installed after the profile is.

alexpott’s picture

FileSize
87.74 KB
18.37 KB

Here's a patch that handles and tests the expectations around the ability of install profiles to override configuration

Berdir’s picture

New tests look great.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -392,9 +415,11 @@
    +    // Gets a profile storage to search for overrides if necessary.
    +    $profile_storage = $this->getProfileStorage($name);;
    

    nitpick: ;;

  2. +++ b/core/modules/config/src/Tests/ConfigInstallProfileUnmetDependenciesTest.php
    @@ -0,0 +1,95 @@
    +   * Set to TRUE if the expected exception is thrown.
    +   * @var bool
    

    should have an empty line above @var.

  3. +++ b/core/modules/config/src/Tests/ConfigInstallProfileUnmetDependenciesTest.php
    @@ -0,0 +1,95 @@
    +    $config_file = $dest . DIRECTORY_SEPARATOR . InstallStorage::CONFIG_INSTALL_DIRECTORY . DIRECTORY_SEPARATOR . 'system.action.user_block_user_action.yml';
    +    $action = Yaml::decode(file_get_contents($config_file));
    +    $action['dependencies']['module'][] = 'action';
    +    file_put_contents($config_file, Yaml::encode($action));
    +    parent::setUp();
    

    So the installation aborts because the config would be installed together with user.module and at that point, action is not yet installed.

    First I thought that it might make sense to have a more obvious dependency, but actually, this is perfect and a real life scenario. The install profile does depend on that module, but user is installed first, so that doesn't work then.

alexpott’s picture

Fixed everything @Berdir mentions in #104. Thanks for the review.

Berdir’s picture

Great. I think the patch is ready now. Issue summary looks good to me as well.

AFAIK, the only missing pieces are a few follow-ups and a change record that explains that configuration dependencies are now validated and that a new config/optional folder exists for dependencies that are optional and are only installed when the dependencies are met and the config doesn't already exist. It might be worth mentioning that optional config is only installed at the end of the installer and what that means.

alexpott’s picture

FileSize
10.33 KB
95.64 KB

I think we might want to think about optional configuration in profiles a little bit. In the current patch profiles can't provide optional configuration and anything that is there is silently ignored. I think we should either:

  1. Throw an exception during installation if the profile has an optional folder
  2. Allow install profiles to have an optional folder and search it for configuration during module installs even when not in an installation.

I'm leaning towards option 2 because this means that install profiles are more like regular modules and there is less to explain and it might be useful for some profiles to give extra functionality when an optional module is installed. For example, if a profile has a install step form that allows people to select optional functionality that results in modules being installed. The one bit of awkwardness is how should overriding configuration provided by other modules in profiles work? Not entirely sure...

Here is a patch that does what I think we should do:

  1. Profiles can contain optional configuration
  2. Optional configuration can not override default install configuration
  3. Profile's optional configuration is used even when not installing Drupal

The patch also contains tests for all of these expectations plus we can simplify the code in ExtensionInstallStorage that deals with profile overrides.

alexpott’s picture

Damn EOF new lines :)

The last submitted patch, 107: 2090115-opt.107.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 108: 2090115-opt.108.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
97.22 KB

Fixing the tests. It's brought up something very interesting about installing with an existing settings.php - we don't ensure that the profile is written in settings.php or can be. We should address that in a followup.

YesCT’s picture

alexpott’s picture

FileSize
851 bytes
97.25 KB

Add the one of the issues to the added comments. The other followup don't have related comments added by this patch.

I think all that is left is for a change record to be written.

alexpott queued 114: 2090115-opt.114.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 114: 2090115-opt.114.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
100.38 KB

Rerolled patch.

So ConfigImporterTest::testLanguage() is kind of in the wrong placed - moved it to ConfigInstallTest since this is about testing the installer and fixed the missing dependency of config_test_language on config_test.

This patch no longer converts ConfigOtherModuleTest to a kernel test we can do that is separate issue - there is no need to for it to be a web test but with #356399: Optimize the route rebuilding process to rebuild on write there's too much to change to do the conversion in this issue.

alexpott’s picture

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Great. We have follow-ups, CR, issue summary looks good, patch looks good to.

alexpott queued 117: 2090115-opt.117.patch for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

I've been reviewing this on and off during the week and haven't found anything except some nitpicks (which I lost). Committed/pushed to 8.0.x, thanks!

  • catch committed 40cc631 on 8.0.x
    Issue #2090115 by alexpott, xjm: Don't install a module when its default...
Gábor Hojtsy’s picture

Great improvement! Optional configuration however lost its translatability both on the localization server side and the locale module side:

Sorry for not tracking this in real time to make you more aware sooner.

tstoeckler’s picture

Status: Fixed » Needs work
Issue tags: +Needs change record updates

Neither the issue summary nor the change record mention the new enforced dependencies key so reopening for that.

Some further notes on that:

  1. The code related to this key is not at all clear to me from reading it. It seems that it is unconditionally merged, so it's not clear what distinguishes it from non enforced dependencies. This could certainly use a code comment (in a followup perhaps)
  2. Migrations have the concept of optional dependencies so it's unfortunate that we're following the opposite naming pattern here
alexpott’s picture

Status: Needs work » Fixed

re #124 this patch did not introduce enforced key. See https://www.drupal.org/node/2404447 for an existing CR about this.

What distinguished enforced dependencies is that they are not based on code but are enforced by the configuration itself. See CR for more.

tstoeckler’s picture

Ahh, thanks. Sorry for the noise.

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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