Updated: Comment #94
Problem/Motivation
It is unclear what should happen to configuration entities (configurables) when installing and uninstalling a module that contains configuration connected to another module's API
For example:
- Views allows modules to provide default views
- views_test_config.module has its own config directory and its own default views
- The config files are in Views' namespace, and "belong" to it
- uninstalling views_test_config will not remove its config. Only uninstalling views.module will do that
Proposed resolution
In part because this issue is blocked on #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed and in part because we want to limit the scope of this issue, we have decided to make this issue a "meta" issue by creating two child issues:
#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall
#2082117: Install default config only when the owner and provider modules are both enabled
#2090115: Don't install a module when its default configuration has unmet dependencies
If a module creates a node type, for example the Forum module creates a node type of forum. Then is up to the module to decide if this remains after uninstalling if there are no dependencies. For example, it is entirely reasonable to install forum, get the vocab, get the content type, create content, then uninstall forum and build a new forum listing with views or whatever.
Remaining tasks
Determine when configurables are installed and uninstalled.
User interface changes
TBD
API changes
TBD
Related Issues
#734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled
#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
#1444766: Determine method for automatically cleaning up configuration after a module is uninstalled
#1497268: Add revert functionality to Config entities
#1515312: Add snapshots of last loaded config for tracking whether config has changed
#1605324: Configuration system cleanup and rearchitecture
#1605460: Implement dependencies and defer process for importing configuration
#1620140: Allow synchronizing config entities from default config when modules are updated
#1765936: Invoke ConfigStorageController::delete on module uninstall and add tests
#1776778: Non-overridden default views should be disabled when their providing module is disabled
#1888218: Default configuration entities provided by a module should include the module name in the machine name
#1932600: Configuration names do not always begin with the extension that actually owns the configuration
Comment | File | Size | Author |
---|---|---|---|
#83 | 1776830.patch | 893 bytes | Anonymous (not verified) |
#75 | 73-75-interdiff.txt | 7.18 KB | alexpott |
#75 | 1776830-75.patch | 20.31 KB | alexpott |
#73 | 1776830-73.patch | 18.08 KB | alexpott |
#53 | config-1776830-53-tests.patch | 14.6 KB | xjm |
Comments
Comment #1
tim.plunkettThis just adds back the removed code.
Now it will try to remove config it provided, or any belonging to it. Both are necessary.
Comment #2
tim.plunkettThis needs tests, but let's see if it breaks anything as is.
Comment #3
xjmWorking on clarifying this a bit. This is kind of a neat bug.
Comment #4
xjmAdded some comments clarifying why this is necessary and reordered the code to a more logical sequence. I also added an
@todo
and@see
that probably should not be a part of the final patch in this issue, but that will need to be resolved as part of #1398040: Detect if default configuration of a module has been changed, and allow to restore to the original.Next I'm going to add some test coverage. There's none for uninstallation whatsoever, so we definitely need to codify our expectations here.
Comment #5
xjmAlso related to this problem space (@tim.plunkett fished these up):
Comment #6
sunoh wowowow... thanks for jumping on this ;)
However, this has *big* architectural consequences, and directly ties into these issues:
#1677258: Configuration system does not account for API version compatibility between modules
#1605460: Implement dependencies and defer process for importing configuration
#1765936: Invoke ConfigStorageController::delete on module uninstall and add tests
I don't think the CMI team had sufficient time to think about this detail of the system yet (at least I only captured many related considerations along the way, but never really thought through the entire problem space), so I think we should think through this first and also have a call to discuss.
Also demoting to normal, since nothing is really broken here that wasn't broken before already. (In the worst case; i.e., if all fails, modules would simply have to implement hook_modules_uninstalled() to properly clean up config that relates to other modules.)
Comment #7
tim.plunkettSee also #1765936: Invoke ConfigStorageController::delete on module uninstall and add tests
Comment #8
sunI do not understand this patch. :) The default configuration of a(ny) module is completely irrelevant for the problem space at hand. ;)
Along the lines of #1677258: Configuration system does not account for API version compatibility between modules, and I believe that @merlinofchaos wanted to see this as well, I was much rather thinking of force-introducing a
property for Configurables (ConfigEntity), which, like the UUID property, would only be set once upon creation.
That way, it would technically be possible to scan for all configurables that belong to a certain module being uninstalled.
How to actually perform that scan ("grep"), however, is a different, non-trivial question.
And of course, whether it's even valid to delete all related configurables upon uninstalling a module, is an entirely different question (and can of worms)...
Comment #9
gddIn order to gain some movement on this, I would like to make a proposal for how uninstallation works. This proposal represents how I always expected the system to work in my brain. It will be easy to implement and if we want to grow on it later we can.
Module foo provides views integration in the form of a default view. It also provides its own module-specific settings.
-- modules/foo/
foo.module
foo.info
/config
foo.settings.yml
views.view.foo.yml
The foo module owns both of these pieces of config and is ultimately responsible for their installation and cleanup. When installed, this config is copied to the active config directory. It is copied whether Views is installed or not. When foo is uninstalled, its default config directory (core/modules/foo/config) is scanned, and all matching config is deleted.
If Views is now installed, then that the view foo will be listed. Great! What happens when Views is uninstalled? views.view.foo.yml is not touched. This is as it should be. If we delete that view then and there, then when Views is reinstalled we have no way to know that it should be revived when Views is reinstalled. Yes this does mean that there will sometimes be config sitting in this directory that is not doing anything, and I think that's fine and really a very minor problem when compared to the other issues that we face here.
Now what about views created via the UI? They should be removed when Views is uninstalled. Currently we have no way to identify these views vs the ones supplied by modules, and that's a question that needs to be answered, because we need to be able to do that. This is where, perhaps, the 'module' key in config supplied by config entities suggested by sun would be useful. It would also mean we have to open all the views to check that on uninstallation. Another option, if we go forward with the 'manifest' files for config entities (see #1697256-44: Create a UI for importing new configuration) then we can store this kind of 'metadata needed for listings and installation' in there. I had already discussed with dawehner the possibility of storing enabled/disabled in there as well.
Other than the question of UI-created config, this is simple and we can do it now.
Comment #10
tim.plunkettI don't know how often one completely uninstalls the Views module, but I can't imagine it's too often.
Having a 'module' key for shipped config sounds like a reasonable solution.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm not opposed to the module key, but i don't think it's going to be particularly efficient if we really want to clean up all files that a module owns, for a couple of reasons.
first, "The foo module owns both of these pieces of config and is ultimately responsible for their installation and cleanup. When installed, this config is copied to the active config directory. It is copied whether Views is installed or not. When foo is uninstalled, its default config directory (core/modules/foo/config) is scanned, and all matching config is deleted."
bar module may have shipped views.view.foo.yml, which was then deleted. then a completely different view is created via the UI, that also happens to be called views.view.foo.yml. so, it's never safe to delete config entities (views, image styles, blocks, ...) without first reading them in to memory and checking for this module tag. this one is not so bad from the SCAN ALL THE THINGS point of view, because we only have to look at config that is in the module's config dir.
but wait, there's more. what about config that a module installs dynamically in hook_install()/hook_enable()? do we want to support cleaning this up? if so, we need to scan all the config for all the config entity types, check the module tag, delete if it matches.
Comment #12
xjm#9 sounds like what I'd expect to happen. This however sounds bad:
Plus what @beejeebus points out in #11. Does the providing module simply need to be part of the config name?
Comment #13
xjmWe just discussed this as the VDC sprint and so far the problem is knowing which
views.view.*.yml
files "belong" to views, and which belong to node etc. and should be left alone, per #9. There's a few possible solutions. When (e.g.) Views is uninstalled, we could:views.view.*.yml
files, and not delete those if they're found in one of those other modules. Con: Lots of filesystem action.views.view.node
orviews.view.views
instead ofviews.view
.So we can discuss what the appropriate solution would be, but we have consensus somewhat from today's discussion on the expected behavior. I'll write a test (for real this time) and describe the expected behavior with that.
Comment #14
xjmAttached just extends the existing test coverage for the installation and uninstallation of one module and should pass. No second module yet. I had to switch
ConfigInstallTest
to a web test because I was getting a kernel error at the end of the test withDrupalUnitTestBase
(even after adding$install = TRUE
toenableModules()
). If someone knows how to fix that, feel free.Next I'll add another test module that provides config for
config_test.module
's namespace and API.Comment #15
xjmHelps to attach the patch.
Comment #16
sunTwo things I strongly object to:
Installing all default config that a module provides at installation time, regardless of whether the "other" module exists.
This would be fundamentally wrong, because - if the "other" module (e.g., Views) is not enabled - the default config that is being installed did not go through Views API. That must not happen. If it happens, then the extension architecture is fundamentally flawed.
Default configuration of one module that is supplied for another module can only be installed if the API of the other module exists and is functional.
Overloading the semantics of config object names via
$owner.$type.$implementor
.We have a single semantic in config object names in that the part up to the first period is the name of the owning extension. That should remain the only enforced assumption in the configuration system. Introducing a special meaning to the third part will inherently lead to massive problems and artificially limit the system.
Comment #17
tim.plunkettSo, how do we know that
core/modules/node/config/views.view.frontpage.yml
belongs to Views module when Node is installed while Views is disabled?
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedi agree with #16.1 - that's a real issue, and it will break stuff that relies on ConfigEntity API calls. yay.
as for #16.2 - meh. we're already stuck with having to cram lots of information into filenames, by design. i have sympathy with keeping it simple as sun suggests, but there are obvious limitations without metadata about config objects existing outside of the filenames, which we've resisted as much as possible so far.
Comment #19
xjmComment #20
xjm@sun, @beejeebus, we were going in the direction of using the manifest file and not adding an additional component to the name. The four options in #13 are separate possible solutions.
WRT copying config that is not yet enabled -- the idea was that the config would be copied but not used. I can see the concern about the API not being available, though.
So, if you enable (e.g.) views after other modules, views has to go look in every single module's config directory to see if they have any views? (That's not the current behavior btw; currently, the frontpage view is copied in when node is installed even if views is disabled.)
Comment #21
gddThe simple solution is using the manifests to register who is responsible for the config, I don't really see another good option. So views.view.frontpage.yml could have
module: node
in its manifest file entry. We're already going to be using the manifests for #1826602: Allow all configuration entities to be enabled/disabled which sets the precedence of using them for more than their original intention. Views created through the UI would be registered asmodule: views
.As far as 16.1, lets say node is installed and views.view.frontpage.yml is copied to the active config directory. Now you install views. Views can scan for views.view.* in the active directory and make sure they get properly run through the appropriate APIs. Then it can install its own default config. It's better than scanning through all the module directories.
Comment #22
gddComment #23
xjmThis actually makes the most sense to me. This way, if a module has reason to do stuff to the provided default configs on "create", it can; and if not, it just works.
Comment #24
sunre #17:
By taking the only semantic of config object names into account: It starts with "views", so the "views" extension must exist and be enabled in order for this config file to be installable.
It can't get simpler than that.
re #20.2:
Views has of course no business in doing that. Either the configuration system, or module_enable() has to perform that task.
re #21.1:
What you suggest there is a clear abuse of manifest files. The moment manifest files turn into a jack of all trades, we end up in a unreliable, unmaintainable, and continuously broken system. This is guaranteed to break so horribly bad, you will not want to use Drupal anymore.
re #21.2:
That would not only break API contracts on all levels, but also breaks any kind of remotely left sense of staging configuration.
Can you create a comment entity record if Comment module is not installed?
The moment you install config for an extension that does not exist, what happens if you install the extension? Your configuration gets overwritten. What happens if you update the extension? Who will maintain the configuration that no one owns?
Comment #25
gddWho does it is avoiding the fact that it still has to be done, and going into every module's default config to find potential matches for modules being installed is going to be an arduous task.
In the current system, why would views.view.frontpage.yml get overwritten when views is installed if views does not provide a file named this? The only reason this would happen is because of a name collision which is going to cause other problems anyways.
No I never said you could?
Views will go in and update all the views that are in the active config directory of course. I don't understand what you're getting at here.
Can we pull back on the rhetoric please? It doesn't help anything.
Comment #26
sunUninstallation of manifest files is fixed in #1831776: removing manifest files from uninstalled modules already.
Comment #27
sunRemoved duplicate code.
Comment #28
sunSorry, that was ill-minded.
Fixed default config merge code path is impossible to reach.
Comment #30
gddGiven that there is no kind of consensus on this issue I don't think a patch is appropriate yet. Lets try and come to some agreement about the matters at hand first.
Comment #31
sunI thought I had sufficiently clarified already.
That's a simple, natural behavior. Please explain where you see an architectural flaw in this.
Comment #32
gddAll I said was that the discussion above between various people in this issue hadn't been concluded, and I'd like to see everyone involved getting a chance to weigh in until we move forward. Putting forth a patch while a topic is still being debated implies that everyone is on board when this is not the case here (yet).
I assume that in the above scenario, if module Foo provides views.view.foo, then that is removed if either Foo or Views is uninstalled?
Also while the above talks about install/uninstall, I assume that if an extension is enabled/disabled the configuration remains in place?
Comment #33
xjm#1806334: Replace the node listing at /node with a view is convincing me that it is probably better for the installation of the API-providing module (from the namespace) to be the operation that installs all default config everywhere, per #30. I have no idea why simpletest is invoking
views_config_import_create()
with Views allegedly disabled, but the whole problem can hypothetically be avoided if the config system skips overviews.view.frontpage.yml
when installing node if views is not installed.So, a few scenarios.
views.view.default.yml
is not copied.views.whatever
and installing them.views.view.frontpage.yml
is now installed.views.view.frontpage.yml
is now installed, because views is enabled.views.view.frontpage.yml
is deleted.views.view.frontpage.yml
, because only uninstall is destructive.Comment #34
gddI think the operation should still take place if one or both is installed but disabled. In other words in the following:
views.view.frontpage.yml
to active config.views.view.frontpage.yml
still gets deleted.This is essentially as changing 'enabled' to 'installed' in the following statement:
While I think this just makes good sense, it also means we only have code dealing with default config at the install/uninstall level, while enable/disable remain blissfully ignorant.
Comment #35
sunThat's not a problem, but instead, exactly right: enable/disable is not supposed to trigger any kind of critical actions.
I will admit that #1826602: Allow all configuration entities to be enabled/disabled slightly muddies the waters there, but definitely not to an extent that couldn't be resolved with a
drupal_flush_all_caches()
.#33.3 + .4 unlocked a secret treasure:
1) Nope, views.view.frontpage would only be deleted if Views module is uninstalled. Not if Node is uninstalled.
2) ACK.
3) You've successfully unlocked the door to insanity there. Thanks for taking the time to think through the consequences here! Your one-line sentence is essentially a summary of these issues:
#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
#734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled
(Actually, I wasn't able to find the most important related issue, so consider this list as incomplete.)
Anyway, that concern is 100% valid, and I'm afraid, it leaves us with unresolved problems (i.e., back to the drawing board). The "simple" solution might be to take disabled extensions into account as well when a new extension is installed, but off-hand, I'm not 100% sure about the consequences of that. Without much consideration, it sounds scary/invalid to me.
Likewise, uninstalling and re-installing config on enable/disable sounds equally scary/invalid to me...
In the end, it is perfectly possible that we're running into one of the most awkward architectural flaws of Drupal here (a.k.a. the current notion and behavior of "disabled extensions"), and, it is perfectly possible that it is straight-out impossible to find an appropriate or correct answer to the question/concern being raised. The correct/proper answer might very well mean that we need to change the meaning and behavior of disabled modules/extensions. I'm totally aware this sounds like a lot, and indeed, it is. We should not be afraid of changing a behavior/system that is utterly broken in the first place though. Not necessarily in this issue, but if this issue provides sufficient architectural reasoning for finally fixing and eliminating a broken aspect of the system, then that is only for the better.
In any case, really, thanks for thinking through it in-depth, and raising those fundamental architectural concerns before it is too late. :) Since the most important part is buried into a list of other bullet points, I'd bet that we need some reminders of that, here, as well as in other issues. ;)
Comment #36
xjmAh, this puts us in "missing/broken handler" land then: #1822048: Introduce a generic fallback plugin mechanism. It's reasonable, though; a module could provide a view that has nothing to do with handlers provided by that module, and if I customize that view, I wouldn't want my customized view to disappear just because I uninstalled the other module.
I'll add some more tests based on the above.
Comment #37
gddI'm not sure I agree with this. The view is provided by Node module, and without Node modules the view won't work properly (or perhaps at all.) It is easily managed by install/uninstall from both sides, so I'm not sure I see the benefit in leaving it there in this situation.
This is exactly what I was proposing in #34. It may have some unrealized side effects, but any other option is going to involve serious rethinking of things from the ground up, and is a much better option than adding operations on enable/disable. We can at least use this as a starting point and see if we run into any serious problems.
Comment #38
sunMeanwhile, I know why it is scary and indeed invalid:
Disabled modules are disabled. Their services are not registered. Their entity types are unknown. They're not loaded to begin with. You cannot invoke API functions in disabled modules.
Comment #39
gddThen I guess we need to run the same code on enable as we do on install. Which sucks but I don't really see another option.
Comment #40
sunIMHO, it rather means we need to get rid of disabled modules (entirely), or significantly change its behavior.
It does not make sense to me to work around the architectural flaw that the current notion of disabled modules present.
Essentially, what we're asking for here is this:
1) If we're installing config of a module because it was installed, then "installed" has to mean "installed." Nothing else.
2) If we're uninstalling config of a module because it is uninstalled, then "uninstall" has to mean "uninstall." Nothing else.
3) It is impossible for the configuration system to cater for a notion of "disabled modules" without implanting a 'disabled: 1|0' property into all configuration objects and adjusting the Config class to consider a successfully loaded config object that is disabled to effectively return that the load was not successful.
As mentioned in #35 already, what we're facing here is yet another consequence of a terrible architectural flaw of the current notion of disabled modules. It is not the only one. Reality is, it's just the next one in line. We're facing sufficient other problems that are almost impossible to resolve in various other issues already. We should not be afraid of changing that notion.
Comment #41
xjmYeah I'm thinking we have to.
We could check to see if the config is already installed... except then, what if it's from an "old" default before an update from the module? A solution for that probably depends on #1515312: Add snapshots of last loaded config for tracking whether config has changed and #1620140: Allow synchronizing config entities from default config when modules are updated.
Comment #42
gddIs removing disabled modules on the table? Last I saw an issue around that it was pretty well rejected, but I admit that was a while ago. Is there a more recent issue around this discussion? I searched and didn't see anything immediately obvious.
If we are in fact going to take that on I'd rather postpone this issue and separate the discussion of disabled modules as a concept into a new one.
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentednot sure where that ended up either, but +1 to killing disabled.
Comment #44
xjmThe issue was re-scoped: #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
Comment #45
gddI would say that the likelihood of any major rearchitecture going on around disabled modules in this release cycle seems pretty unlikely, and the state of that issue only make me believe it more strongly. I'm not 'afraid' of doing it, I'm more trying to be realistic about it. We really need to come up with a solution that works with the current disabled modules architecture. If disabled modules go away or get fixed, and we end up throwing our solution out, then fine. In the meantime we have something we can move forward on without being delayed on a controversial issue that isn't going to get solved anytime quickly.
Comment #46
sunAs mentioned in #40, the only remotely possible way I can think of would be to:
1) stuff a new, hard-coded, and enforced property into all configuration objects, which denotes whether the owning module is enabled.
2) if Config::load() is invoked on a config object of a disabled module, pretend the config object would not exist (even though it does).
3) when Config\Storage::listAll() is invoked, we can no longer just list matching objects, as we need to read them to check the disabled status of each, and only return those config object names that are owned by disabled modules.
4) whenever a module is disabled, grep for all existing config objects that it owns, and flag them as disabled.
5) whenever a module is enabled, grep for all existing config objects that it owns, and flag them as enabled.
This cannot use
state()
or any other outside helper mechanism, since this special disabled status of config has to be staged, in the same way the owning module's status is staged along with the system.module configuration. It needs to baked right into the config objects themselves.That sounds like a terrible hack to me though, which is guaranteed to slash back, so any other option is better before we even attempt to do that.
The only other option I can think of is in attached patch, but it is so scary that I immediately stopped after those two lines...
Comment #47
xjmEesh.
Comment #48
xjmMaybe we should fix the install/uninstall part first, since that's not an edgecase, and address the enable/disable horrors in a followup (edit: once we have snapshots and an API for synching changes)?
Also, this should probably be major.
Meanwhile, a bit more for the tests. The interdiff here resolves a test failure for #1806334: Replace the node listing at /node with a view.
Comment #49
xjmThe
@todo
in the interdiff above is kinda-related to #1760358: Provide a way to extract the ID from a config object name (see also #1831774: Config import assumes that 'config_prefix' contains one dot only).Comment #50
gddI like the idea of proceeding on the install/uninstall part first, and worrying about enable/disable in a followup while we wait to see what happens with the greater architectural issue around disabled modules.
This patch merges sun's patch in #28 (which changes installation as discussed in #33 on) and xjm's tests from #48 (which all pass with the new patch.)
It appears we don't need any changes to the existing uninstall functionality, as it already functions per #33 (when an extension is uninstalled, all config matching its namespace is also uninstalled) although I wouldn't mind some more discussion around the topic introduced in #36. Namely:
1) You have your front page set to be
views.view.frontpage.yml
which is provided by Node.2) You uninstall Node, which leaves the view in your active config.
3) Your front page is now broken because without Node, that view won't work.
Although, even if we removed the config, we would still have the problem of your homepage pointing to a view that isn't there (it would just 404 instead of looking broken.) As XJM points out this problem exists already in Views handlers as well. So maybe that's just something we live with.
We will still need tests for the gamut of possibilities around providing config for another module. That'll be fun! I might start working on that today.
Comment #52
xjmI split #1850158: Bugged assumption in ModuleTestBase::assertModuleConfig() off into its own issue since it passes already and blocks #1806334: Replace the node listing at /node with a view more immediately than the rest of this issue.
Comment #53
xjmHere's the rest of my tests. I left one @todo because I'm not sure I agree with @sun's statement from #35:
The original scope of this issue was to address it so that the opposite was true.
The test here also needs the fix from #1806334-70: Replace the node listing at /node with a view so I've filed that as its own issue as well: #1850352: config_import_invoke_owner() should check whether a module exists before invoking its hooks
I haven't yet tried to debug why @sun's patch from #28 (the fix used here) breaks every Views test...
Comment #55
sunFor the problem space of disabled modules (#35 and following), I've created:
#1883658: Remove the concept of disabled modules
Comment #56
xjm#53: config-1776830-53-tests.patch queued for re-testing.
Comment #57
xjm#53: config-1776830-53-combined.patch queued for re-testing.
Comment #59
gddThis has been coming up more and more in a wide range of issues and I think it deserves to be critical given its importance in the grand scheme of things.
Note that the question of enable/disable of modules has moved to #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed.
Comment #60
xjmSee: #1932600: Configuration names do not always begin with the extension that actually owns the configuration
Comment #61
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
Comment #62
gddNote that as of #1199946-148: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed Dries has stated he is OK with removing the disabled module state. This work is progressing at #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair. Therefore I think we can go back to the solution put forth in #34, after which the discussion centers around what to do about disable/enable and isn't really applicable anymore.
Comment #63
andypostthe disabled used in #111715-179: Convert node/content types into configuration so better land this after node-config
Comment #63.0
andypostUpdated issue summary.
Comment #63.1
mtiftUpdated issue summary.
Comment #63.2
mtiftUpdated issue summary.
Comment #63.3
mtiftUpdated issue summary.
Comment #63.4
mtiftUpdated issue summary.
Comment #64
mtiftUpdated issue summary
Comment #64.0
mtiftUpdated issue summary.
Comment #64.1
mtiftUpdated issue summary.
Comment #64.2
mtiftUpdated issue summary.
Comment #64.3
mtiftUpdated issue summary.
Comment #65
xjmSo #1932600: Configuration names do not always begin with the extension that actually owns the configuration would also provide a solution for this problem.
Comment #66
xjmWe probably need further discussion about the different scenarios that cause this problem. @alexpott expressed that he does not agree with the earlier consensus in #34. Let's try to list out specific scenarios for views, field, etc. So far, we've used the frontpage view as the example, and we're not in agreement about what the expectation for the removal of the data on uninstallation.
Comment #67
mtifttagging
Comment #68
mtifttagging
Comment #68.0
mtiftmtift updated issue summary
Comment #69
yched CreditAttribution: yched commented@xjm asked me to chime in here from the 'field' perspective of things - but I'm not sure I get what the consensus is exactly (seems to be #31 / #33 rather than #34 ?), and which parts of it @alexpott challenges :-)
- The OP quotes approaches from #31 and #34, which are contradictory.
- #34 is formulated mostly in terms of "enabled / disabled", which makes it seem stale now that 'disabled modules' are not supposed to be a worry.
The latest patches seem to go with #31 anyway (sun's proposal - "install only the parts of [module]/config/[owner].*.yml where [owner] is an enabled module, and rescan all modules for new 'default config' items to import each time a module is installed") - so maybe the second part of the "Proposed resolution" would be removed - or at least rephrased without 'disabled / enabled' ?
FWIW, I'm in favor of #31 as well. No inert config items in the active config folder.
Then it seems the issue that's still debated is "cleanup at uninstall time". So I'll focus on that aspect.
Comment #70
yched CreditAttribution: yched commentedSo, yeah: What causes views.view.frontpage.yml, shipped in Node's default config, to be removed ?
a) When either one of Node or Views is uninstalled
b) Only when Views is uninstalled, it stays if Node is uninstalled
Sun was for b), there were contradictory positions around #30 / #40, then in #50 @heyrocker says he could go with b), and in #53 @xjm says she's not so sure.
- It's very possible that leaving the config items provided by a module after the module itself has gone produces stuff that's just broken. views.view.frontpage.yml cannot work without Node.
The module built the config items shipped in its default config with the assumption that its own code is running (entity types, plugins, hooks, routes...)
- But: leaving it breaks the site, removing it breaks the site too...
It's hard to come up with a single generic rule for what's best across all ConfigEntity types :-/ (this "different provider & owner" case we're talking about only involves ConfigEntities, right ?)
- "views.view.frontpage.yml cannot work without Node" is true, unless the site admin has totally changed it meanwhile, possibly even deleted it & created a new one with the same name months later. Deleting it seems pretty violent then. At the very least we'd need to delete based on UUIDs rather than just file names ?
More specifically on the field side:
- deletion is quite serious - that's db tables with user data we're talking about, cannot be undone by simply creating the same config item again
- if a module provides a field, deleting it when uninstalling the module will chain to deleting all the field instances as well (and the user data they hold), including the instances that the module did not provide itself but that were created by the admin through the "Reuse existing field" UI. All instances of the field across all entity types & bundles, no matter who defined them...
So I'm not really in favor of auto-deletion, at least when it comes to fields...
Proposals:
1) Play safe, go with b) above: no auto cleanup, only delete config items when their *owner* (views, field...) is uninstalled.
This is then pretty similar to D7's "modules can create stuff in their hook_install(), they can delete it *if they want* in their hook_uninstall(), there is no auto cleanup".
"default config import" is then just a shorthand for "doing stuff in hook_install()", without an uninstall shorthand counterpart. If you *want* to delete the stuff you added on install, use hook_uninstall().
2) Let each ConfigEntity type specify which behavior they want.
E.g when a module is uninstalled, its views should be removed, but its fields should stay
3) Same as 1), plus provide a UI screen on module uninstall saying "do you want to also delete these config items ?" with checkboxes (opt-in or opt out, TBD.)
Personal take:
2) feels like it would add confusion and inconsistency / unpredictability
3) would be sweet but can come as icing on the cake, most probably explored in contrib
So I'd go with 1)...
Comment #71
tstoeckler@yched: I think what you are discussing in #70 is certainly related to this issue but also touches on a whole different problem-space: Dependencies of configuration entities.
I.e. as soon as a view references a field/row/display/... plugin it depends on the module that ships it. If it lists entity types it depends on the module that provides it. Etc., etc. This is so far completely unsolved in Drupal 8 but is something we'll run into / are running into (not only) in this issue.
Rules in D7 contrib solved this IMO elegantly by providing a top-level 'dependencies' key, similar to module themselves, which lists modules that are needed for the rule to work. That might be something we should explore for core as well. That would be in a separate issue, though, I imagine.
I think the core issue here is that it's hard to distinguish which module actually owns a configuration entity. The resulting problems are similar, but I think it's still a sort-of different discussion.
Comment #72
tstoecklerAlthough, thinking about it explicitly declaring dependencies in config entities would let us fix this problem as well, by reversing it. We could disallow uninstalling module completely if they are required by any configuration entities. That would be similar to what we currently do with field-type-providing modules, except generalized in the Config system. You would then have to completely override or consciously delete the frontpage view in order to uninstall Node module.
Comment #73
alexpottHere's a stab at providing the required functionality.
I've implemented auto-removal contrary to recommendations of #70 since I can't think of a case were uninstalling a module that provided default config for a field it wouldn't want to remove this at this time. An example in core is Forum - see
forum_enable()
andforum_uninstall()
Some tests from the configuration suite have been commented out since they are testing for the behaviour prior to this patch.
This patch is a stepping stone. I think we might need to get #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed as this will make it easier to discern what to in
config_uninstall_default_config()
Comment #75
alexpottOkay a couple of cans of worms.
We have specific code and tests around the uninstalling of the Forum module and the persistence of the node type it provides. We need to consider why this is and if this is another argument in favour of yched's recommended approach in #70. OTOH we could say this is MAD if a module provides something and it's uninstalled then what it provides should be removed too. This is just another example of where we're trying to be far too bloody clever (like disabled modules) and getting ourselves in a twist.
Also the failure in the InstallerTest is very very interesting.
The entity module provides entity type
EntityFormMode
The use module provides the default configuration
entity.form_mode.user_register
In HEAD: when when
entity.form_mode.user_register
is installed it done a simple config not an entity.With the new patch: it only get imported when entity is enabled because it can only exist when both the providing module (user) and the config entity defining module (entity) exist
What's interesting is that this then causes it to be imported as a proper config entity which fires a EFQ query which depends on Field because it uses the FIELD_LOAD_CURRENT constant in QueryBase and field is not enabled yet! Hence the exception.
New patch attached which fixes a few things.
Comment #76
alexpottComment #78
yched CreditAttribution: yched commented- As @Berdir pointed, FIELD_LOAD_CURRENT should move to Entity / StorageControllerBase or something. after #1497374: Switch from Field-based storage to Entity-based storage (preferably in a followup :-)
- fixing import so that entity.form_mode.user_register is imported as a proper config entity rather than as a plain settings file sounds like an important win ?
- deleting a node type if it was provided by a module default config: yes, that's another example of massively destructive magic...
Comment #79
andypostAnd then we can to clean-up forum's terms #2032699: Preserve taxonomy_forums field when uninstalling forum module
Comment #80
andypostComment #80.0
andypostbroken link
Comment #81
tim.plunkettI opened #2079121: When a module providing a base table is uninstalled, delete all views built on it as an example of how Views could help clean up after modules providing views.
Comment #82
BerdirAnd the field constant change has a patch now in #2079011: Move field.module constants related to storage to EntityStorageControllerInterface
Comment #83
Anonymous (not verified) CreditAttribution: Anonymous commentedi don't think we want to uninstall config entities for other modules APIs.
however, we do want to allow config thingies to get a chance to be deleted via their storage controller API.
attached patch does that, and nothing else. happy to move this elsewhere if we decide we still want to do more.
Comment #85
catchSo the problem with leaving the front page view in when node module is disabled, is the same problem with disabling and re-enabling modules I think. Except it's worse, because we run updates for disabled modules currently, don't do that for uninstalled ones.
Here's what I think will happen, correct me if I'm wrong:
1. Node module is uninstalled, views is left installed.
2. Front page view (still using node entity or other views plugins provided by node) stays in active config.
3. Meanwhile, node module code is updated to rename a plugin, includes an update handler to rename the plugin in views configuration files. Because node module is currently uninstalled, this update never runs.
4. Node module is re-installed, now the view is missing plugins (assuming we don't overwrite it on re-install? But then what would be the point of keeping it...).
So of the options, I prefer this one:
I don't see a way around week/month/year-long race conditions unless we either prevent modules from being uninstalled when configuration depends on them, or uninstall all that configuration. Both of those options are valid. People don't like making modules 'required' dynamically though, so here's an alternative suggestion:
- When uninstall is submitted, validate whether any config entities have a dependency on the module.
- If they do, show a confirmation screen.
- That confirmation screen tells you what's going on, if you want to delete those config entities anyway, you have to explicitly select a checkbox. Otherwise it prevents the uninstall of those modules. For drush and similar it could add an option or its own confirmation.
Comment #86
mtiftAs much as it would be nice to keep things simple -- and delete all related config upon uninstalling a module -- I agree that the confirmation screen would be a good way forward. That seems better than risk having too many modules leave around unused config.
However, wouldn't that also mean we would need a screen like that when modules are enabled, asking whether or not the active config should be overwritten with the default config?
Comment #87
catch@mtift, if we went with #85 then you'd either be able to delete everything, or leave the module installed - but you wouldn't be able to both uninstall the module and leave orphaned config.
However yes this does leave the case where you modify some configuration so it has no dependencies on the original module, but it still has the same config name / uuid, so if that module's re-installed...
Comment #88
mtiftFollow-up: #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall
Comment #88.0
mtiftUpdated summary
Comment #89
mtiftConverting this issue to a "meta" issue. Currently it is blocked on #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed, but by splitting it up, we can begin work on #2082117: Install default config only when the owner and provider modules are both enabled.
Comment #90
David_Rothstein CreditAttribution: David_Rothstein commentedIs this issue blocked on disabled modules except in the narrow way required by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall (ability for a module's API to be queried by other modules while it's being uninstalled)?
There is some discussion above that it might be somehow blocked for other reasons, but I don't see what the issue is; as others pointed out above, it is certainly possible to run the same code on enable as install whenever necessary. #40 and #46 imply that disabled modules would need to have config() return FALSE when attempting to load their configuration, but I don't understand why... If a module is disabled, its configuration still exists, so why we would want to make the API pretend that it doesn't?
Comment #91
alexpottIn answer to #90 for config entities we need the module that provides the config entity to be enabled during uninstall so we can call the delete method on the config entity object rather than just deleting the file.
Comment #92
Berdir_block_rehash:
This makes up 7.8s of a drush_flush_all_caches() (32s in total).
If we can get rid of this, this would make cache flushes a lot faster :)
Comment #93
Anonymous (not verified) CreditAttribution: Anonymous commentedbump.
Comment #93.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated issue summary.
Comment #94
alexpottMinor update to issue summary as a result of Prague CMI discussions.
Comment #94.0
alexpottUpdated issue summary.
Comment #94.1
alexpottUpdated issue summary.
Comment #94.2
alexpottUpdated issue summary.
Comment #95
catchComment #96
xjmThe outstanding parts of issue are being subsumed into a few others; @alexpott is going to update them with the usecases and information from this issue.
Comment #97
xjmComment #98
jessebeach CreditAttribution: jessebeach commentedComment #99
catchI added #2090115: Don't install a module when its default configuration has unmet dependencies to the issue summary since it's related.
Comment #100
Anonymous (not verified) CreditAttribution: Anonymous commenteddiscussed this with alexpott and xjm at DrupalDevDays Szeged.
we decided on three rules when you uninstall a module:
- the module's static config goes away
- any config entity, regardless of how it was created, goes away if it's dependencies are not met
- all other config entities remain
to make this true, we need to change the views calculate dependency code - #2224521: don't put the module key into the dependency list when installing a view.
Comment #101
xjmSo, the work being done on config dependencies should cover most of the usecases described here already. I think part of what we need to do here is ensure there's test coverage for each of the cases described in this issue, codifying that what we expect to happen, does. Then we can finally wrap up this issue. :)
Comment #102
alexpott#2082117: Install default config only when the owner and provider modules are both enabled deals with install. This ensures the views.view.frontpage is not created if only node is installed. ON views install if node is installed then it will be created.
#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall deals with uninstall. Since views.view.frontpage uses node as its base table table when node is uninstalled the views.view.frontpage is removed. If node is then re-installed the default view will be recreated.
Both issues introduce test coverage of the code but there is no explicit test around views.view.frontpage which might be nice to add.
Issues to do with disabled modules reported no longer exists because modules can not be disabled #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed.
Comment #103
alexpottOh I forgot I added
Drupal\config\Tests\ConfigOtherModuleTest::testUninstall()
- we have test coverage!Comment #104
xtfer CreditAttribution: xtfer commentedPossibly related: #2227783: Reinstalling a module already providing config entities causes a Fatal