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

#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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Priority: Normal » Major
FileSize
1.18 KB

This just adds back the removed code.

Now it will try to remove config it provided, or any belonging to it. Both are necessary.

tim.plunkett’s picture

Status: Active » Needs review

This needs tests, but let's see if it breaks anything as is.

xjm’s picture

Assigned: Unassigned » xjm

Working on clarifying this a bit. This is kind of a neat bug.

xjm’s picture

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

xjm’s picture

sun’s picture

Priority: Major » Normal

oh 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.)

tim.plunkett’s picture

sun’s picture

I 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

module: mymodule

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

gdd’s picture

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

tim.plunkett’s picture

It would also mean we have to open all the views to check that on uninstallation.

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

Anonymous’s picture

i'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.

xjm’s picture

Assigned: xjm » Unassigned

#9 sounds like what I'd expect to happen. This however sounds bad:

It would also mean we have to open all the views to check that on uninstallation

Plus what @beejeebus points out in #11. Does the providing module simply need to be part of the config name?

xjm’s picture

Assigned: Unassigned » xjm

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

  • Scan every module's config dir to look for views.view.*.yml files, and not delete those if they're found in one of those other modules. Con: Lots of filesystem action.
  • Store the owner module in each view, and open each view on uninstallation to see what the owner is. Con: again, lots of filesystem action.
  • Change the config prefix to be (e.g.) views.view.node or views.view.views instead of views.view.
  • List it in the manifest or something. Con: Tacking more and more stuff onto the manifest and making it a file-of-doom.

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.

xjm’s picture

Attached 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 with DrupalUnitTestBase (even after adding $install = TRUE to enableModules()). 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.

xjm’s picture

FileSize
7.77 KB

Helps to attach the patch.

sun’s picture

Issue tags: +Configurables

Two things I strongly object to:

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

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

tim.plunkett’s picture

So, 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?

Anonymous’s picture

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

xjm’s picture

Status: Needs review » Needs work
xjm’s picture

@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.)

gdd’s picture

Status: Needs work » Needs review

The 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 as module: 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.

gdd’s picture

Status: Needs review » Needs work
xjm’s picture

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.

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

sun’s picture

re #17:

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?

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:

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?

Views has of course no business in doing that. Either the configuration system, or module_enable() has to perform that task.

re #21.1:

using the manifests to register who is responsible for the config

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:

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.

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?

gdd’s picture

Views has of course no business in doing that. Either the configuration system, or module_enable() has to perform that task.

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

The moment you install config for an extension that does not exist, what happens if you install the extension? Your configuration gets overwritten.

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.

Can you create a comment entity record if Comment module is not installed?

No I never said you could?

What happens if you update the extension? Who will maintain the configuration that no one owns?

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.

This is guaranteed to break so horribly bad, you will not want to use Drupal anymore.

Can we pull back on the rhetoric please? It doesn't help anything.

sun’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
3.62 KB
  1. Fixed default config provided for another module is not installed.
  2. Fixed other extensions may not provide default config for non-config-entities and cannot override the owner's default config.

Uninstallation of manifest files is fixed in #1831776: removing manifest files from uninstalled modules already.

sun’s picture

FileSize
780 bytes
3.6 KB

Removed duplicate code.

sun’s picture

FileSize
1.64 KB
3.44 KB

Sorry, that was ill-minded.

Fixed default config merge code path is impossible to reach.

Status: Needs review » Needs work

The last submitted patch, config.default.28.patch, failed testing.

gdd’s picture

Status: Needs work » Active

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

sun’s picture

I thought I had sufficiently clarified already.

  1. When an extension is installed, we need to allow all other enabled extensions to provide additional default configuration.
  2. The owner of configuration is always determined by the first part of the config object name, which is the only enforced semantic for config object names and that should stay that way.
  3. We must not write and store configuration for an extension that does not exist.
  4. The performance concerns raised against scanning all config directories of all extensions for as to whether they provide default [entity] config for an extension that is being installed are moot, since installing an extension is a rare operation in the first place.
  5. If module Foo provides additional default config for module Bar, but module Bar is not installed, then Foo module's default config for Bar module is not installed when Foo module is installed. The default config for module Bar is only installed when Bar module is installed.
  6. If an extension is uninstalled, all configuration that belongs to it ceases to exist.
  7. If an extension is uninstalled and re-installed, its default configuration is re-polluted from the currently enabled modules.
  8. Result: If I do not install Views module, then the foreseeable tonne of default views configurations has no business at all in my active configuration. Configuration for views only exists if Views is installed.

That's a simple, natural behavior. Please explain where you see an architectural flaw in this.

gdd’s picture

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

xjm’s picture

#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 over views.view.frontpage.yml when installing node if views is not installed.

So, a few scenarios.

  • Node is installed first:
    1. Node is installed. Views is not installed. The config system installs node's config directory's contents if and only if the API-providing module is enabled, so views.view.default.yml is not copied.
    2. Views is then installed. The config system first installs views' config directory contents, then checks all enabled modules' config directories on behalf of Views, looking for views.whatever and installing them. views.view.frontpage.yml is now installed.
  • Views is installed first:
    1. Views is installed. Node is not installed. The config system first installs views' config directory contents, then checks all enabled modules' config directories on behalf of Views. Only Views' views are installed.
    2. Node is then installed. views.view.frontpage.yml is now installed, because views is enabled.
  • If either Node OR Views is uninstalled, views.view.frontpage.yml is deleted.
  • If either Node OR Views is disabled but not uninstalled, nothing happens to views.view.frontpage.yml, because only uninstall is destructive.
  • What about when is installed but disabled when the other is installed? Does there also need to be an operation on enable then?
gdd’s picture

What about when one are both are installed but disabled when the other is installed? Does there also need to be an operation on enable then?

I think the operation should still take place if one or both is installed but disabled. In other words in the following:

  • Views is installed and disabled, Node is not installed.
  • When installed, Node copies views.view.frontpage.yml to active config.
  • When Views is enabled nothing happens (the config is already there.)
  • When Node is disabled nothing happens (the config stays there.)
  • When Node is re-enabled nothing happens (the config is already there.)
  • If either is uninstalled, views.view.frontpage.yml still gets deleted.

This is essentially as changing 'enabled' to 'installed' in the following statement:

The config system installs node's config directory's contents if and only if the API-providing module is enabled

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.

sun’s picture

we only have code dealing with default config at the install/uninstall level, while enable/disable remain blissfully ignorant.

That'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. If either Node OR Views is uninstalled, views.view.frontpage.yml is deleted.
  2. If either Node OR Views is disabled but not uninstalled, nothing happens to views.view.frontpage.yml, because only uninstall is destructive.
  3. What about when is installed but disabled when the other is installed? Does there also need to be an operation on enable then?

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. ;)

xjm’s picture

Assigned: Unassigned » xjm

Nope, views.view.frontpage would only be deleted if Views module is uninstalled. Not if Node is uninstalled.

Ah, 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.

gdd’s picture

1) Nope, views.view.frontpage would only be deleted if Views module is uninstalled. Not if Node is uninstalled.

I'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.

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.

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.

sun’s picture

Meanwhile, 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.

gdd’s picture

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

sun’s picture

IMHO, 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.

xjm’s picture

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

gdd’s picture

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

Anonymous’s picture

not sure where that ended up either, but +1 to killing disabled.

xjm’s picture

gdd’s picture

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

sun’s picture

FileSize
596 bytes

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

xjm’s picture

Eesh.

xjm’s picture

Title: Configuration provided by a module that belongs to another module's API is not cleaned up on uninstall » Installation and uninstallation of configuration provided by a module that belongs to another module's API
Priority: Normal » Major
Status: Active » Needs review
FileSize
941 bytes
8.68 KB

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

xjm’s picture

gdd’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1776830-install-default-config-50.patch, failed testing.

xjm’s picture

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

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
14.34 KB
18.04 KB
14.6 KB

Here's the rest of my tests. I left one @todo because I'm not sure I agree with @sun's statement from #35:

1) Nope, views.view.frontpage would only be deleted if Views module is uninstalled. Not if Node is uninstalled.

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

Status: Needs review » Needs work

The last submitted patch, config-1776830-53-combined.patch, failed testing.

sun’s picture

For the problem space of disabled modules (#35 and following), I've created:
#1883658: Remove the concept of disabled modules

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Configuration system, -VDC, -Configurables

#53: config-1776830-53-tests.patch queued for re-testing.

xjm’s picture

#53: config-1776830-53-combined.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +Configuration system, +VDC, +Configurables

The last submitted patch, config-1776830-53-combined.patch, failed testing.

gdd’s picture

Priority: Major » Critical

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

xjm’s picture

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

gdd’s picture

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

andypost’s picture

the disabled used in #111715-179: Convert node/content types into configuration so better land this after node-config

andypost’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Updated issue summary

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm’s picture

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

mtift’s picture

Issue tags: +sprint

tagging

mtift’s picture

tagging

mtift’s picture

Issue summary: View changes

mtift updated issue summary

yched’s picture

We 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

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

yched’s picture

So, 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)...

tstoeckler’s picture

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

tstoeckler’s picture

Although, 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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
18.08 KB

Here'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() and forum_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()

Status: Needs review » Needs work

The last submitted patch, 1776830-73.patch, failed testing.

alexpott’s picture

FileSize
20.31 KB
7.18 KB

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

alexpott’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1776830-75.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

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

andypost’s picture

andypost’s picture

andypost’s picture

Issue summary: View changes

broken link

tim.plunkett’s picture

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

Berdir’s picture

Anonymous’s picture

FileSize
893 bytes

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

Status: Needs review » Needs work

The last submitted patch, 1776830.patch, failed testing.

catch’s picture

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

Although, 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.

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.

mtift’s picture

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

catch’s picture

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

mtift’s picture

mtift’s picture

Issue summary: View changes

Updated summary

mtift’s picture

Title: Installation and uninstallation of configuration provided by a module that belongs to another module's API » [META] Installation and uninstallation of configuration provided by a module that belongs to another module's API
David_Rothstein’s picture

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

alexpott’s picture

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

Berdir’s picture

Issue tags: +Performance

_block_rehash:

    // Remove any invalid block from the list.
    // @todo Remove this check as part of this issue.
    if (!$block->getPlugin()) {
      unset($blocks[$block_id]);
      continue;
    }

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

Anonymous’s picture

bump.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Minor update to issue summary as a result of Prague CMI discussions.

alexpott’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Issue tags: +beta blocker
xjm’s picture

Issue tags: -sprint

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

xjm’s picture

Issue tags: +Naming things is hard
jessebeach’s picture

Title: [META] Installation and uninstallation of configuration provided by a module that belongs to another module's API » [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API
catch’s picture

Issue summary: View changes
Anonymous’s picture

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

xjm’s picture

Status: Needs work » Active

So, 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. :)

alexpott’s picture

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

alexpott’s picture

Status: Active » Closed (duplicate)

Oh I forgot I added Drupal\config\Tests\ConfigOtherModuleTest::testUninstall() - we have test coverage!

xtfer’s picture