Problem/Motivation
Default configuration entities could depend on a module that is not installed.
Proposed resolution
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 an unmet dependencies error.
- 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.
- 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
- Install minimal
- Uninstall the node module
- If testing the patch: move config/optional/views.view.frontpage.yml to config/install/views.view.frontpage.yml
-
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
- 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
- Install views - this should work.
- 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.
- Install telephone
- 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.
Comment | File | Size | Author |
---|---|---|---|
#117 | 2090115-opt.117.patch | 100.38 KB | alexpott |
#117 | 114-117-interdiff.txt | 4.48 KB | alexpott |
#114 | 2090115-opt.114.patch | 97.25 KB | alexpott |
#114 | 111-114-interdiff.txt | 851 bytes | alexpott |
#111 | 2090115-opt.111.patch | 97.22 KB | alexpott |
Comments
Comment #1
catchThis 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.
Comment #2
catchOK we slightly re-purposed that issue to explicitly include the API and one implementation, so this does have a hard dependency now...
Comment #3
catchComment #4
xjmComment #5
xjmNow postponed until after #1808248: Add a separate module install/uninstall step to the config import process.
Comment #6
jessebeach CreditAttribution: jessebeach commentedComment #7
jessebeach CreditAttribution: jessebeach commentedComment #8
jessebeach CreditAttribution: jessebeach commentedOne postponing issue down. The other postponer is no longer, itself, postponed.
Comment #9
jessebeach CreditAttribution: jessebeach commentedUnpostponed!
Comment #10
jessebeach CreditAttribution: jessebeach commentedComment #11
xjmComment #12
alexpottI'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
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.
Comment #13
catchThat 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.
Comment #14
xjmI 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?
Comment #15
xjmPer @catch.
Comment #16
catch#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.
Comment #17
xjmComment #18
xjmComment #19
alexpottIf 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.
Comment #20
xjmDiscussed 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.
Comment #21
BerdirHm, 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?
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?
Comment #22
catchNow 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.
Something like that sounds OK too, although you could have configuration with multiple other dependencies, not sure it works for that.
Comment #23
catchComment #24
catchComment #25
alexpottnot anymore
Comment #26
xjmComment #27
YesCT CreditAttribution: YesCT commentedComment #28
Gábor HojtsyRe #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.
Comment #29
BerdirI 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.
Comment #30
alexpottre #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?
Comment #31
BerdirOk, 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.
Comment #32
alexpottComment #33
alexpottAn 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.
Comment #34
Gábor HojtsyIMHO 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 :)
Comment #35
BerdirThat is pretty much what I wrote, I think :)
Going to work a bit on this.
Comment #36
alexpottoops @Berdir - I started some preliminary work on this last night - I should have posted a comment - my bad.
Comment #38
BerdirFor 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?
Comment #39
alexpottWe 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.
Comment #40
alexpottSome unnecessary change in #39
Comment #43
alexpottSome more fixes.
Comment #45
alexpottOkay 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...
Comment #46
alexpottComment #47
dawehnerCan't we just remove the IF? The second if will never be TRUE if the install profile is empty.
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
Previously we used Settings::get() is there a reason we can't here?
Urgs
Do we plan to include core into the configuration file at some point? Might be interesting for weight problems.
Helpful exception message!
I really like this change!
meh
Comment #48
alexpott@dawehner thanks for the review...
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.Added tests as well. And rerolled following #2406543: Remove ConfigFactory::setOverrideState and ConfigFactory::getOverrideState()
Comment #50
alexpottre #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.Comment #51
larowlanManual testing for office hours:
install minimal
install node
install views
check the frontpage
Comment #52
cafuego CreditAttribution: cafuego commentedComment #53
cafuego CreditAttribution: cafuego commentedManually tested. Installed minimal (which already includes node). Then enabled views and visited /node (as the default front page location wasn't changed)
Screenshots below:
Comment #54
alexpottCleaning up IS.
Comment #55
alexpottAdded a small CR to document the new install task (
install_install_profile
) and the fact that the install taskinstall_profile_modules
no longer installs the install profile https://www.drupal.org/node/2413003Comment #56
BerdirStarted 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.
Comment #57
BerdirTook 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.
Comment #58
cilefen CreditAttribution: cilefen commentedI 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:
Comment #60
effulgentsia CreditAttribution: effulgentsia commentedGiven #54, I wanted to remove the "Needs issue summary update" tag, but a few things I don't understand from the current IS:
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.
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.
Comment #61
alexpottAnswered #60 by updating the issue summary.
Comment #62
larowlan6 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?
<3 seeing this hack go by the wayside
As much as I love the phrase 'Bail', not sure it meets our standards. Perhaps 'Throw an execption'
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)
lovely use of php scope
sounds like something I'd have come up with :-P
<3
We have this hunk three times, would it make sense to have a method on UnmetDependenciesException to generate this message? UnmetDependenciesException->getDependencyMessage()? or similar?
Comment #63
alexpottThanks @larowlan
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).Comment #65
alexpottMore KernelTestBase weirdness for the same reason.
Comment #66
xjmDid a first round of manual testing just using the steps in the summary. There are three things I found unintuitive:
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.
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?
Also in step 8, the error message I receive when I try to install node is:
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.
Comment #67
xjmTo 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.
Comment #68
xjmUpdated the testing steps to clarify what's different between HEAD and the patch.
Comment #69
alexpottThe 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
inConfigInstaller
which feels icky - but in this case necessary.Comment #70
Berdir1) 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 ;)
Comment #71
xjm@alexpott:
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.
Comment #72
xjmOh, looking at #57 from @berdir:
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.Comment #73
xjmI 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 moveznode
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:
Whereas this installation fails with an exception:
If I call the module
anode
instead ofznode
, 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.Comment #74
xjmI 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...
Comment #79
xjmOkay
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.
Comment #80
xjm@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?
Comment #81
alexpottSo 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.
Comment #82
alexpott@xjm also should have said in #81 the module ordering randomness due to the dependency graph is a great catch.
Comment #83
xjmSo 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...
Comment #84
xjmOr is "optional" config installation another separate and final step in installation and synch?
Comment #85
alexpottThe 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 innode/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 extensionsconfig/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:
config/install
has to have its dependencies met during install - if it does not then you get the unmet dependencies error.config/optional
. During an extension's installation all configuration objects, whose dependencies can be met, are installed.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)
Comment #86
BerdirThat 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 :)
Comment #87
alexpottNew 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.Comment #88
alexpottRe #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?
Comment #89
alexpottBye logging service.
Comment #90
BerdirI 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 :(
Comment #91
alexpott#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.
Comment #92
effulgentsia CreditAttribution: effulgentsia commentedI haven't reviewed the patch itself, but +1 on the new direction as explained in #85 and subsequent comments.
Comment #93
swentel CreditAttribution: swentel commentedSo this check can be removed completely ?
exits => exists
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 ?
80 chars
4 = 5
it's ?
Comment #94
swentel CreditAttribution: swentel commentedre: 3 - ignore that idea. Sometimes, we don't even know if the file is for a config entity or a simple configuration anyway.
Comment #95
alexpottComment #96
BerdirUff, long review below. Looks overall very good, comments are mostly about details, naming and questions.
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).
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.
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?
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)) ?
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, ..." ?
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?)
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?
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.
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.
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.
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?
I guess this documentation is just moved, is it worth clarifying that it returns configuration object *names*?
nitpick: missing . at the end.
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... :)
migration... config entity... I've seen you have the same opinion ;)
is it worth unsetting enforced after merging it, so that we don't have to loop over it below?
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).
I think in other cases we just have custom constructor arguments, but this is fine as well.
Interesting, here the comment changes from default to optional configuration, which kind of supports my point about the method name above :)
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?
#2443485: Remove extension:views cache tag and other views related cache improvements should remove the need for ->clear() :)
Comment #97
jibranNW as per #96
Comment #98
alexpottAwesome review @Berdir - thanks!
Re #96
InstallAllOptionalConfig
and replaced it withInstallOptionalConfig
that accepts a storage and a prefix so it can be used to install single pieces of optional configuration or configuration from alternative directories.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?Comment #99
alexpottComment #100
alexpottComment #101
Berdir4. 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.
Comment #102
alexpottI think given the issue summary:
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.
Comment #103
alexpottHere's a patch that handles and tests the expectations around the ability of install profiles to override configuration
Comment #104
BerdirNew tests look great.
nitpick: ;;
should have an empty line above @var.
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.
Comment #105
alexpottFixed everything @Berdir mentions in #104. Thanks for the review.
Comment #106
BerdirGreat. 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.
Comment #107
alexpottI 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:
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:
The patch also contains tests for all of these expectations plus we can simplify the code in ExtensionInstallStorage that deals with profile overrides.
Comment #108
alexpottDamn EOF new lines :)
Comment #111
alexpottFixing 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.
Comment #112
YesCT CreditAttribution: YesCT commentedHere is an issue: #2451363: Ensure install_profile is exists in settings.php after installation
Comment #113
alexpottFollowups created:
Comment #114
alexpottAdd 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.
Comment #117
alexpottRerolled 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.
Comment #118
alexpottAdded a CR https://www.drupal.org/node/2453919
Comment #119
BerdirGreat. We have follow-ups, CR, issue summary looks good, patch looks good to.
Comment #121
catchI'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!
Comment #123
Gábor HojtsyGreat 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.
Comment #124
tstoecklerNeither the issue summary nor the change record mention the new
enforced
dependencies key so reopening for that.Some further notes on that:
optional
dependencies so it's unfortunate that we're following the opposite naming pattern hereComment #125
alexpottre #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.
Comment #126
tstoecklerAhh, thanks. Sorry for the noise.
Comment #127
Gábor Hojtsy#2457551: Regression: optional default configuration is not translatable anymore in locale is complete and ready for review!