Problem/Motivation
Blocking #2090115: Don't install a module when its default configuration has unmet dependencies
Our configuration entity dependency system only understands names, not capabilities. That is, we only check if names match to decide if something meets a dependency, not the capabilities of a config entity.
If a module ships foo.bar.config_entity, and that already exists in the active store, we have no way to know if it is safe to install the module. This is not to say things will break - we just have no idea, because we only use names. Fixing that is another discussion.
As such, we can't install a module unless all of it's default config entities are installed, because we can't see if an existing config entity with the same name provides the capabilities that the module requires.
We could also do on-the-fly renames during module install, but that would require a new code path for what is potentially a corner case.
This issue was created as a result of @yched's comment #2138559-17: Replace UUIDs in configuration entities with a creation ID
Proposed resolution
We should block install of any module where the config entity names match an existing config entity, and provide an error screen explaining why we blocked install and how the user can resolve the issue.
"Foo module could not be installed because some of it's default configuration can not be installed. Please rename "$foo.bar" so that installation is possible."
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
(after reroll) Create a patch to address points in #86 (and update verbiage?) and make an interdiff | Instructions | ||
Reroll the patch if it no longer applies. | Instructions | ||
(after reroll) Add automated tests | Instructions | ||
Embed before and after screenshots in the issue summary | Novice | Instructions | |
discuss UX (see #84) | |||
when this is fixed, unpostpone #2090115: Don't install a module when its default configuration has unmet dependencies |
User interface changes
A new warning message.
API changes
A new way in module installation can fail.
How to test and current screenshots
- Open settings.php and at the end uncomment the lines to include settings.local.php
- Open settings.local.php and make sure extension_discovery_scan_tests is uncommented - this allows you to enable test modules
- Go to admin/modules, you can test following scenarios.
- Toggle 'Configuration test' and 'Configuration install fail test' - you should get a message after enabling on the modules page
- Toggle 'Configuration install fail test'. You should go to the confirm form and after that a message on the modules page
- Toggle 'Configuration install fail dependency test'. You should go to the confirm form and after that a message on the modules page
- Enable 'Configuration test' separately. After that enable 'Configuration install fail test'. You should go to admin/extend/config-clash and see information on the clashing configuration
Postponed until
Comment | File | Size | Author |
---|---|---|---|
#115 | 2140511-2.115.patch | 43.97 KB | alexpott |
#115 | 113-115-interdiff.txt | 2.76 KB | alexpott |
#113 | 2140511-2.113.patch | 42.1 KB | alexpott |
#113 | 111-113-interdiff.txt | 1.23 KB | alexpott |
#111 | 2140511-2.111.patch | 42.06 KB | alexpott |
Comments
Comment #1
alexpottComment #2
alexpottThe attached patch creates a warning message when a config file can not be imported -
'Active configuration with the name %config_name already exists therefore can not install the default configuration with the same name from %extension_name.'
Also interestingly we have system.module.yml in system/config but it is never being used because this file is written by the installer before the system module is enabled! So this patch removes it because pretending we have default config when we don't is a nonsense.
Comment #3
alexpottOne issue is that we have different situations where this can occur and we might want to do different things in each - which will almost be impossible.
I'd argue that in scenario 1 we'd want to prevent forum from being installed and in scenario 3 we're want to prevent module B from being installed. But we also have specific tests around forum node type preservation after disabling the forum module! With the patch in #1 doing scenario 3 using drush will lead to the following output when enabling forum for the final time:
Comment #4
alexpottAdded the related issue where we discussed what to do with config entities owned by another module but provided as default configuration by the module that is being uninstalled. In other (simpler words) should the views.view.frontpage be uninstalled when the node module is uninstalled?
Comment #5
alexpottComment #6
alexpottComment #8
xjmSo, scenario 1 in D7 is hilarious. You end up with one content type called
forum
that is a hybrid of both content types. So I don't think anyone is relying on a particular behavior there. ;)Comment #9
alexpottDrupal\system\Tests\Common\UrlTest
does not fail locally going to retest.Comment #10
alexpott2: 2140511.2.patch queued for re-testing.
Comment #12
Gábor Hojtsy2: 2140511.2.patch queued for re-testing.
Comment #14
Gábor HojtsyRerolled to apply again.
Comment #15
Gábor HojtsyThis is the output after copying node.type.page.yml into actions/config and trying to enable actions module.
Comment #16
Gábor HojtsyClosed #2205845: Configuration file name collisions silently ignored as duplicate. Applying same priority and beta target tag.
Comment #17
Gábor HojtsyComment #18
Gábor HojtsyHere is a proof of concept of a more strict approach. This checks before installing a module whether the default config will overlap with anything in active config. If so, the module is not installed and all the module installation is stopped (because further modules may have requirement to install the prior one).
Comment #20
Gábor HojtsyThe error message certainly needs work, and should not be a warning because it does stop the installation from happening. Here is an updated patch with that and some more minor fixes. Here is how it looks like:
Comment #22
Gábor Hojtsy#1986090: Profile config does not overwrite module default config on install (system.cron.yml) is related although the problem there is the overwrite is *desired*.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedi don't agree with any 'prevent install of module' validation.
here's my logic, please correct me if i'm wrong:
- modules cannot rely on configuration entities. config entities can be arbitrarily deleted/renamed/updated so that the name is the same, but everything else about them is different.
- if a module can't safely be installed without it's default configuration entities, that is a bug with the module.
Comment #24
catchThis isn't strictly true either from an API standpoint or the status quo in core. Forum module for example creates locked configuration entities:
Comment #25
Gábor Hojtsy@beejeebus: the problem lies within dependencies in configuration, you don't even need to have any line of PHP code. Let's say your module ships with field_product which is an image field. It is included in one or more displays of your products_view, that you also ship with the module. Now in Drupal 8, let's say you already have a field_product which is an SKU text field. Your module will be installed, the new (image field) field_product will not be installed because it conflicts, but that is silently ignored. The view that has the image field specific configuration for the field that is now a text field will be. Now what?
Comment #26
alexpott@swentel's point is relevant to this discussion - https://drupal.org/comment/8557879#comment-8557879
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedre #25 - or field_product could be an image field supplied by another module, and could work just fine.
or views is not installed when your module is installed, and the default view is not installed until a later date. or s/view/image/ etc etc
really, this is a bigger problem, and not at all confined to just names, and is only really solved by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall and any follow ups to that.
Comment #28
Gábor Hojtsy@beejeebus I don't think that would solve it, the dependencies are by name, so if my view depends on field_product, that does not really help, since a different field_product may be there. If the dependency is by UUID then we can ensure the concrete config, but then the UUID is not a UUID because it would be the same on each site :D
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedso this seems like a broken pattern then.
if you really need to rely on something by name, this is going to break. a lot. i don't see how declarative is going to work - the module will need to run code, and find a name that can be used, else install is going to fail often.
Comment #30
Gábor HojtsyWell, one option is config names are namespaced. Eg. instead of field_product, it would be field_mymodule_product and field_othermodule_product. That would require for core to follow the best practice and ship with default fields like field_node_body, view.node_frontpage, etc.
Comment #31
alexpottActually #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall might prevent it since that patch only calculates dependencies for incoming config reasoning that if all config entity dependencies have to be supplied in defaults - in other words you can not rely on another modules config entities.
That said we would still need to check this prior to module install and the only reason this will fail is that an import a configuration entity will not occur because one with the same name already exists. So I think the solution here is acceptable - I can't even imagine an edge case that it's not going to help us prevent.
Comment #32
jessebeach CreditAttribution: jessebeach commentedAs a naive user, this part raises alarm bells. The error prevents catastrophic data loss, but it also prevents me from building a site. I don't know, even now, what the remediation would be. Should I rename configuration files? Is this still an open issue and I just haven't found that discussion yet?
Comment #33
xjmComment #34
jessebeach CreditAttribution: jessebeach commentedI'm trying to understand the problem better. So, here is a scenario:
Comment #35
alexpott@jessebeach this only concerns default configuration. Configuration export and subsequent import is a different issue and one that we've solved through only supporting a complete site configuration import - partial import is not supported - our assumption is that the user intends to import what is in the staging directory and has obtained this configuration from their own site.
The problem this issue is trying to address is where a module provides default configuration entities which depend on each other (eg. a view and some fields). And one of the default configuration entities has an ID exactly the same as a configuration entity that already exists.
An example in core of a module that does this is forum. If a field already existed with the machine name forum_container before the module was installed this would be very messy and the installation would create all the other default configuration and use the existing forum_container field even though nothing what-so-ever mandates that is is the same type of field as defined in the forum module's default configuration.
Comment #36
Gábor Hojtsy@jeesebeach: I have a complete example with contrib modules in #25, please see that :) Happy to elaborate.
Comment #37
jessebeach CreditAttribution: jessebeach commentedI also reread #25 and realized I'm using the wrong words. I shouldn't have written import; rather, I meant install.
So, given the clarifications in #25 and #35, here's the scenario as I understand it. There is really only one use case we're trying to defend against.
The core Forum module is not installed. A contrib module is installed and creates a Config Entity called
forum_container
. The user then enables the core Forum module which fails in some way (doesn't install or uses the wrongforum_container
Config Entity).Would it be possible to prevent installation of a module that would create a Config Entity, if a Config Entity with the same name existed in any other module installed or not? Like even if just the code were in the site, but the other module is not yet enabled? Here's what would happen in this case.
Module developer Zed is creating AwesomeForums module. The Forum module is not enabled on their development site. This developer creates a Config Entity called
forum_container
in their new module. The developer attempts to enable the module and whamo, it won't enable. The error message on the install screen explains that their Config Entityforum_container
has the same name as a Config Entity of the Forum module and could lead to conflict -- please namespace the Config Entity name.This would prevent module developers from creating Config Entities that conflict with core Config Entities. Now what about contrib-to-contrib Config Entity name conflicts? The same restriction applies. Except in contrib, developers will just need to namespace their Config Entities with the module namespace to prevent collisions.
And we don't do anything more complex that this. In the case where a Config Entity being installed has dependencies to what it expects to be existing Config Entities (field_image or field_product for example), we can simply list any missing dependencies on an install confirmation screen (field_image was deleted!), state that the missing fields may cause behavior problems with the new module, and ask the site builder if they'd like to proceed.
Comment #38
Gábor HojtsyThe "configuration depends on other configuration that may be deleted already" case is handled by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall. This issue is about depending on a configuration that *exists* but may or may not be as expected. When you ship with configuration but part of your shipped configuration is not installed so your other config may or may not use dependents that it expected to use.
Comment #39
jessebeach CreditAttribution: jessebeach commentedRight, and there are three ways this could happen.
If we can prevent (1) from happening by shutting down module enabling in a module under development and (2) by disallowing a field to be given a name that a Core module might also use, then we avoid the downstream conflicts for site builders who would enable a Core module like Forum and run into conflicts.
And these restrictions, although not preventing (3), would raise conflicts quickly and promote proper namespacing in contrib.
Sorry to harp on this, but I'm trying to find a solution that would simply prevent the conflict from arising in the first place by making it really difficult to post a contrib module to d.o. that defines Config Entities with names that Core might potentially use. We want the decision point to exist primarily at development time so that developers are forced to confront these issues, not at site building time, when a site builder, who may not have a deep knowledge of Drupal, would be confronted with some tough decisions and little in terms of information to make them correctly.
Comment #40
jessebeach CreditAttribution: jessebeach commentedSo, all that being said, I think the approach in this patch is fine for what it seeks to prevent. What I'm arguing is that we should strive to never let a site builder get to the point where this install error is raised.
Comment #41
jessebeach CreditAttribution: jessebeach commentedWith the patch in #20, the Standard profile can no longer be installed :)
Comment #42
Gábor HojtsyLol, that looks like a testament to how successful this is :D
Comment #43
BerdirI guess that means that this is blocked on #1986090: Profile config does not overwrite module default config on install (system.cron.yml)?
Comment #44
jessebeach CreditAttribution: jessebeach commentedThe following code can be removed from
ConfigInstallWebTest::testInstallProfileConfigOverwrite()
, which is added by #1986090: Profile config does not overwrite module default config on install (system.cron.yml).Instead, installing a module or theme that attempts to install a default configuration file that already exists in active configuration should fail with an error message.
Comment #45
jessebeach CreditAttribution: jessebeach commentedSo, when the profile is installed after the basic modules are installed, we run into fatal errors because the profile tries to apply its default configuration files. I think what should happen, given #1986090: Profile config does not overwrite module default config on install (system.cron.yml) will already have taken the Profiles default configuration into account, is that a profile should not attempt to install any default configuration; it should only be installed in the context of a module installing.
Comment #46
alexpottImmediate reaction to #45 was +1 but the problem here is that profiles are full modules and it is perfectly possible for them to have their own configuration. They also supply default configuration entities and these cannot be installed until after all the modules have been because there are instances when it is creating configuration with multiple dependencies. A good example is editor.editor.full_html - this configuration is dependent on both ckeditor and editor but it can not be created when editor is installed because ckeditor is not installed yet (it is dependent on editor) - fun eh?
I think we need to handle profiles in a special way. Removing the already existing configuration in ConfigInstaller::gatherDefaultConfig() seems one option.
Also this patch appears blocked on #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall since the reason
Drupal\comment\Tests\CommentFieldsTest
is failing is that it uninstalls the Comment module and then reinstalls it. This currently will leave the default configuration for configuration entities comment does not provide in the active configuration - for example:views.view.comments_recent
The fails in FieldImportDeleteTest and FieldImportChangeTest were due to the fact that the field configuration provided by field_test_config is imported by \Drupal\field\Tests\FieldUnitTestBase::setUp() when it installs field configuration - this has occurred since #2082117: Install default config only when the owner and provider modules are both enabled landed but in HEAD we silently ignore the duplicate config :)
Comment #47
jessebeach CreditAttribution: jessebeach commentedNice! I jus wrote the same-ish block of code in
ConfigInstaller
to deal with filtering out already installed configs.Comment #49
jessebeach CreditAttribution: jessebeach commentedThe patch in #46 applied on #1986090: Profile config does not overwrite module default config on install (system.cron.yml) now gets us through site installation with the default configs from the standard profile without any errors.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #52
xjmWhat does "update verbiage" mean in the summary? What verbiage where?
Comment #53
Gábor HojtsyI believe that was in reference to the error message.
Comment #54
ohthehugemanatee CreditAttribution: ohthehugemanatee commented#1986090: Profile config does not overwrite module default config on install (system.cron.yml) is committed, but the patch from #46 didn't apply because of significant other changes since then. I tried to re-roll.
The resulting patch applies OK, but now I get an odd sort of WSOD with nothing in the logs, anywhere. It happens on line 1299 of core/modules/system/system.module , during system_rebuild_module_data. The offending line is
But I don't know why! This causes a problem right at the beginning of moduleHandler->install(). I'll work more on this in the coming days, but any advice would be great.
I've attached the patch such as it is for now.
Comment #55
xjmLet's use testbot for some more insight.
Comment #57
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedI tracked this down a bit further, but I still don't know why it's stopping like this. Since there's no error behavior from PHP, it might not even be an error. The values received by \Drupal::state()->set() seem just fine. It tries to set
key: system.module.files
value:
This looks totally normal and valid as far as I can tell. By this point in the Install process, we've already used this same function several times without issue. But when I call it inside ModuleHandler->install(), it dies on keyValueStore->set($key, $value); .
I don't understand why this should be a problem for this particular value in this particular place. In HEAD we call the system_rebuild_module_data() 1 line later, inside if ($enable_dependencies), which works fine for the installer because during install $enable_dependencies == FALSE. It seems like the keyValueStore, and therefore system_rebuild_module_data(), isn't useful this early in the install process.
We are using system_rebuild_module_data() to populate $module_data to skip already installed modules, and, specific to this patch, to run this key snippet:
which is part of the point of this whole patch.
TL;DR: I'm not sure what to do here.
I've attached an updated version of the patch which gets through the install process with the default profile. All I did is move $module_data = system_rebuild_module_data() and the validatePreInstall() into the $enable_dependencies == TRUE if statement, so they don't fire during Install. I don't think that's Right, but it works.
Comment #58
xjm#2090115: Don't install a module when its default configuration has unmet dependencies is postponed on this issue since the validation step would be useful there.
Comment #59
alexpottRerolled patch to account for #2262861: Add concept of collections to config storages
We still validate the modules that will can installed on ModulesListConfirmForm - and if there is any pre existing configuration inform the user that this is the case and give them links to make it easy to delete or rename everything and prevent the user from installing the modules.
Also we'll need to change Drush to have an option to force overwriting configuration so that developers can easily uninstall and reinstall modules that provide configuration entities that do not have a dependency on the module.
@ohthehugemanatee #2262861: Add concept of collections to config storages had a big impact on the ConfigInstaller - the changes in #54 and #57 needed to reimplement the early functionality on top of those changes - not really a straight reroll :)
No interdiff because of PSR4 and the extensive changes.
Comment #61
Gábor HojtsyCertainly is not doing any validation.
Sentence needs cleanup, eg. repetition.
Not a string.
Would be good to document the structure of the array.
Is that a nested associative array?
Not a string.
How does this show up on the UI, what is the user feedback now?
With the same name as? :)
Comment #62
alexpottThis issue is nasty. The new patch implements config clash detection when
Need to implement config clash page for themes. Need to add tests for the config clash pages and need to resolve what to do with NodeTypePersistenance and InstallUninstallTest because these tests become impossible / trickier now.
Comment #64
BerdirRe-roll.
Comment #66
xjmComment #67
xjmPer discussion with @alexpott and @larowlan, postponing on #2224581: Delete forum data on uninstall.
Comment #68
xjmActually postponing. :)
Comment #69
catchComment #70
alexpott#2224581: Delete forum data on uninstall has landed
Comment #71
alexpottRerolled - we have a serious issue with views because currently they do not even have dependencies on the modules that provide the entity type they are querying - see #2267453: Views plugins do not store additional dependencies
Comment #73
alexpottPostponing on #2267453: Views plugins do not store additional dependencies since doing this patch without view dependencies working is impossible due to how
InstallUninstallTest
worksComment #74
alexpott#2267453: Views plugins do not store additional dependencies has landed
Comment #78
alexpottPostponed on #1002164: The Book module can be uninstalled with nodes with a book node type still existing
Comment #79
catchComment #80
alexpott#1002164: The Book module can be uninstalled with nodes with a book node type still existing has landed
Comment #81
alexpottRerolled on top of all the changes including the new ModuleInstaller - which nicely keeps all of this complexity out of the module handler.
Comment #82
alexpottFixed issue title - this is no longer postponed.
Comment #84
alexpottThis might be green - after that we need to fix up the patch, add tests and talk about the UX.
Comment #86
Wim LeersInitial review.
Nit: Wouldn't something like "Ensure that the config to install isn't already installed." make more sense?
After reading this hunk a few times, it made sense to me though.
Nits:
Nit: s/with/on/
Nit: s/configuration already exists with the same name/configuration with the same name already exists/
?
I first read "profiles can not have config clashes" as meaning "ensure that profiles don't have config clashes", but apparently this is saying that we only need to deal with actual modules, and not with install profiles, which are treated internally as modules also but cannot have config clashes by definition.
For the UX, it'd be ideal if this would be checked *before* actually kicking of the module installation logic, and warning the user immediately.Why is this necessary if
ModulesListForm::submitForm()
checks this already? I'm guessing it's necessary for e.g. Drush, i.e. when not using the UI, but the API, to install modules?Third parameter missing in docblock.
:)
Nit: s/pre existing/pre-existing/
?
This is weird, but I think this is part of the hack you mention in
ConfigClashController::moduleReport()
, which probably still needs to be resolved as you say in #85._controller
:)Comment #87
xjmneeds te indeed.
Comment #88
YesCT CreditAttribution: YesCT commentedadding blocker tag, since this blocks #2090115: Don't install a module when its default configuration has unmet dependencies, so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2090115.
Comment #89
alexpottNew patch - response to #86 on its way...
Comment #90
alexpottre #86
Comment #91
swentel CreditAttribution: swentel commentedTagging - looking at the UI and tests.
Comment #92
dawehnerLet's be honest.
Comment #93
swentel CreditAttribution: swentel commentedDid some cleanups and added tests in ConfigInstallWebTest.
The UI now also catches the exception and shows a basic message. Depending on how you toggle modules in the UI, different things happen:
One consequence is that the check for preexisting configuration needs to happen in both submit methods which is kind of stupid right now.
Also should we check on pre existing configuration on new modules or not ?
Comment #94
swentel CreditAttribution: swentel commentedAdded test scenario's and screenshots to summary.
Comment #95
swentel CreditAttribution: swentel commentedComment #96
pfrenssenThis is not actually used.
This documentation is initially a bit hard to understand, unless you already know that we are going to hunt for config clashes, and that this is the only thing that will cause configuration to be considered invalid.
I also think !empty() would be better than count() here.
According to coding standards, object properties should be lowercamelcased. This is not strictly followed in core though.
s/_//
s/_//
"pre-existing" instead of "pre existing".
This can only work if config_install_fail_test depends on config_test. I looked this up in the info.yml file to verify an this is indeed the case. Maybe explain this here to avoid confusion?
config_install_fail_dependency_test depends on config_install_fail_test which depends on config_test. This is well tested :)
Also worth a line of documentation.
Got half way through the review. Chimay Blue is clouding my judgement so I better stop :P
Comment #97
dawehnerBack to needs work.
Comment #98
dawehnerDon't we have to take into account install profiles here as well?
Note: This code is pretty much exactly the same as in
::findConfigEntityDependentsAsEntities()
so I would argue we should better replace the usage there. The only difference is that the other implementation supports multi-loading.Note: In that context you certainly have $config_installer available as variable.
It is a little bit odd that this is not part of the config module, but well, it will otherwise require the config module to solve some of those conflicts.
Comment #99
babruix CreditAttribution: babruix commentedRerolled patch from #93.
Applied proposed changes from comments #96 and #98.
Comment #101
babruix CreditAttribution: babruix commentedAdded back try-catch block around
$this->moduleInstaller->install
.Comment #102
webchickAw, shoot. It's a 0 byte patch. :( Any chance you've still got the changes laying around?
Comment #103
alexpottImproved the exception object so we can do a translated version in the UI and improved the test.
We also need to worry about themes :(
I've also checked that the comments in #96 and #98 (thank you @pfrenssen and @dawehner) where addressed in #99 and they are. The interdiff in #99 does not show the full scope of the changes made.
Comment #105
Wim LeersOverall, this looks great!
Code review
s/extensions/extension/
Elegant :)
s/from/by/, to match the function name?
s/module/extension/
s/as if/if
s/as a module it will override it/because it would be overridden/
?
"This" === config_install_fail_test. Not as clear as it could be in the comment.
EDIT: it becomes clear 12 lines further though.
"to..." -> unfinished?
Should have a blank line in between them.
Unused; probably copy/paste leftover.
This doesn't match; you don't get the factory, but a specific store.
It's correct in the class property docs.
?
There's a lot of code duplication between these two. Can we change it so it's DRY? Or why don't we want that? Because it's out of scope?
Why 60 seconds? Why not 5, why not 300?
"list of configuration" versus "it"
Manual testing
Having tried this, I now understand the 60-second caching (upon trying to install a module and a config clash being detected, the user is redirected to
admin/modules/config-clash
); but I don't understand why you did it this way. Why not make it part of the configuration form's output? Why a separate route? Because that's the only reason why there's this (IMHO awkward) 60-second caching AFAICT.Comment #106
alexpottI've removed the UI completely since deciding what to show people for configuration overrides that clash is impossible. We might consider implementing a UI in a folllow-up issue. Doing that fixes quite a few of the points raised by @Wim Leers in #105. All the relevant feedback is also addressed in the attached patch.
I've implemented the check for themes too.
There are extensive tests for this change in ConfigInstallWebTest and ConfigInstallTest.
Comment #108
alexpottThe TourTest fail is interesting. We inject the active configuration into the ExtensionInstallStorage. If we use the ConfigInstaller to find pre-existing configuration first then to an install and there are collections then the wrong active configuration injected.
Comment #109
alexpottNow for the patch :)
Comment #110
Wim Leers#108: great find! And simple fix in #109.
I can only find nitpicks this time.
s/becuause/because/
:)
Great expansion of this patch! Very valuable information indeed.
s/through UI/through the UI/
.
These are now unnecessary/unintentional changes.
Ideally the symmetry here would also be reflected in the comments.
Comment #111
alexpottThanks @Wim Leers
Also spotted some unnecessary testing in
ConfigInstallWebTest::testInstallProfileConfigOverwrite()
Comment #112
Wim LeersWent over it one more time. Found two very very silly nitpicks, that can easily be fixed upon commit. Patch looks great :)
Unnecessary blank line.
This is a copy/paste error; the other test extensions' info files don't have a comment. Can be deleted.
Comment #113
alexpottA reroll due to #2392319: Config objects (but not config entities) should by default be immutable and fixing #112 - thanks again @Wim Leers
Comment #114
catchNo major complaints but two questions on things that confused me a bit.
There's a lot of code shared between this and ConfigInstaller::installCollectionDefaultConfig().
And ConfigInstaller::findPreExistingConfiguration() als does some similar things as installCollectionDefaultConfig(). Any way some of that code can be factored out?
So just to confirm findPreExistingConfiguration() includes any configuration shipped by install profiles?
OK so install profiles are skipped before this code runs. But install profiles are included in findPreExistingConfiguration().
I'm not really clear from this what happens in the following cases:
1. Install profile and module are installed at the same time.
2. Install profile is installed first, then module is enabled later.
3. Install profile is installed with module at the same time. Module is uninstalled. Module is re-installed.
Comment #115
alexpottYes they look similar but there are subtle differences that make refactoring to share code harder work and more verbose. I've renamed listDefaultConfigCollection to be listDefaultConfigToInstall since that more accurately describes what it does.
Some of this is tested in ConfigInstallWebTest::testInstallProfileConfigOverwrite() but it could be improved - this is functionality is not introduced by this issue so I think this is a separate issue.
Tentatively setting back to rtbc because all I've done is rename a method.
Comment #116
webchickBoing!
Comment #117
catchHmm OK. I'd somewhat assumed that profile overrides would continue to work if a module is uninstalled and re-installed, but quite happy this isn't the case.
Committed/pushed to 8.0.x, thanks!
Comment #120
jhedstromAdding related issue #2431157: Review PreExistingConfigException special casing for install profile.
Comment #121
apaderno