Problem/Motivation

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

Proposed resolution

Implement some additional pre module install configuration validation. We have two different situations to handle:

  1. Module provides configuration with an unmet dependency
    If a module provides configuration that has unmet dependencies and that module is being installed a UnmetDependenciesException will be thrown. The module install forms will catch this exception and display an informative message to the user.
  2. Module provides a configuration entity type where an already installed module provides a configuration entity of that type
    If the configuration is being created during an install due to it being provided by an already installed module then this is logged to watchdog. This is because the module being installed does not contain any incorrect configuration so we should proceed with the install.

Steps to test

  1. Install minimal
  2. Uninstall the node module
  3. Change the views.view.frontpage.yml (provided by node module) to have the following dependencies key:
    dependencies:
      module:
        - node
        - user
      enforced:
        module:
          - book

    This creates an unmet dependency

  4. Install node - this should work
  5. Install views - this should work. In HEAD, a broken frontpage view will be created, but with the patch, the frontpage view will not exist. (If you did not add the Book dependency to the view, it would exist at this point.)
  6. Uninstall views and node
  7. Install views
  8. Install node. In HEAD, this will succeed and again install the broken view. With the patch, this should fail and an error message appear indicating that a dependency is missing for the view.

Remaining tasks

Review patch
Commit

User interface changes

Message to user about missing dependencies for configuration.

API changes

  • 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.

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.

Files: 
CommentFileSizeAuthor
#89 2090115-opt.89.patch66.93 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,411 pass(es).
[ View ]
#89 87-89-interdiff.txt2.31 KBalexpott
#87 2090115-opt.87.patch69 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,401 pass(es).
[ View ]
#87 85-87-interdiff.txt4.88 KBalexpott
#85 2090115-opt.85.patch69.3 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,380 pass(es).
[ View ]
#65 2090115.65.patch41.36 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,865 pass(es).
[ View ]
#65 63-65-interdiff.txt451 bytesalexpott
#63 2090115.63.patch40.92 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,832 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#63 50-63-interdiff.txt7.61 KBalexpott
#53 front.png82.52 KBcafuego
#53 views.png54.13 KBcafuego
#53 modules.png158.04 KBcafuego
#53 install.png89.08 KBcafuego
#50 2090115.50.patch39.27 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,879 pass(es).
[ View ]
#50 48-50-interdiff.txt1.25 KBalexpott
#48 2090115.48.patch38.9 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,155 pass(es), 9 fail(s), and 1 exception(s).
[ View ]
#48 45-48-interdiff.txt9.4 KBalexpott
#45 2090115.45.patch32.04 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,998 pass(es).
[ View ]
#45 43-45-interdiff.txt507 bytesalexpott
#43 2090115.43.patch31.54 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,012 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#43 40-43-interdiff.txt3.31 KBalexpott
#40 2090115.40.patch30.88 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,673 pass(es), 34 fail(s), and 30 exception(s).
[ View ]
#40 39-40-interdiff.txt1.19 KBalexpott
#39 2090115.39.patch31.57 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,890 pass(es), 34 fail(s), and 30 exception(s).
[ View ]
#39 36-39-interdiff.txt6.99 KBalexpott
#36 2090115.36.patch26.77 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,737 pass(es), 178 fail(s), and 87 exception(s).
[ View ]

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

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: Remove hook_hook_info() + corresponding hook_info persistent cache 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
StatusFileSize
new26.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,737 pass(es), 178 fail(s), and 87 exception(s).
[ View ]

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
StatusFileSize
new6.99 KB
new31.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,890 pass(es), 34 fail(s), and 30 exception(s).
[ View ]

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

StatusFileSize
new1.19 KB
new30.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,673 pass(es), 34 fail(s), and 30 exception(s).
[ View ]

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
StatusFileSize
new3.31 KB
new31.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,012 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new507 bytes
new32.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,998 pass(es).
[ View ]

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
StatusFileSize
new9.4 KB
new38.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,155 pass(es), 9 fail(s), and 1 exception(s).
[ View ]

@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
StatusFileSize
new1.25 KB
new39.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,879 pass(es).
[ View ]

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
StatusFileSize
new89.08 KB
new158.04 KB
new54.13 KB
new82.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 screenEnable viewsCheck views UIFront 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

StatusFileSize
new7.61 KB
new40.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,832 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

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
StatusFileSize
new451 bytes
new41.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,865 pass(es).
[ View ]

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

StatusFileSize
new42.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,930 pass(es), 79 fail(s), and 29 exception(s).
[ View ]
new1.42 KB
new42.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,916 pass(es), 85 fail(s), and 29 exception(s).
[ View ]
new1.43 KB
new42.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,853 pass(es), 89 fail(s), and 29 exception(s).
[ View ]
new1.43 KB
new42.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,900 pass(es), 89 fail(s), and 30 exception(s).
[ View ]
new1.42 KB

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
StatusFileSize
new69.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,380 pass(es).
[ View ]

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

StatusFileSize
new4.88 KB
new69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,401 pass(es).
[ View ]

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

StatusFileSize
new2.31 KB
new66.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,411 pass(es).
[ View ]

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.