Updated: Comment #321

Suggested commit message: "Issue #2003966 by fubhy, swentel, alexpott, ParisLiakos, bojanz, aspilicious, beejeebus, steinmb: Fixed Disabled modules are broken beyond repair so the 'disable' functionality needs to be removed."

Background

Problem

Details

Drupal has two kinds of dependencies between modules:

1. Explicit, hard dependencies, as defined in dependencies[] - module A depends on module C.
2. Soft, conditional dependencies, as a result of references to things provided by module A in configuration owned by module B (possibly provided as a default by module C).

The second kind of dependency is the one which has caused most of the bugs with disabled modules.

For example:

Module A provides a field type plugin.

Module B (field module) references this field type plugin in a field/field instance configuration entity.

Module C provides an entity type and bundle.

Module B references the entity type and bundle in the same field plugin.

Module B does not have an explicit dependency on module A or C, but the configuration that it owns does.

Some modules like Views are able to handle these cases from the perspective of 'broken handlers'.

However modules dealing with user content (i.e. entity/field but also hard-coded references such as comment_node_statistics) are unable to guarantee data integrity. The modules that are still enabled may need to act on either their configuration entities or user data that is based on the configuration entity, but are unable to because the plugin/entity type is unavailable. When the plugin/entity type becomes available again, things may have moved on.

This can result in anything from php notices due to 'missing' data, to fatal errors where things have moved on significantly i.e. a module referencing serial IDs from another module that is disabled, uninstalled then reinstalled could end up referencing different entities altogether without having any way of knowing this.

At the moment modules try to deal with this in myriad different ways, from the Views concept of 'broken handlers', to field_info_system_alter() attempting to convert soft dependencies to hard dependencies dynamically.

#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall (postponed on this issue) is necessary to resolve the situation for uninstalled modules.

David Rothstein has (rightly) pointed out that the same approach in #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall can also be applied to disabled modules too, if we additionally enforce modules are uninstalled in one step (i.e. remove the ability to go from disabled -> uninstalled). However, given a critical issue to restore the disabled modules UI in a more limited way, this ends up at the same end point, just via different routes.

There are various examples given in the issue, as well as many other bugs which are less fundamental, but still numerous, hard to solve and frequent due to the inherent instability/limbo state of disabled modules, and a lack of clarity in terms of what it means.

  • Field types disappear; all field values get stale.
  • Entity types disappear; all references are unresolvable, data is lost.
  • Plugin types disappear, and along with it, all plugin IDs; all unresolvable plugin references get orphaned.
  • ...
  • Configuration, content, and data of disabled modules cannot be staged. The system is unable to know how to deal with the data, since it is impossible to serialize/export and import it.
  • The current transitional disable » uninstall state in itself means that the API of a module is not invoked when its data is deleted, which consequently means that data integrity gets broken in enabled modules.
  • Users are currently required to use the dangerous facility of disabling modules and the system makes it look as if it was a safe and sane thing to do.
  • History

    Solution in #317 and API changes

    • Completely removes the concept of disabled modules.
    • system.module.disabled config is removed.
    • module_enable() is deprecated and now does an install.
    • module_disable() is removed as it does not make sense to change this to the destructive operation of uninstalling modules.
    • All usages of module_enable() and module_disable() are removed from core.
    • module_uninstall() is deprecated but core usages are left to be converted in a followup (this is less confusing than module_enable() and module_disable()).
    • moduleHandler::enable() and moduleHandler::disable() are removed.
    • moduleHandler::install() is added.
    • The following hooks are removed:
      • hook_modules_preenable()
      • hook_enable()
      • hook_modules_enabled()
      • hook_disable()
      • hook_modules_disabled()
      • hook_modules_preinstall()
    • The following hooks are added:
      • hook_module_preuninstall()
      • hook_module_preinstall()

    The change from invoking hook_modules_preinstall() with a list of modules to invoking hook_module_preinstall() with single module is so that we can guarantee the environment that will be running when the preinstall action is fired before installing a particular module. For example if both taxonomy and forum are being installed this change means that you now know that the taxonomy will be installed when the hook fires for the forum module - which is a good thing because taxonomy is a dependency for forum.

    In order to explore the ability to disable modules the contrib module https://drupal.org/project/disable_modules has been created

    Updating contrib code

    Functions

    • module_enable(array('mymodule')) becomes Drupal::moduleHandler()->install(array('mymodule'))
    • module_disable(array('mymodule')) should be removed.
    • module_uninstall(array('mymodule')) becomes Drupal::moduleHandler()->uninstall(array('mymodule'))

    Hooks

    • hook_enable() functionality should be moved to hook_install()
    • hook_modules_enabled($modules) functionality should be moved to hook_modules_installed($modules)
    • hook_disable() functionality should be moved to hook_uninstall()
    • hook_modules_disabled($modules) functionality should be moved to hook_modules_uninstalled($modules) or hook_module_preuninstall($module) (note per module not a list)
    • hook_modules_preenable($modules) functionality should be moved to hook_modules_installed($modules) or hook_module_preinstall($module) (note per module not a list)
    • hook_modules_preinstall($modules) functionality should be moved to hook_module_preinstall($module) (note per module not a list)

    Related issues

    Follow-up issues

    Files: 
    CommentFileSizeAuthor
    #434 disabled-fix.patch1.85 KBdawehner
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch disabled-fix_0.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    #433 disabled-fix.patch947 bytesdawehner
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch disabled-fix.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    #407 1199946-407.patch199.67 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 58,266 pass(es).
    [ View ]
    #382 1199946-382.patch192.77 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
    [ View ]
    #372 1199946-372.patch192.64 KBalexpott
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1199946-372_0.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    #370 1199946-370.patch191.98 KBalexpott
    FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
    [ View ]
    #358 356-358-interdiff.txt604 bytesalexpott
    #358 1199946-358.patch193.07 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 57,571 pass(es).
    [ View ]
    #356 1199946-356.patch192.33 KBalexpott
    FAILED: [[SimpleTest]]: [MySQL] 57,686 pass(es), 1 fail(s), and 0 exception(s).
    [ View ]
    #343 333-343-interdiff.txt2.61 KBalexpott
    #343 1199946-343.patch192.31 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 57,549 pass(es).
    [ View ]
    #333 328-333-interdiff.txt5.04 KBalexpott
    #333 1199946-333.patch194.38 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 57,558 pass(es).
    [ View ]
    #332 Screen Shot 2013-09-02 at 12.38.49 PM.png49.3 KBwebchick
    #332 Screen Shot 2013-09-02 at 12.40.02 PM.png49.19 KBwebchick
    #331 Screen Shot 2013-09-02 at 12.31.52 PM.png66.85 KBwebchick
    #331 Screen Shot 2013-09-02 at 12.34.31 PM.png65.58 KBwebchick
    #328 324-328-interdiff.txt12.98 KBalexpott
    #328 324-328-non-whitespace-interdiff.txt2.71 KBalexpott
    #328 1199946-328.patch192.76 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 55,863 pass(es).
    [ View ]
    #327 1199946-do-not-test.patch3.1 KBchx
    #324 322-324-interdiff.txt12.67 KBalexpott
    #324 1199946-324.patch196.27 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 55,991 pass(es).
    [ View ]
    #322 321-322-interdiff.txt758 bytesalexpott
    #322 1199946-322.patch200.95 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 55,929 pass(es).
    [ View ]
    #321 320-321-interdiff.txt2.52 KBalexpott
    #321 1199946-321.patch200.96 KBalexpott
    FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
    [ View ]
    #320 319-320-interdiff.txt1.75 KBalexpott
    #320 1199946-320.patch198.88 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 56,239 pass(es).
    [ View ]
    #319 1199946-319.patch197.77 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 56,166 pass(es).
    [ View ]
    #317 305-317-interdiff.txt40.88 KBalexpott
    #317 1199946-317.patch198.19 KBalexpott
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1199946-317.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    #305 1199946-305.patch193.41 KBfubhy
    PASSED: [[SimpleTest]]: [MySQL] 56,554 pass(es).
    [ View ]
    #305 interdiff.txt1.72 KBfubhy
    #303 interdiff.txt11.35 KBfubhy
    #303 1199946-303.patch193.58 KBfubhy
    PASSED: [[SimpleTest]]: [MySQL] 56,089 pass(es).
    [ View ]
    #274 1199946-274.patch187.74 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 55,941 pass(es).
    [ View ]
    #271 268-271-interdiff.txt582 bytesalexpott
    #271 1199946-271.patch187.97 KBalexpott
    PASSED: [[SimpleTest]]: [MySQL] 56,060 pass(es).
    [ View ]
    #268 1199946-268.patch187.28 KBalexpott
    FAILED: [[SimpleTest]]: [MySQL] 55,727 pass(es), 1 fail(s), and 0 exception(s).
    [ View ]
    #266 1199946-266.patch176.28 KBsteinmb
    FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
    [ View ]
    #262 1199946-262.patch188.82 KBfubhy
    PASSED: [[SimpleTest]]: [MySQL] 54,979 pass(es).
    [ View ]
    #259 1199946-258.patch177.25 KBfubhy
    FAILED: [[SimpleTest]]: [MySQL] 54,855 pass(es), 108 fail(s), and 8 exception(s).
    [ View ]
    #259 interdiff.txt2.27 KBfubhy
    #257 1199946-257.patch177.34 KBfubhy
    FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
    [ View ]
    #257 interdiff.txt5.88 KBfubhy
    #251 1199946-251.patch178.67 KBfubhy
    PASSED: [[SimpleTest]]: [MySQL] 54,821 pass(es).
    [ View ]
    #242 1199946-242.patch178.52 KBfubhy
    PASSED: [[SimpleTest]]: [MySQL] 54,274 pass(es).
    [ View ]
    #240 interdiff.txt5.51 KBfubhy
    #240 1199946-240.patch178.51 KBfubhy
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1199946-240.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    #237 disabled-modules-1199946-238.patch178.54 KBfubhy
    PASSED: [[SimpleTest]]: [MySQL] 54,181 pass(es).
    [ View ]
    #84 1199946-fix-disabled-modules-alt.patch20.69 KBbojanz
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1199946-fix-disabled-modules-alt.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    #79 1199946-fix-disabled-modules.patch15.9 KBbojanz
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1199946-fix-disabled-modules.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]

    Comments

    fubhy’s picture

    Status:Needs work» Needs review

    A module that is missing hook_uninstall does not remove data as I know.

    Of course it does. Look at ModuleHandler code, look at what happens when you uninstall a module that provides a schema. That's all removed automatically. hook_uninstall() is just for additional tasks obviously. Also, in this patch we add some additional entity/field purging code.

    Allow modules that are able to assure data integrity to be disabled, please.

    If you have an idea/concept for how that could possibly work, let's hear it. We talked about this possibility before and concluded that it's simply not possible/viable.

    As said previously, there will be a contrib module which is likely to handle this by simply temporarily removing modules from the module list in ModuleHandler and thereby disabling it. Technically there is absolutely no reason why this can't be solved in contrib. However, we won't do that in core for the aforementioned reasons. The decision has been made already, let's move on.

    Contrib is no option for this issue. This is a must function for core. Every field maintainer must expect inconsistent data. That's why we have empty() and isset() and others.

    Yeah, that won't work. Do you really expect every module to fully support graceful fallbacks for every scenario that may occur? Including fixing the data again once re-enabled? How would that even be possible? It's not. We can only support features in core that we can make work flawlessly. This is not such a thing. We don't know how to make it work and I am bold enough to say it's simply impossible.

    thedavidmeister’s picture

    I wish there was a status for "Do not demand the issue be changed without providing a patch".

    Ignore that. But seriously, this discussion has almost completely devolved into nothing but re-hashing the same, tired complaints with no suggestions whatsoever as to how the complaints could be addressed or alternate approaches that are actually implementable.

    Please, please, if you're considering leaving a comment here, don't until you've read the *whole* thread above. Has somebody already suggested and discussed in depth what you're about to post?

    webchick’s picture

    Rather than telling people to read ~300 comments, how about people familiar with this issue update the issue summary so they can read only one? That would avoid re-hashing the debates, because they'd be in one place, along with their outcomes.

    I'd add the tag, but it's already there. :)

    klonos’s picture

    @falcon03, #278: + 1mil!!!

    @fubhy: All I read from your posts is this:

    ...Let's please not start this discussion again. ...So please, let's stop arguing now and instead get this patch reviewed and committed. ...The decision has been made already, let's move on.

    Give us a break!

    I am 100% sure though that there will be a contrib module that will bring this functionality ...

    How can you be *absolutely* 100% sure? Is there any module/sandbox already created for that purpose? Is even any namespace reserved in d.o for that? Any people volunteered and made a pledge that this would certainly happen for those who need it? Are you offering? If any of these is true, then yes we might as well shut up about it already. If not, can you please let people express their opinions freely and be polite about it? Thanx.

    We have concerns about this fundamental change and even if we don't get replies (or the sort of replies we'd like), we still want to express our fear of things going terribly wrong for our use cases. Please be kind enough to bear with us. Thanx again.

    Anyways, the matter still stays: the way things are about to change in this issue, it seems like contrib (where most innovation happens IMO - see CCK and Views for example) will feel like a minefield where people would be scared to touch things unless they have "proper" dev/deploy environments and backup workflows in place and even then they'd have to be extra careful!

    And before anybody rushes to claim that "this is plain FUD", let me tell you that so far I found contrib testing in *actual/production* environments (where actual data/scenarios can be tested) to be relatively safe. When a module maintainer said "give the new dev version a go and report back", I though: "Sure, why not?". Now I'd be "Yeah, right!". So, if you want all us willing testers to become risky suckers, well good luck with that and lets see how slow contrib is ported to D8 this time and in which decade d.o will be running D8. Simple as that.

    klonos’s picture

    ...and let me say that I've been following the issue since day 1. So I do know what the implications are if we keep the old way. What I meant to say in my previous comment is this:

    Sure, your decision is perfectly well justified, but have you actually considered all the implications of removing this functionality? Are you willing to risk having them?

    thedavidmeister’s picture

    where people would be scared to touch things unless they have "proper" dev/deploy environments and backup workflows in place and even then they'd have to be extra careful!

    And so you very well should be! The dangers of making untested changes to a live site cannot be overstated. Natural disasters like volcanoes and earthquakes are similar to disasters caused by untested changes to your prod environment - extremely sporadic, yet potentially extremely destructive.

    I'm not sure why you have to be 'extra careful' if you have a correct workflow in place. This patch looks like it would make a proper workflow easier to maintain, not harder, as there are less "states" your modules can be in.

    Rather than telling people to read ~300 comments, how about people familiar with this issue update the issue summary so they can read only one? That would avoid re-hashing the debates, because they'd be in one place, along with their outcomes.

    Tru dat.

    klonos’s picture

    ...one final thing (and sorry for the multiple posts):

    I understand that so far we were unable to find a good solution for the problems that the "disable" functionality causes. The thing is that D8 has improved so much and will offer developers with a pleiad of tools (3rd party libraries + frameworks + OOP) that were not available before and might help a great deal. Are you sure that these won't suffice to potentially help in figuring this out without removing the feature? Can we postpone this for D9 then?

    thedavidmeister’s picture

    #296 - unfortunately the issues have nothing to do with tools or frameworks, or that with "more technology", or more effort or more PHP objects we could some day support disabled modules...

    The issue is that what we say to disabled modules currently is: "Manage your data carefully, no matter what, but you're forbidden to run any code to do so". What we're saying in this issue is that this requirement contradicts itself, that we can't support disabled modules in as fundamental a way as we could never make "1 + 1 = 3" become true. Actually, from reading this thread, it looks like we've never supported disabled modules in core, we've just allowed them and crossed our fingers that it would be ok, leaving it to module maintainers to bear the brunt of delivering a product that meets any and all user expectations, hoping that modules would never get "so complex" that it would ever cause problems that could be traced back to core.

    We're saying here that if your module works as well today as it did yesterday when you're re-enabling after disabling it last night is as more down to good luck and circumstance than anything somebody did to ensure that such a situation makes sense in the first place. We're saying this situation is wrong, irresponsible and unprofessional and this issue is our intervention.

    swentel’s picture

    For those concerned and anxious, I created a project that will allow you to disable modules again from the UI: https://drupal.org/project/disable_modules
    I will work on it, and I'm sure I'll get others on it too. Of course there is not code yet, as we haven't got a final patch yet in for D8.

    Our now mantra then will be: download the 'Disabled modules' project and then please test by disabling the module.

    fubhy’s picture

    How can you be *absolutely* 100% sure? Is there any module/sandbox already created for that purpose? Is even any namespace reserved in d.o for that? Any people volunteered and made a pledge that this would certainly happen for those who need it? Are you offering? If any of these is true, then yes we might as well shut up about it already. If not, can you please let people express their opinions freely and be polite about it? Thanx.

    How was I not polite? I answered every single question/comment and outlined why we have to do this. However, for months now we are running in circles with people raising the same concerns and asking the same questions over and over again and us giving the same answers repeatedly. We already outlined, quite well to be honest, how and why the 'disable/enable feature' does not work. After all, many people have been trying very hard to solve this, working countless hours on possible solutions to then decide that the best way to solve it is to simply remove it. Then again, we spent hours and hours to write a patch. Now, after all the work we put into this - how frustrating do you think it is for us who thought about and worked on this issue for so long when people still come here to complain about the same things that we already answered a billion times? Things that we said a hundred times we simply can't solve properly. How polite do you think that is? Yes, it DOES suck that we can't have this feature. But removing this (broken) feature is the only responsible thing we can do here.

    And before anybody rushes to claim that "this is plain FUD", let me tell you that so far I found contrib testing in *actual/production* environments (where actual data/scenarios can be tested) to be relatively safe. When a module maintainer said "give the new dev version a go and report back", I though: "Sure, why not?". Now I'd be "Yeah, right!". So, if you want all us willing testers to become risky suckers, well good luck with that and lets see how slow contrib is ported to D8 this time and in which decade d.o will be running D8. Simple as that.

    You don't test in production. That's a problem with the workflow you described, not with the removal of this feature. You don't change screws or parts of the engine on a running car either. It's simply not possible to fiddle with the innards of a system this complex while it's running and it's data is in flux. It's simply not professional either. How are you going to explain to your client that you just lost all their data because you thought playing with the modules on the production site was okay?

    catch’s picture

    If necessary we can put a UI back into core for disabling modules - on it's own dedicated page out of the way somewhere with a big red warning. That or a contributed module is plenty IMO.

    For now this patch needs to go in as soon as it's ready, without that UI, so we can untangle years and years of mess.

    fubhy’s picture

    It's ready imho. We do have follow-ups for the UI already (fixing and simplifying the install/uninstall page and improving UX). Who's up for a review?

    catch’s picture

    Reviewed a bit, overall this looks great.

    1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      @@ -688,184 +653,147 @@ public function enable($module_list, $enable_dependencies = TRUE) {
      +          // Skip already disabled modules.

      Comment needs updating.

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      @@ -688,184 +653,147 @@ public function enable($module_list, $enable_dependencies = TRUE) {
      +   * Helper method for removing all cachebins registered by a given module.

      cache bins

    3. +++ b/core/modules/language/language.install
      @@ -21,6 +21,10 @@ function language_install() {
      +  // Update the language count, if the module was disabled before, the

      uninstalled? Or is this even needed?

    4. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.php
      @@ -61,28 +61,6 @@ function testUpdateProjects() {
      -   * Check if a list of translatable projects can include hidden projects.

      Doesn't this test need to stay in? It checks enabled hidden modules too doesn't it?

    fubhy’s picture

    StatusFileSize
    new193.58 KB
    PASSED: [[SimpleTest]]: [MySQL] 56,089 pass(es).
    [ View ]
    new11.35 KB

    Doesn't this test need to stay in? It checks enabled hidden modules too doesn't it?

    Good catch, catch ;). That can indeed stay. And it should. It also checks for themes.

    Fixed the rest as well as some other occurrences of enable/disable in the ModuleHandler (we are touch those lines anyways).

    fubhy’s picture

    +++ b/core/modules/locale/lib/Drupal/locale/Form/LocaleSettingsForm.phpundefined
    @@ -38,10 +38,10 @@ public function buildForm(array $form, array &$form_state) {
    +      '#title' => t('Check for updates of disabled themes'),

    Oh, I guess that should read 'projects' too then.

    fubhy’s picture

    StatusFileSize
    new1.72 KB
    new193.41 KB
    PASSED: [[SimpleTest]]: [MySQL] 56,554 pass(es).
    [ View ]

    Removing references to drush and fixing that one line mentioned in #304

    falcon03’s picture

    @effulgentsia, @thedavidmeister: I *don't think* that feeds is a bad example. Let's say that I disable the single feed: the module code (which is about 760 KB compressed) is still running. Now, maybe not all the code is loaded during each page loading, but some is loaded anytime any users visits my website for sure. Exporting the feed, uninstalling the module, reinstalling the module (BTW, does "re-installing" mean also re-uploading the module source to the server?) and re-importing the feed is doable, but requires more time than simply disabling the module when it is not needed, then re-enable it when it is needed.

    Enabling feeds without some of the used plugins can give really bad results. But, honestly, this is my own responsibility. If I'm doing something wrong (not enabling some plugins if they are needed) I know that I could experience some issues. But I want to decide wether to do or not it.

    Regarding the weird notices example: they don't always depend on us; what about the hosting provider changing suddenly the PHP version? Not anyone can manage his/her server configuration: every site I build is on a shared hosting.

    In addition to this, I want to say that if a lot of us are coming and asking for not removing this functionality, maybe the community will not be happy if it gets removed. This means that we know the consequences of keeping this functionality in core and we're ready to deal with them.

    mikeryan’s picture

    I have watched this issue with some trepidation, because Migrate does have a seemingly valid use case for temporary disablement of modules - we often do not want modules to operate on migrated content the way they do on manually-created content. It may be for performance; it may be that whatever it's doing is being explicitly migrated (think user points/badges) and thus redundant; it may be inappropriate and even dangerous at migration time (think comment_notify). In cases where we've done this the disablement was temporary and didn't present high risk of the sorts of corruption problems we're concerned with here, and indeed may prevent messing up data with duplicate actions, so we might have made an argument that this was a valid use case to maintain the module disable functionality.

    However, disabling an entire module was always too big a hammer for our particular nail. If the module provides elements of the site structure (menus, fields, entities) it could render the site UI unusable (or at least ugly). More and more often such modules may be required by an installation profile and cannot be disabled without disabling the whole profile and breaking the site entirely. And... well, there's plenty of good arguments against disabling modules already in this thread.

    I happen to be in the middle of a large-scale migration project into a Commons environment with lots of contrib modules I want to shut up during migration, and I've come up with an approach to selectively disable hooks in D7 via hook_module_implements_alter(): #2065295: Disabling hook invocations for specific modules. It's rather hacky (because I want to disable them just within the current migration process, not site-wide) but so far seems to be working nicely. I'm saying good-bye to even thinking about disabling modules in D7, and fully support the effort here. What I want from D8 (I haven't dug in deeply enough to see if it's already there) is to be sure there are documented and supported APIs to disable hooks and services either locally (within a single request/process) or globally. If that's there, I'm happy - and I expect we will see at least one contrib module providing a UI to this functionality. Hell, someone could do a UI for disabling hooks globally in D7 now using hook_module_implements_alter()...

    thedavidmeister’s picture

    @effulgentsia, @thedavidmeister: I *don't think* that feeds is a bad example. Let's say that I disable the single feed: the module code (which is about 760 KB compressed) is still running. Now, maybe not all the code is loaded during each page loading, but some is loaded anytime any users visits my website for sure. Exporting the feed, uninstalling the module, reinstalling the module (BTW, does "re-installing" mean also re-uploading the module source to the server?) and re-importing the feed is doable, but requires more time than simply disabling the module when it is not needed, then re-enable it when it is needed.

    What are you even talking about?

    1. Enable Feeds
    2. Build a Feed
    3. Bounce Feed to Features
    4. Commit Feature to Git
    5. Uninstall Feeds
    6. Re-install Feeds
    7. Clear cache
    8. Your Feeds that were previously bounced to Features still exist, any configuration that wasn't deployed correctly is gone. Clean. Simple :)

    By the time we get to the end, nothing has changed. There's no extra re-import step, there's no "re-upload". Uninstalling a module doesn't touch the code on the server and never has, it just "flushes" the database for data related to the module being uninstalled.

    David_Rothstein’s picture

    I'm going to complete the proposed review/analysis/summary (see #223, #228, #229) over the next several days.

    I dropped the ball on this unfortunately, due to life and an incredibly busy time at work intervening. I'm hoping by next week, but now know better than to promise :(

    If necessary we can put a UI back into core for disabling modules - on it's own dedicated page out of the way somewhere with a big red warning. That or a contributed module is plenty IMO.

    Modifying the UI for disabling modules (rather than removing it) is a compromise that could make a lot of people in this issue happy. But codewise it would be extremely different from the code in the current patch.

    And while it's theoretically possible to imagine this as a contrib module, given that the current patch removes the feature from core not just in the UI but also at the API level, that means the contrib module would be responsible for maintaining hook_enable(), hook_disable(), etc... It would be difficult to do that effectively.

    You don't test in production. That's a problem with the workflow you described, not with the removal of this feature. You don't change screws or parts of the engine on a running car either. It's simply not possible to fiddle with the innards of a system this complex while it's running and it's data is in flux. It's simply not professional either. How are you going to explain to your client that you just lost all their data because you thought playing with the modules on the production site was okay?

    Many sites aren't built by "professionals" for "clients", but I'm afraid the owners of those sites aren't represented very well in the discussion in this issue. Per the statistics I cited in #147, only around 15-20% of Drupal 7 sites use the Features module, which suggests to me that people who make lots of changes directly on their production site are in fact in the majority.

    thedavidmeister’s picture

    Many sites aren't built by "professionals" for "clients", but I'm afraid the owners of those sites aren't represented very well in the discussion in this issue. Per the statistics I cited in #147, only around 15-20% of Drupal 7 sites use the Features module, which suggests to me that people who make lots of changes directly on their production site are in fact in the majority.

    Well that's not an amazing reason to not try to be as professional as possible in the patches we commit. The philosophy behind removing "disabled" is saving users from themselves - the more "newbie" the user, the more they need protection from the more experienced users! I'd argue that the fact people are experimenting on their production environments is a point *for* removing the ability for people to cause all their production to go stale because they're "just fiddling around" with things. Newbies can understand concepts like "safe" and "not safe" operations but they don't understand "implications of stale data" or "your module's hooks won't be fired" so turning the enabled/disabled/uninstalled into a binary system helps make things easier to understand.

    We're trading in "easy to figure out how to make a bandaid solution to a problem" for "easy to understand all the *actual* ramifications of a decision and avoid hidden time bombs in the system".

    When I went to Drupalcon in Sydney, there were keynotes and speeches about how Drupal 8 was going after the "enterprise" market, this issue should fit squarely into that discussion as *not* doing this should be formally recognised as an "enterprise blocker". Regardless, the barriers to version controlling configuration, even for hobbyists, should be hitting an all time low in Drupal 8 with non-database configuration management a part of core.

    It's never been easer to set up a consumer/hobbyist/personal grade deployment workflow for Drupal that automatically enforces the principles we're discussing, see companies like Pantheon that offer a CI workflow for under $1 per day.

    If you need to use FTP, services like Beanstalk can deploy git to FTP...

    Excellent, free UI tools for git like sourcetree exist that provide an incredibly shallow learning curve for git.

    There's a lot of things we can teach and show beginners how to implement a safe workflow, within their means to achieve right now, let alone in a few years when D8 is finally in wide circulation. If the stats are currently low, let's try to boost them in the next release by streamlining, encouraging and simplifying the workflows we *want* and make decisions based on that.

    hass’s picture

    The solution to prevent removal of variables automatically is to provide no schema file? Great as I have none created for google analytics yet. And if it's made a requirement i provide an empty one?

    swentel’s picture

    hass - if you have config, config will be deleted on uninstall. That's just how CMI works. In fact, there's little chance that you will have an uninstall hook as everything is done for you.

    If you would still use a table for your configuration, your config is not deployable in D8, so you will simply get a bug report. Again, that's CMI, nothing todo with this discussion at all.

    fubhy’s picture

    The solution to prevent removal of variables automatically is to provide no schema file? Great as I have none created for google analytics yet. And if it's made a requirement i provide an empty one?

    No. Default configuration (I assume you mean configuration when you say "variables") is automatically purged when a module is uninstalled. No matter what you do. Same with the schema and entities (and their field instances [and subsequently the entire field if no instances are left])...

    @see ModuleHandler::uninstall()

    EDIT: Darn, @swentel beat me to it :)

    catch’s picture

    Not using Features !== making changes directly on production. I work on sites that export to ctools and use hook_update_N() for the rest, because very little development is done in the UI at all, Features makes that workflow harder rather than easier at a certain point.

    @Falcon03, disabling a module is not going to make a measurable difference to performance on your site (unless that module is doing something ridiculous in hook_init() or hook_boot(), but that should be a bug report against the module). Having 30 vs. 300 modules installed does but shaving 2-3 modules off either number you won't notice.

    @David the code looking very different is the point of this issue if a new UI is provided. Neither core nor a contrib module is going to provide the implicit features that disabled modules currently do (like data integrity if you re-enable after six months) - it'd just be for debugging.

    xjm’s picture

    xjm’s picture

    Trying the tagging again.

    Let's please update the summary here with the current approach and with a reference to https://drupal.org/project/disable_modules. Should help to dispell some of the concern around this issue.

    xjm’s picture

    Issue summary:View changes

    Updated issue summary.

    alexpott’s picture

    StatusFileSize
    new198.19 KB
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1199946-317.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    new40.88 KB

    Noticed that we're introducing a new function module_install() and immediately deprecating it. This doesn't make a great deal of sense.

    Status:Needs review» Needs work

    The last submitted patch, 1199946-317.patch, failed testing.

    alexpott’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new197.77 KB
    PASSED: [[SimpleTest]]: [MySQL] 56,166 pass(es).
    [ View ]

    Rerolled after Entity storage changes

    alexpott’s picture

    Issue summary:View changes

    Adding follow up

    alexpott’s picture

    Issue summary:View changes

    Updating solution

    alexpott’s picture

    Issue summary:View changes

    Removing potential follow ups as it is not necessary

    alexpott’s picture

    Issue summary:View changes

    Removing followups that have been done :)

    alexpott’s picture

    Issue summary:View changes

    Updated issue summary.

    alexpott’s picture

    StatusFileSize
    new198.88 KB
    PASSED: [[SimpleTest]]: [MySQL] 56,239 pass(es).
    [ View ]
    new1.75 KB

    Added tests for new hooks hook_preinstall_module and hook_preuninstall_module.

    alexpott’s picture

    Issue summary:View changes

    Updated issue summary.

    alexpott’s picture

    StatusFileSize
    new200.96 KB
    FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
    [ View ]
    new2.52 KB

    After discussing the removal of the ModuleEnableDisable test (our longest running test btw) with @fuhby and @chx on IRC

    09:44 fubhy: alexpott, chx the problem with that test is, even if you get a failure that, that -in most cases- doesn't tell you anything. it just shows that there is SOMETHING going wrong. Debugging that test is not an option though
    09:44 fubhy: so it's really just a blank warning signal
    09:44 chx: correct
    09:45 alexpott: yep but at least we have the signal
    09:45 chx: usually enabling forum from minimal (cos it enabled taxonomy) then disabling and uninstalling everone involved
    09:45 chx: shows what's wrong
    09:45 chx: as it covers dependencies

    Often that test was a single point of failure in the test suite that picked up certain regressions. So I've added an uninstall all the dependencies and reinstall the forum module to a test in Drupal\forum\Tests\ForumUninstallTest.

    alexpott’s picture

    StatusFileSize
    new200.95 KB
    PASSED: [[SimpleTest]]: [MySQL] 55,929 pass(es).
    [ View ]
    new758 bytes

    Just what we need - more comments on this issue :) - but ho hum.

    alexpott’s picture

    Issue summary:View changes

    Adding instructions for contrib

    amateescu’s picture

    Here comes a nitpicky review, but only for lines that are already touched by this patch.

    1. +++ b/core/includes/language.inc
      @@ -203,8 +203,8 @@ function language_types_disable($types) {
      +  // invocation of \Drupal\Core\Extension\ModuleHandler::install() or
      +  // module_uninstall() could outdate the cached information.

      @@ -343,8 +343,8 @@ function language_negotiation_get_switch_links($type, $path) {
      +  // invocation of \Drupal\Core\Extension\ModuleHandler::install() or
      +  // module_uninstall() could outdate the cached information.

      These two refereces look a bit weird, first one pointing at a method from the module handler and the second pointing to the procedural helper.

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
      @@ -253,77 +253,50 @@ public function invokeAll($hook, $args = array());
          * @param $enable_dependencies
      ...
          * @return
          *   FALSE if one or more dependencies are missing, TRUE otherwise.

      These two need type hints (bool for both).

    3. +++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
      @@ -105,7 +102,7 @@ public function enable($module_list, $enable_dependencies = TRUE) {
      +        $config_storage = \Drupal::getContainer()->get('config.storage');

      \Drupal::service('config.storage')?

    4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorConfigurationTest.php
      @@ -59,7 +59,7 @@ function testSettingsPage() {
      -    module_disable(array('aggregator_test'));
      +    module_uninstall(array('aggregator_test'));

      +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/UpdateFeedItemTest.php
      @@ -72,7 +72,7 @@ function testUpdateFeedItem() {
      -    module_disable(array('aggregator_test'));
      +    module_uninstall(array('aggregator_test'));

      +++ b/core/modules/block/block.install
      @@ -184,7 +184,7 @@ function block_update_8005() {
        */
      ...
      -  module_enable(array('custom_block'));
      +  Drupal::moduleHandler()->install(array('custom_block'));

      I noticed that for installing modules we switch to the longer version (Drupal::moduleHandler()->install()) but for uninstalling we stay with the shorter function wrapper (module_uninstall()). Any reason to not go with the longer version everywhere since we're already touching those lines?

    5. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.php
      @@ -61,31 +61,32 @@ function testCommentDefaultFields() {
      +   * Tests that comment module works when install after a content module.

      "installed"

    6. +++ b/core/modules/locale/config/locale.settings.yml
      @@ -3,7 +3,7 @@ javascript:
      -  check_disabled_modules: '0'
      +  check_disabled_projects: '0'

      This change looks a bit weird.. why do we change "modules" to "projects"? A custom module is not a project.

      Wouldn't it be better to switch to "extensions"?

    7. +++ b/core/modules/overlay/overlay.install
      index a88fe6a..8acf925 100644
      --- a/core/modules/php/php.install

      Offtopic.. but sad to see we still have this in core :P

    8. +++ b/core/modules/system/lib/Drupal/system/Tests/Module/DependencyTest.php
      index 8458bc7..0000000
      --- a/core/modules/system/lib/Drupal/system/Tests/Module/EnableDisableTest.php

      --- a/core/modules/system/lib/Drupal/system/Tests/Module/EnableDisableTest.php
      +++ /dev/null

      No one is going to miss this one!

    9. +++ b/core/modules/system/lib/Drupal/system/Tests/Module/InstallTest.php
      @@ -7,6 +7,7 @@
      +use Drupal\Core\Extension\ExtensionNameLengthException;

      Related to a point above: apparently we do call them extensions, not projects.

    10. +++ b/core/modules/system/lib/Drupal/system/Tests/Module/InstallTest.php
      @@ -46,6 +47,53 @@ function testDrupalWriteRecord() {
      +  function testEnableUserTwice() {
      ...
      +  function testRequiredModuleSchemaVersions() {
      ...
      +  function testModuleNameLength() {

      I didn't say anything about a few test methods that were just renamed in the first half of the patch, but at least the new ones should get the member visibility.

    11. +++ b/core/modules/system/system.api.php
      @@ -1958,14 +1944,13 @@ function hook_modules_preenable($modules) {
      + * the order in which install and enable hooks are invoked.

      This line is already touched, so we could remove the "and enable" part.

    12. +++ b/core/modules/system/system.api.php
      @@ -1975,45 +1960,13 @@ function hook_modules_installed($modules) {
      + * @param string $modules

      @param string $module (singular, not plural).

    13. +++ b/core/modules/system/system.module
      @@ -2570,13 +2570,11 @@ function system_rebuild_module_data() {
      +    $all_modules = (array) Drupal::config('system.module')->get('enabled');
      ...
      +      $record->status = (int) isset($all_modules[$module]);

      I think $installed_modules would be more accurate here.

    alexpott’s picture

    StatusFileSize
    new196.27 KB
    PASSED: [[SimpleTest]]: [MySQL] 55,991 pass(es).
    [ View ]
    new12.67 KB

    Rerolled after forum service patch and comments in #323

    alexpott’s picture

    Added follow up #2079513: hook_modules_installed runs in a inconsistent environment to cope with differences (that currently exist) between installing 1 module at a time and multiple modules.

    alexpott’s picture

    Issue summary:View changes

    Updating issue summary

    alexpott’s picture

    Issue summary:View changes

    Adding follow up

    amateescu’s picture

    Status:Needs review» Reviewed & tested by the community

    This issue already got attention/code/reviews from most people that really know this system, so assuming the patch is green, there's only way forward: rtbc :)

    chx’s picture

    Status:Reviewed & tested by the community» Needs work
    StatusFileSize
    new3.1 KB

    I am sorry for resetting to CNW but this degrades the performance of Drupal install quite a lot because it removes the optimization from module_enable (now install) where system_rebuild_module_data was not called when the enable_dependencies was set to FALSE -- which is what _install_module_batch is doing.

    Another thing I would like to see although it can be a followup is the removal of addNamespaces. It's an abomination that exists only because I was unable to solve the bundle uninstall otherwise. As both calls are removed, it is very easy to remove. I have attached a patch you can patch -Rp1 and it's gone.

    alexpott’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new192.76 KB
    PASSED: [[SimpleTest]]: [MySQL] 55,863 pass(es).
    [ View ]
    new2.71 KB
    new12.98 KB

    Okay this patch should address the performance regression discovered by @chx

    chx’s picture

    Status:Needs review» Reviewed & tested by the community

    Thanks!

    David_Rothstein’s picture

    Status:Reviewed & tested by the community» Needs work

    I'm still working on the review mentioned above.

    In the meantime, going from this:

    -  /**
    -   * Tests that all core modules can be enabled, disabled and uninstalled.
    -   */
    -  function testEnableDisable() {
    -    $modules = system_rebuild_module_data();
    -    foreach ($modules as $name => $module) {
    -      // Filters all modules under core directory.
    -      $in_core_path = (strpos($module->uri, 'core/modules') === 0);
    -      // Filters test modules under Testing package.
    -      $in_testing_package = ($module->info['package'] == 'Testing');
    -      // Try to enable, disable and uninstall all core modules, unless they are
    -      // hidden or required or system test modules.

    to this:

    +  /**
    +   * Tests that a fixed set of modules can be installed and uninstalled.
    +   */
    +  public function testInstallUninstall() {
    ...
    +    foreach (array('book', 'ckeditor', 'toolbar') as $name) {
    +      $modules[$name] = $all_modules[$name];

    is not OK.

    That wouldn't catch something like https://drupal.org/node/1029606#comment-4524904 (which was the original bug that caused these tests to be added in the first place).

    webchick’s picture

    Since there's a UI impact on this patch, here's a screenshot of the modules page:

    Checkboxes on modules page

    Once modules are enabled, the checkboxes next to them get "locked" and there is no way to uncheck them.

    Here's a screenshot of the uninstall page:

    Checkboxes on uninstall page

    Unlike before, now all modules (not disabled ones only) are listed on the uninstallation page. If a module is a requirement of another module, its checkbox is "locked" and there's no way to check it unless the dependent module is uninstalled first.

    webchick’s picture

    Warnings about how destructive uninstallation is happen at two points (afaik, unchanged from this patch):

    1) At the top of the "uninstall modules" table in plain old regular text in the "invisible zone" above the table. (That's a "needs work," too, since this text erroneously mentions disabling modules being a requirement to use this page.)

     The uninstall process removes all data related to a module. To uninstall a module, you must first disable it on the main Modules page.

    2) In the confirm box after you've checked the box and told it to uninstall:

     The following modules will be completely uninstalled from your site, and *all data from these modules will be lost!*

    Are either of those explicit/visual enough to catch the attention of someone who's expecting Drupal, like every other system with the concept of extensions on the entire planet, to have a concept of "disabling" plugins and preserving their data? I couldn't say, but I suspect not.

    alexpott’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new194.38 KB
    PASSED: [[SimpleTest]]: [MySQL] 57,558 pass(es).
    [ View ]
    new5.04 KB

    Re #330 well that's the interesting thing with this patch - that issue completely goes away. .modules are loaded when hook_uninstall is fired - so other hooks can fire etc. See entity_module_preuninstall() for why this is great. And this gets even better once #2079513: hook_modules_installed runs in a inconsistent environment lands as we will be assured of what is actually loaded when the installed and the uninstalled are fired. At the moment you can not assume that any module apart from required modules are enabled!

    That said @beejeebus and I spent sometime rewriting this test to work like the old test and discovered something interesting that the old test did not catch.

    02:19 alexpott: beejeebus: do you wanna know what the problem is?
    02:19 beejeebus: alexpott: yes, yes i do :-)
    02:20 alexpott: beejeebus: so custom_block gets enabled
    02:20 alexpott: beejeebus: creates an new custom_block bundle of type basic
    02:20 alexpott: beejeebus: this gets a body field added
    02:20 alexpott: beejeebus: then custom_block is disabled and now (becuase of the patch) this bundle is deleted
    02:21 alexpott: beejeebus: marking the field as purged
    02:21 alexpott: beejeebus: then forum comes along and gets installed
    02:21 alexpott: beejeebus: and then it gets uninstalled
    02:21 beejeebus: lulz
    02:21 alexpott: beejeebus: forum tries to play nice and purge the fields it deletes
    02:22 alexpott: beejeebus: and then run field_purge_batch a couple of times so that you can disable taxonomy and options
    02:22 alexpott: beejeebus: but now it picks up the custom_block field
    02:23 alexpott: beejeebus: but that's no longer installed... a kaboom

    So this patch gets round it by adding a check to field_purge_batch to ensure that it is remove fields for entity types that still exist.

    The patch also fixes the help text pointed out by @webchick in #332

    beejeebus’s picture

    Status:Needs review» Reviewed & tested by the community

    yay, alexpott++, so we've addressed #330.

    i think this is ready to fly.

    David_Rothstein’s picture

    Status:Reviewed & tested by the community» Needs review

    Thanks. It was less the specific issue in https://drupal.org/node/1029606#comment-4524904 that I was worried about than the fact that it was a bug that affected uninstall for a couple random core modules (and an unknown, but nonzero, number of contrib modules). The only way to catch things like that is to test all the modules.

    I think this definitely still needs review based on the comments above (@webchick's and ones before it).

    beejeebus’s picture

    is asking that we do UI stuff in a critical follow up ok? i know of a couple of CMI issues that will benefit from this change, that won't care about the UI.

    tim.plunkett’s picture

    Status:Needs review» Reviewed & tested by the community
    tim.plunkett’s picture

    Issue summary:View changes

    fixing html

    catch’s picture

    Yes there's half a dozen follow-ups listed in the summary, split between post-commit clean-up to keep the patch 200kb rather than 500kb, and bugs that are blocked on this issue.

    xjm’s picture

    Assigned:Unassigned» Dries

    I'm not fully convinced that all the feedback has been addressed, and postponing critical work to followups is something Dries has repeatedly stated should not be done at this point in the release cycle. Rushing to commit this patch seems unwise to me. That said, I'm also very aware of just how much this is blocking and how problematic scope is for this issue.

    But I'm not going to play RTBC wars. This patch is too fundamental a change to Drupal as a product for someone other than Dries to commit it, so I'm making that explicit.

    catch’s picture

    It comes down to whether the follow-ups this issue creates are likely to hold things up vs. the issues it blocks staying blocked for longer.

    I think the balance of this issue is that it unblocks a lot more work than it creates (and the various issues after this goes in don't have interdependencies), but I also opened it more than two years ago so I'm not neutral.

    If more needs to be done here, then yes we really need to figure out what the scope of that remaining work is.

    xjm’s picture

    @catch: Yeah, agreed. I'd really rather see this go in soon, and my gut is that unblocking the entirety of CMI and whatever else will outweigh the risk of adding a critical UI followup or two, but I also think more review is needed here. (None of the RTBCs above are terribly thorough.) And I think it's important for the buck to stop with Dries for potentially controversial decisions.

    @David_Rothstein: You indicated in #330 that you are still working on a review; is that the case? If so we'll wait for that.

    David_Rothstein’s picture

    Yeah, that is my long-promised and not-delivered review :( I am really hoping to do that tomorrow... but if I can't get to it tomorrow, it unfortunately won't be possible until at least the weekend.

    So, thank you for suggesting that it be held up, but it's also probably not fair for me to hold up an issue more than a couple days. At the same time, I agree that this issue/patch (despite the large number of comments) has not been reviewed enough, given the effect the proposed change is potentially going to have on Drupal.

    alexpott’s picture

    StatusFileSize
    new192.31 KB
    PASSED: [[SimpleTest]]: [MySQL] 57,549 pass(es).
    [ View ]
    new2.61 KB

    Realised that change to ForumUninstallTest added in #321 is completely unnecessary now that InstallUninstallTest does it and does it for all modules.

    fubhy’s picture

    So, thank you for suggesting that it be held up, but it's also probably not fair for me to hold up an issue more than a couple days. At the same time, I agree that this issue/patch (despite the large number of comments) has not been reviewed enough, given the effect the proposed change is potentially going to have on Drupal.

    Are you doing a pure code-review or also concept/ui/etc? Because we are aware of the UI problems that need to be solved. There is a dedicated issue for that and we can hopefully get Dries to agree that we don't want to further increase the size of the patch by solving that right here. It's also going to result in a lot of bikeshedding about what the right approach for the module install/uninstall page is. @yoroy and @Bojhan had some nice ideas for that already.

    Code-wise, I think, we did get a lot of reviews already. Although, to be fair one can never get enough reviews...

    So, yes, we do have to solve a few usability issues, especially around the install/uninstall pages. But first I think we really need to unblock all those other issues, especially for CMI. Otherwise we are just going to accumulate an even longer trail of blocking tasks.

    yched’s picture

    re @alexpott #333:

    this patch gets round it by adding a check to field_purge_batch to ensure that it is remove fields for entity types that still exist

    Aw, nice catch :-/. Actually there are two places that should be checked. Posted a patch in #2081609: field purge should bail out on unknown entity types

    Dries’s picture

    Clearly, this is a difficult issue. All things considered, I'm still planning to commit this, but I'm going to wait one week for David to come back with his analysis, and for the rest of us to make a plan for the follow-up issues.

    webchick’s picture

    I support committing this as an interim step to unblock CMI and related issues. However. I think there are several people in this issue who've made an extremely strong case for the "temporarily disable a module for debugging/performance/whatever" reasons, and feel like they're getting steamrolled quite a bit by the folks who've actively been bitten by the data integrity issues that this can cause if this disabling is not so temporary.

    Catch actually made a suggestion in a conversation earlier today, which I think is brilliant, and have written down at #2081873: Re-implement module disabling in a temporary/debugging capacity that is environment aware and explicit about risks to data integrity.

    David_Rothstein’s picture

    I'd say I'm about 50% done, and will get back to it this weekend and finish then, barring natural disasters, etc. Thanks for the extra time.

    I support committing this as an interim step to unblock CMI and related issues.

    So all the big issues that are blocked on this, is it basically these?

    #1765936: Invoke ConfigStorageController::delete on module uninstall and add tests
    #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall
    #2026983: Clean up module permissions on uninstall

    If so, I think they might all be unblocked by the patch I posted at #734710-18: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled. It's a simple 2kb patch that doesn't do anything as radical as the patch here. Not perfect (and not passing tests yet either), but if we're talking about an interim commit to unblock other issues, I think it's worth looking at...

    fubhy’s picture

    If so, I think they might all be unblocked by the patch I posted at #734710-18: API functions that invoke hooks don't work as expected when a disabled module is being updated or uninstalled. It's a simple 2kb patch that doesn't do anything as radical as the patch here. Not perfect (and not passing tests yet either), but if we're talking about an interim commit to unblock other issues, I think it's worth looking at...

    Whatever we do, we have to do this drastical step including the removal of certain API layers involved (hooks, etc.) to both, proceed with the blocked issues and to start thinking about possibly re-implementing this feature in a way that does not screw up core (and contrib) development. As long as we support it in it's current form (and take responsibility for it) or even claim that it works and provide code to manage the states of things affected by this (just think about fields, etc.) we are needlessly and drastically increasing the complexity of our system and even entirely blocking things that would otherwise simply work.

    I posted my thoughts about that over at #2081873: Re-implement module disabling in a temporary/debugging capacity that is environment aware and explicit about risks to data integrity which is also the place, I think, where we should discuss how to get this back (in a suitable form that does not screw over core, or any, development).

    falcon03’s picture

    Given that as I stated I'm completely against the removal of enabling/disabling module functionality, getting this patch in and then discuss how to get back the functionality it removed sounds a bit weird to me, but I'm fine with that if that follow up is a critical (and so release-blocker) issue. These will be my last two cents on this issue.

    On a side note: when refactoring the UI, (either in this issue or in the follow up) let's make sure to not introduce accessibility issues.

    catch’s picture

    @David, #1808248: Add a separate module install/uninstall step to the config import process is another critical CMI issue. I don't know if it's completely blocked or not, but certainly it's a lot more straightforward once this gets committed.

    Looking at #2081873: Re-implement module disabling in a temporary/debugging capacity that is environment aware and explicit about risks to data integrity that would make it harder rather than easier - i.e. how do you deploy the uninstallation of a disabled module if the uninstall process requires enabling the module first?

    beejeebus’s picture

    i can't speak for anyone else, but i have zero interest in working on those CMI issues until enable/disable goes away.

    ParisLiakos’s picture

    This issue is blocked on this one as well #2004316: Exception on field_purge_batch() after deleting a field...which makes #15266: Replace aggregator category system with taxonomy blocked as well and eventually postponed to d9.
    the faster this issue goes in the better imho

    mtift’s picture

    hass’s picture

    Before we auto-truncate all data from databases. How is the CORE module named that backup/exports/re-imports the modules data/permissions/etc. in a consistent way? I guess I'm standing alone in the dark with this stuff with a comment like - your are on your own. But this is not the way how this works for the most. This is a core job and not an unreliable contrib job, too.

    Data consistency or not, this is a show stopper.

    alexpott’s picture

    StatusFileSize
    new192.33 KB
    FAILED: [[SimpleTest]]: [MySQL] 57,686 pass(es), 1 fail(s), and 0 exception(s).
    [ View ]

    Conflicted in core/modules/block/lib/Drupal/block/Tests/BlockTest.php due to #2078341: Restructure Block UI and custom block pages

    Status:Reviewed & tested by the community» Needs work

    The last submitted patch, 1199946-356.patch, failed testing.

    alexpott’s picture

    Status:Needs work» Reviewed & tested by the community
    StatusFileSize
    new193.07 KB
    PASSED: [[SimpleTest]]: [MySQL] 57,571 pass(es).
    [ View ]
    new604 bytes

    #2082499: Uninstalling action module removes actions created by the user module introduced a new test that disabled a module - fixing and setting back to rtbc as the change is minor and in no way changes the solution.

    sun’s picture

    Features can be added and removed at any time.

    The special case here: By removing this facility, a situation similar to #8 is introduced — a fundamental aspect of the application's architectural design ceases to exist.

    Thus, a decision is only hard to make, because it will be extraordinarily painful to reintroduce a fundamental facility like this into the application's architecture later on. I'm talking weeks or even months of effective work by the most skilled engineers.

    That is to say, the decision should not be taken lightly. However, everyone who is familiar with the internals of the related issues is certainly able to confirm that removing the current implementation is the only way to guarantee integrity of user data and to make the application meet user expectations.

    From an architectural perspective though, I would recommend to consider retaining the concept, without its current implementation. Implementations can be easily added, changed, replaced, and removed; Architectural application concepts cannot.

    That's the most important part.

    Aside from that - while reading through a depressingly low signal/noise ratio of comments - some interesting ideas were raised, but mostly ignored. I'd like to summarize and highlight them:

    1. In #156, @joachim suggested to retain hook_disable(), but only support it for modules that implement the hook.

      Clarify in the hook documentation that it may only be used by modules, which are guaranteed to have no data integrity issues.

      This essentially boils down to modules that have absolutely no dependencies on other modules (including core modules, AND including required core modules). "Dependencies" includes the notion of optional support code for other modules, too. In short, if you have a completely independent isle of a module, then there is nothing wrong with disabling that.

      The example of Google Analytics module was compelling (although I did not verify whether it actually meets the above requirements).

      Given D8's architecture though (involving plenty of non-obvious dependencies), I wonder/doubt how useful that distinction would be.

    2. In #227, @mikeryan suggested a dump-restore approach for the enable/disable concept.

      That suggestion was certainly by far the best alternative resolution proposal, as it cuts 100% on the underlying problem space:

      When getting re-enabled, disabled modules are effectively "imported" into a system that has progressed independently since the module was disabled.

      Spot-on. However, data is being imported from an undefined state, which means that modules would have to provide hook_import_N() implementations for every single hook_export() state of their lifetime. Re-injection of all data also would not be reliable, since the rest of the system's configuration and data design might have changed and progressed since the export. I've to admit, I don't know how Migrate handles these conflicts, but I'm also certain that Core can learn a lot from Migrate.

      In any case, that almost appears to be the only suggested solution in this thread that is of technical merit and thus deserves huge respect.

    3. In #232, @joachim suggested to retain most of the facility as-is, but force-enable the maintenance mode in case any module is disabled.

      That suggestion certainly does not attempt to fix the underlying data integrity problems in any way, but it does clarify on a few concrete things: (1) This state is not supported. (2) The site is undergoing maintenance, and maintenance means that things can go wrong. (3) Ordinary site visitors and users are prevented from causing data integrity problems. (4) To get back to normal operations, enable all modules.

      In fact, that is the only alternative and viable solution at this point, both from a technical and UX perspective. It is not ideal, as it only fixes the symptom, but not the root cause. But it does appear to fix the primary issue of concern: Not having to support data corruption and data integrity issues caused by disabled modules.

      Furthermore, it might have to be enhanced with a global application flag that causes all modules to deny certain data/reference changing operations.

    Thanks a lot to @joachim and @mikeryan for thinking about these alternative suggestions and raising them. I'm sure those took a good amount of time as well as studies on potential consequences. That is the level of input that is required here.

    Please note that D8's architecture may require us to perform the identical (whichever) resolution of this issue for themes, too. This aspect was not raised in the discussion thus far. There is little difference between modules and themes when it comes to configuration, potential cross-references in configuration, parent-theme to child-theme inheritance, and worse, module-specific theme settings. As a result, the identical issues that apply to modules also apply to themes. (This is why the concepts ought to be abstracted into "extensions".)

    Last but not least, I'd like to clarify one question that was originally raised in #280 by @joachim, and which continued to cause confusion later on:

    Removing/deleting a module's files without uninstalling it is generally no longer supported in D8. You will get (and you deserve) a fatal error. Modules are no longer "silently disabled"; that execution path cannot be reached in the first place anymore. This has been changed ~2 years ago already; primarily due to performance reasons. Various file_exists(), conditional code paths, and other safety measures have been removed. It was inappropriate and not helpful to support that dirty state in the first place.

    Finally, while I'm confident that these considerations (and certainly more) also played a key role in evaluations, the project lead's level of involvement in this issue and communication on the matter is, frankly, unacceptable. — Decision authority in an open-source project relies on trust. Not outlining how and why someone comes to a certain conclusion, and which consequences have been considered when making and stating a decision, is the approach of obscurity, to avoid being made responsible for decisions later on. To be clear, this is a major change proposal about the most important subsystem in the Drupal product: The module system. A fundamental aspect of the Drupal architecture that made it popular. If that doesn't deserve attention, then what is?

    StuartJNCC’s picture

    Tanks, @sun for a very thoughtful overview.

    I don't understand what you mean by

    By removing this facility, a situation similar to #8 is introduced

    The link takes you to a very old issue which does not seem to be relevant. Comment #8 in this thread (from webchick) also does not seem to fit the context. Elucidate?

    Your item 1: @joachim's suggestion that some modules stand-alone and can be safely disabled.
    Seems OK in principle, but is the module screen the right place for this? For example, the (old, pre-Drupal8) Views_UI module would seem to be a good example. The documentation suggests that this can be turned off on a live site. Fine, but it always seemed strange to me that this was done on the module screen. Disabling/uninstalling a module always struck me as a pretty drastic thing to do. I would prefer to see this as a configuration setting ("Developer UI to add or change views on/off" with an explanation that turning this functionality off on a live site could slightly improve performance by avoiding some code being run). @xmacinfo suggested something like this in #157.
    Your item 3: @joachim's suggestion that the functionality to disable modules should be retained, but only temporarily and by forcing site into maintenance mode.
    The main use case for retaining the ability to disable modules seem to me to be the debugging one. You have added a module, you get a problem and suspect it is caused by the new module. So disable it and see if the problem goes away. The suggestion that the site be put in maintenance mode and then all hooks implemented by a module are disabled would seem to cover this.
    Your item 2: @mikeryan's suggestion of a dump-restore approach.
    I agree with your analysis that this is the only fix for the issue that could possibly meet all the issues. However it seems likely to be a huge amount of work and merging the data back in could be very problematic. It is also likely to take quite a long time, especially the import (and I would want to test in thoroughly before I relied on it!), so it doesn't really answer the debugging use-case.

    There seems to be a fair amount of consensus that something like @sun's 3 (force site into maintenance mode and temporarily disable functionality) needs to be done. There is a debate about whether it needs to be done now, in core, or can be done by contrib later. I do not have the technical understanding of the internals to judge, but it does seem to be the key decision that core developers need to take. If they decide it needs to be done in core for technical reasons, then I guess it should either be part of this issue or, at least, a critical follow-up.

    fubhy’s picture

    Thanks for your comments... However, I would like to move this discussion to #2081873: Re-implement module disabling in a temporary/debugging capacity that is environment aware and explicit about risks to data integrity which was opened exactly for that... Let's keep this issue clean for actual reviews and discussion for the patch. We are already well over 300 comments and really need to keep focused now if we ever want to proceed with this issue.

    David_Rothstein’s picture

    I went through this entire issue and every single related issue that I could find, trying to understand what the actual problems with disabled modules really are.

    As we know, there are tons of issues linked here, some of them highly technical, and this has been a huge barrier for site builders and site administrators (who would be by far the most affected if this feature is removed from Drupal) and UX folks to participate in the discussion.

    What I've attempted to do here is characterize the issues into categories and explain them, to facilitate actually evaluating this issue.

    After reading through these, I don't see any evidence that the disabled module functionality is inherently broken or has inherent data loss issues. I know that has been claimed several times in this issue (and for a while I believed it too) but I'm simply not seeing the evidence. If I left an important issue out below, please feel free to correct me.

    1. Issues that are related to the fact that modules must be disabled before they are uninstalled

      Examples:

      #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall
      #1765936: Invoke ConfigStorageController::delete on module uninstall and add tests
      #943772: field_delete_field() and others fail for inactive fields (partly; see also "long-running processes" section below)
      #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it)
      #1115510: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall() (partly; see also "garden-variety bugs" section below)
      #1872876: Turn role permission assignments into configuration.
      #2026983: Clean up module permissions on uninstall
      #737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module)
      #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced
      #1503314: Remove the concept of active / inactive (field types, storage) from Field API
      #1204820: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall()
      #1330472: clean up entity-related data on module uninstall

      Explanation:

      This is the largest category of issues.

      In short, the problem here is that because Drupal requires that a module first be disabled and then (at a later time) uninstalled, by the time the module is uninstalled it is already turned off. Its normal code can't run, so during the uninstall process it is very limited in what it is able to do and what other modules are able to do with it.

      In my estimation this part is indeed broken beyond repair, but it certainly doesn't require removing the entire concept of disabled modules to fix. As has been stated several times in this issue, we can simply require that modules be enabled before they are uninstalled (rather than disabled), and the patch I posted at #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled proves it.

    2. "Orphaned configuration" issues (configuration for module A contains within it configuration for module B, and the latter disappears)

      Examples:

      #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies)
      #2007248: When CKEditor module is uninstalled, the right text editor config entities are affected but their labels are missing
      #1822048: Introduce a generic fallback plugin mechanism
      #1881096: Regression: Site broken when disabling module without removing block instances.

      Explanation:

      Views plugins are a good example of this (where Views is "module A"). Views has to gracefully handle the case where some parts of the View configuration provided by module B no longer exist.

      As has been mentioned several times above (see for example Crell's comment in #130), although disabling module B is certainly one way to trigger this, it is not the only one. Similar issues arise if module B is uninstalled, or if module B is updated to a new version in which some of the configuration has been changed or removed.

      Removing the concept of disabled modules from Drupal will not solve these problems.

    3. "Orphaned data" issues (module A stores references to module B's data, then module B's data changes while module A is disabled, then module A is enabled again)

      Examples:

      #1451072: Deleting a comment author while the Comment module is disabled leads to an EntityMalformedException error after it's reenabled
      (my guess is there are others, but that's really the main one linked here)

      Explanation:

      The issue here is that because module A was disabled at the time module B's data changed, it didn't know about the change, so when module A is turned on again it still thinks module B has the old data. (This is actually related to the previous category in many ways; maybe I shouldn't have split them up.)

      Ultimately (and this is a bit technical), the issue comes down to basic programming principles. If module A is going to work with data, somewhere along the way it needs to verify that data; it is never a good idea for code to blindly assume that the data it is loading is actually what it thinks it is because "there is some independent code path somewhere else that is supposed to keep it up to date". There are tons of ways that can go wrong, and the disabled module issue is only one of them (another would be, a year ago the module had a bug where it didn't keep its data references up to date; the bug has since been fixed, but the module has been used on this site for more than a year so its old data still has that problem)... In the example above, the specific issue is that the Comment module is calling its own API function to return a list of comment IDs, then loading the comments and assuming they are all properly loaded. Disabled modules or not, either (a) the API function should guarantee a valid set of comment IDs is returned, or (b) the calling code should deal with the possibility they are not.

      It has been stated above by some that this would lead to empty() and isset() checks all over the place but I don't see how it will lead to more than necessary or evidence that there have been tons of them added (or that need to be added). Generally, these are consolidated in central API functions so there don't have to be that many of them.

    4. Issues related to long-running processes which are interrupted when a module is disabled

      Examples:

      #943772: field_delete_field() and others fail for inactive fields (partly; see also "disabled => uninstalled" section above)

      Explanation:

      The basic idea here is that a module may be undergoing a "cleanup" process that takes a long time to run (e.g. split up over many cron runs). If the module is disabled while that process is still ongoing, it will not be able to finish.

      This issue is also not limited to disabled modules, since the same thing can happen if you uninstall the module before the process has finished. Now in some cases, having the cleanup process interrupted by uninstall is not as bad as having it interrupted by disable (since uninstalling may clean up the remaining data on its own anyway, via brute force when it removes all data associated with the uninstalled module). But that's definitely not always the case and in fact wasn't the case in the above issue; there were orphaned files in the file system that were being cleaned up by the long-running process, and as far as I know that is cut short by uninstall just as much as it is cut short by disable.

      So, the code added in the above issue (which took the approach of disallowing the module from being disabled/uninstalled until the cleanup process is finished) is not going to be removed by getting rid of disabled modules. In fact, people have even been talking about generalizing it and expanding it to other scenarios where uninstall needs to be delayed or prevented. (And such an extension could certainly work equally well for disable as it does for uninstall.)

    5. "Garden-variety" bugs involving disabled modules

      Examples:

      #1887904: update_retrieve_dependencies() only considers enabled modules
      #2031707: field_sync_field_status() does not enable fields defined without hook_field_info()
      #2032369: _update_8003_field_create_field() relies on field schemas never changing and plugins being available
      #1115510: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall() (partly; see also "disabled => uninstalled" section above)
      #1227966: Dynamically-defined blocks are never removed from the site, even when they disappear from hook_block_info()
      #895014: All fields of a node type are lost on module disable
      #1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors

      Explanation:

      My categorization of these as "garden-variety" bugs is not meant to trivialize them; it does not mean that lots of work didn't go into fixing them (lots of work did), and it does not mean that they weren't serious bugs (some were very serious).

      Instead, what I mean is that looking at the proposed or committed patch in these issues, it seems like they are "just bugs"; rather than indicating any fundamental flaw in the concept of disabled modules, they appear to simply be cases where someone wrote code for Drupal that didn't take into account disabled modules, and changing the code to take disabled modules into account and fix the bug was reasonable and possible.

      Pretty much every area of Drupal has had bugs (including critical bugs) over the years, so finding a few of these is definitely not an argument for removing the feature. It would only be an issue if there were a huge number of them, which as far as I can see is not the case.

      Also, as was the case in some of the previous categories, several of these bugs here are not limited to disabled modules; some of them affected uninstalled modules or other scenarios also.

      I want to call special attention to the last issue (#1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors) since it has been mentioned frequently in this issue as a bug that involved data loss. First of all, the data loss was not limited to disabled modules. For example, if you disabled the Forum module and updated your site from Drupal 6 to Drupal 7, comment bodies (stored by the Comment module) were destroyed; however, it is equally true that uninstalling the Forum module on Drupal 6 and then updating from Drupal 6 to Drupal 7 would have incorrectly destroyed the very same comment bodies. Second (and more important), there is a huge difference between "Drupal once had a bug that caused data loss in disabled modules" and "Disabled modules have fundamental data loss issues". I put this issue squarely in the first category; if you look at the eventually-committed patch the bug was basically that some code used by one of the callers of _update_7000_node_get_types() needed to be moved inside that API function so that other callers could benefit from it too. This is just a bug; it's not an inherent flaw with disabled modules in any way.

    6. Issues that don't appear to be directly related to disabled modules

      Examples:

      #773828: Upgrade of disabled modules fails
      #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API
      #2004316: Exception on field_purge_batch() after deleting a field
      #1458338: Media file widget causes unrecoverable error after disabling and re-enabling media module
      #1387438: Timeout on enabling modules: make it a batch operation
      #2079513: hook_modules_installed runs in a inconsistent environment
      #2004784: Move module install/uninstall implementations into ModuleInstaller
      #2024083: [META] Improve the extension system (modules, profiles, themes)
      #1808248: Add a separate module install/uninstall step to the config import process

      Explanation:

      The above issues were all cases where it wasn't clear to me how they are meaningfully related to this issue (or related anymore), or where the only relationship to this issue seems to be that they happen to touch some of the same code as the patch here does (i.e., a couple are marked postponed on this one but there is no fundamental reason they need to be), or where I couldn't reproduce the issue, etc.

      Where appropriate, I've left comments on those issues asking for clarification. If I've misinterpreted them, I assume someone will correct me :)

      The last issue (#1808248: Add a separate module install/uninstall step to the config import process) in particular is one that was specifically mentioned in a recent comment here as being blocked on this one, but I don't yet understand how either the patch here or the one in #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled affect it, except perhaps by requiring changes to a few very small lines of code.

    David_Rothstein’s picture

    Status:Reviewed & tested by the community» Needs review

    So in summary, after reading through this entire issue and pretty much everything it links to (plus some issues that it doesn't link to):

    1. I didn't see any evidence that disabled modules are "broken beyond repair", except for the narrow issue of the disabled => uninstall transition which can also be fixed by the much simpler (and much less controversial) approach in #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled (which is now passing tests), rather than throwing out the entire concept of disabled modules.
    2. I didn't see any issues that are blocked on this one that wouldn't also be unblocked by the above-mentioned patch in #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled.
    3. I didn't see any data loss issues associated with disabled modules. The one example of data loss is #1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors but as mentioned above that was not caused by any fundamental issue involving disabled modules (and furthermore it was triggered by uninstalled modules too).

    I think that's a "needs review".

    David_Rothstein’s picture

    I didn't see any data loss issues associated with disabled modules. The one example of data loss is #1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors but as mentioned above that was not caused by any fundamental issue involving disabled modules (and furthermore it was triggered by uninstalled modules too).

    Clarification: I suppose the original issue in #895014: All fields of a node type are lost on module disable (related to the above issue, but not the same thing) had some serious data loss too. But looking at the committed patch, it belongs in the "garden-variety bugs" category where I put it also (the data loss was not due to some inherent problem with disabled modules that allowed the data to go stale or something, but rather due to code that was reaching out and explicitly deleting things when it shouldn't have; see this explanation).

    catch’s picture

    Just quickly:

    I want to call special attention to the last issue (#1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors) since it has been mentioned frequently in this issue as a bug that involved data loss. First of all, the data loss was not limited to disabled modules. For example, if you disabled the Forum module and updated your site from Drupal 6 to Drupal 7, comment bodies (stored by the Comment module) were destroyed; however, it is equally true that uninstalling the Forum module on Drupal 6 and then updating from Drupal 6 to Drupal 7 would have incorrectly destroyed the very same comment bodies. Second (and more important), there is a huge difference between "Drupal once had a bug that caused data loss in disabled modules" and "Disabled modules have fundamental data loss issues". I put this issue squarely in the first category; if you look at the eventually-committed patch the bug was basically that some code used by one of the callers of _update_7000_node_get_types() needed to be moved inside that API function so that other callers could benefit from it too. This is just a bug; it's not an inherent flaw with disabled modules in any way.

    The issue that tries to deal with this once and for all is: #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall, which is a generic and less-hacky version of #943772: field_delete_field() and others fail for inactive fields. You didn't mention #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall in your review so I'll assume you've not seen it.

    When Forum module is uninstalled, either of these two is valid:

    1. All forum nodes and their comments should be deleted (because you wanted to completely remove that feature and everything it provides from the site).

    2. The forum node type should be 'divorced' (as opposed to orphaned) from the forum module - as if it was any other custom node type (because you're going to replace forum with some views + custom code).

    3. You abort the uninstall, realising that you have 30,000 nodes + comments that you need to figure out what to do about first (because whoops).

    Just because there's a bug with both disabled and uninstalled modules, does not mean that leaving data and configuration around orphaned by the module isn't a bug with disabled modules.

    On uninstall it's legitimate to offer a site administrator a choice between deleting dependent configuration, aborting the uninstall, and whether to remove default configuration that doesn't have an actual dependency on the module. That's not an option that can be offered when disabling the module, because of the expectation that everything comes back as it was when the module is re-enabled - so the only option we have at the moment is preventing modules from being disabled or uninstalled at all (per field_system_info_alter()) and requiring people to manually go around removing specific config before trying to remove them.

    catch’s picture

    "Orphaned configuration" issues (configuration for module A contains within it configuration for module B, and the latter disappears)
    Examples:

    #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies)
    #2007248: When CKEditor is disabled, editor config entities that refer to it cause a Twig exception
    #1822048: Consider improving/removing the concept of "broken" handlers
    #1881096: Regression: Site broken when disabling module without removing block instances.

    Explanation:

    Views plugins are a good example of this (where Views is "module A"). Views has to gracefully handle the case where some parts of the View configuration provided by module B no longer exist.

    Views plugins aren't the best example of this. The best example is field plugins - since those impact CRUD operations for entity storage. Views never has to deal with CRUD, so invalid plugins only means the View itself has trouble, it can't bleed out into the rest of the system.

    catch’s picture

    "Orphaned data" issues (module A stores references to module B's data, then module B's data changes while module A is disabled, then module A is enabled again)
    Examples:

    #1451072: Deleting a comment author while the Comment module is disabled leads to an EntityMalformedException error after it's reenabled
    (my guess is there are others, but that's really the main one linked here)

    Explanation:

    The issue here is that because module A was disabled at the time module B's data changed, it didn't know about the change, so when module A is turned on again it still thinks module B has the old data. (This is actually related to the previous category in many ways; maybe I shouldn't have split them up.)

    Ultimately (and this is a bit technical), the issue comes down to basic programming principles. If module A is going to work with data, somewhere along the way it needs to verify that data; it is never a good idea for code to blindly assume that the data it is loading is actually what it thinks it is because "there is some independent code path somewhere else that is supposed to keep it up to date". There are tons of ways that can go wrong, and the disabled module issue is only one of them (another would be, a year ago the module had a bug where it didn't keep its data references up to date; the bug has since been fixed, but the module has been used on this site for more than a year so its old data still has that problem)... In the example above, the specific issue is that the Comment module is calling its own API function to return a list of comment IDs, then loading the comments and assuming they are all properly loaded. Disabled modules or not, either (a) the API function should guarantee a valid set of comment IDs is returned, or (b) the calling code should deal with the possibility they are not.

    While there are ways that a module might not keep its data up to date, the example you gave is a bug, not a feature of the system.

    The system should be able to assume that data is going to maintained correctly, and when it's not, that's an error to be handled - ideally by throwing an exception or logging so that the user knows there's a bug can can report it.

    Allowing data to go stale on purpose then expecting modules to silently ignore that it's broken is not a good approach to that problem. That is exactly the 'inherent data loss issue' which you said doesn't exist.

    effulgentsia’s picture

    @David: thanks for the great synthesis and analysis. Regarding this point:

    As has been mentioned several times above (see for example Crell's comment in #130), although disabling module B is certainly one way to trigger this, it is not the only one. Similar issues arise if module B is uninstalled, or if module B is updated to a new version in which some of the configuration has been changed or removed.

    Similarly to the logic in #365, catch also responded to this in #131 after Crell first brought it up. The key difference is that during uninstallation, both module A and module B are still enabled, and can work together to remove module B's configuration data from module A's configuration files (and per #366, in the case of Field types and other plugins containing other data references, clean that up as well). While #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled partially solves this particular problem, it doesn't solve it for the case that while module B is disabled but not yet uninstalled, module A is unable to do what its configuration tells it to do, and for non-trivial plugins, like field types, that ends up being extremely problematic.

    David_Rothstein’s picture

    Responding to a few of these at once, since they're related:

    The issue that tries to deal with this once and for all is: #2080823: Disallow module uninstall if there are configurables that depend on that module, which is a generic and less-hacky version of #943772: field_delete_field() and others fail for inactive fields. You didn't mention #2080823: Disallow module uninstall if there are configurables that depend on that module in your review so I'll assume you've not seen it.

    ....

    On uninstall it's legitimate to offer a site administrator a choice between deleting dependent configuration, aborting the uninstall, and whether to remove default configuration that doesn't have an actual dependency on the module. That's not an option that can be offered when disabling the module, because of the expectation that everything comes back as it was when the module is re-enabled - so the only option we have at the moment is preventing modules from being disabled or uninstalled at all (per field_system_info_alter()) and requiring people to manually go around removing specific config before trying to remove them.

    ....

    Similarly to the logic in #365, catch also responded to this in #131 after Crell first brought it up. The key difference is that during uninstallation, both module A and module B are still enabled, and can work together to remove module B's configuration data from module A's configuration files (and per #366, in the case of Field types and other plugins containing other data references, clean that up as well). While #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled partially solves this particular problem, it doesn't solve it for the case that while module B is disabled but not yet uninstalled, module A is unable to do what its configuration tells it to do, and for non-trivial plugins, like field types, that ends up being extremely problematic.

    I was aware of #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall - it's actually the first issue in the first section of my list in #362 :) I put it in that section because as far as I can see, it's only blocked by the fact that you can't use a module's API while it's being uninstalled. Once that's fixed, I don't see any barrier to making it integrate nicely with disabled modules too.

    In other words, my category #2 ("configuration for module A contains within it configuration for module B, and the latter disappears") was based on the situation right now - plugins do have to deal with what happens when things they store go missing out of the blue, and it's an equal problem regardless of whether it went missing because module B was disabled or uninstalled or for some other reason.

    In the future, #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall does offer a possible way to improve the situation for uninstalled modules, but it could work just as well for disabled modules too (and with no extra effort). If you try to "turn off" a regular module, your choices would normally be (a) uninstall, or (b) disable. If you try to "turn off" module B in the above example, the choices would change to (a) uninstall and let Drupal automatically remove the dependent configuration for you, or (b) abort and leave it enabled. So you'd never be allowed to get in a situation where module B is disabled while module A still has module B's configuration stored within it.

    Remember, a key point of this is that we'd be allowing people to go directly from enabled to uninstalled, so the fact that a module is prevented from being disabled in the above situation wouldn't prevent someone from uninstalling it. If their goal is just to get the thing off their site entirely, they wouldn't be forced to go around manually removing configuration at all. They could still just choose the "uninstall and let Drupal automatically remove the dependent configuration" option directly.

    alexpott’s picture

    StatusFileSize
    new191.98 KB
    FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
    [ View ]

    Just keeping the patch applying. #2081609: field purge should bail out on unknown entity types has been committed and improves on one of the fixes in this patch.

    Status:Needs review» Needs work

    The last submitted patch, 1199946-370.patch, failed testing.

    alexpott’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new192.64 KB
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1199946-372_0.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]

    Chasing HEAD... #2084197: Uninstalling menu module removes all custom menus disables a module in a test

    thedavidmeister’s picture

    Reading over #362, I think the example given for category 3 is far from "worst case" and so understates the problem (it assumes we can make the problem go away with liberal application of empty/isset calls everywhere).

    "Harder" example of orphaned, stale data:

    1. Module A has some entity or configuration with a "machine name" of 'foo'
    2. Module B references the configuration 'foo' of Module A
    3. Module B is disabled
    4. Module A has the entity/configuration with machine name 'foo' deleted - Module B's data is now stale
    5. Module A creates a *new* entity/configuration with a machine name of 'foo' - Module B's data is now stale and irreparable.
    6. Module B is re-enabled and has absolutely no way of knowing/validating that the 'foo' its stale data references is the "wrong" foo. An isset/empty check is far from sufficient here.
    7. Unfortunately, Module B's data associated with its reference to 'foo' is not compatible with the Module A's data associated with the ID 'foo' - Fatal errors are highly likely at this point and potentially rather nasty to debug.

    From the perspective of Module A, the data associated with the ID 'foo' is *completely unrelated* to the data that was associated with 'foo' at the beginning of the example.

    From the perspective of Module B, 'foo' has always been 'foo'. If it was enabled at step 4, it would have the same opinion on what the ID 'foo' represents as Module A.

    In this case we have two options available to resolve the problem:

    1. Developer friendly "machine names" are not allowed, only UUID values that are going to be unique across all databases, all CRUD operations and all time (so, spatially and temporally), not just within this database as this moment, are allowable ways to reference data. Sequential ID values do a good job of being unique across time within a database but a terrible job of being portable across sites. This makes all existing validation callbacks for machine names (that validate against the current state of the database only) obsolete, and will make exports rather hostile to a good DX. Working with classes and IDs in CSS will become a nightmare as the HTML attributes output by Drupal will almost exclusively consist of 32-digit hashes.

    2. Remove "disabled" functionality.

    Considering how horrendous the DX for both "back end" and "front end" developers would be in a world of UUID-hash-only markup, I think this example would be some evidence of the "broken beyond repair" claim for disabled modules?

    catch’s picture

    So David already tried to answer this in #369. The argument is that we're going to have to prevent uninstall of modules with 'soft dependencies', and if we did that, the same validation could be applied to disabling modules too.

    That way a module with a 'soft dependency' in the system somewhere can never be disabled, and it can only be uninstalled if it takes the dependent configuration with it. If you have a disabled module, then you'd need to enable it first before uninstalling - i.e. we'd never allow the situation where a module is first disabled then uninstalled from that state.

    If we implemented everything that way, I don't see that the code would end up looking any different from the current patch - assuming we put a UI for disabling modules back into core.

    If a module has no soft-dependencies anywhere, then it's also got no need for hook_disable() or hook_enable(), and nor do other modules have a need for hook_modules_disabled() or hook_modules_enabled(). All of that logic that tries to find references and clean them up or put them back would be handled by validation, or can just move to hook_install() / hook_uninstall().

    alexpott’s picture

    So this issue removes the ability to disable modules and makes everyone go from installed to uninstalled. The other solution #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled proposes a similar solution. The major difference is that is does not remove disabled modules and (atm) will re-enable during an uninstall (I don't think that is tenable as there is just too much that can go wrong).

    This patch has several critical followups including #2081873: Re-implement module disabling in a temporary/debugging capacity that is environment aware and explicit about risks to data integrity which will restore the disabling ability but in a way that will help us prevent the problems described here - and it will have to add a sensible UI and DX.

    My essential point is that there is a general consensus between @David_Rothstein and those that want this patch to be committed. Modules should be enabled as a prerequisite to uninstallation. The only discussion is how to get there. This issue is ready - unblocks CMI work, and will enable has to remove swathes of difficult to understand code in the field module.

    I think that committing this patch offers us the quickest way to the best solutions.

    fubhy’s picture

    Status:Needs review» Reviewed & tested by the community

    Based on the last comments I am setting this back to RTBC.

    Dries’s picture

    I'll be reviewing this patch today.

    Dries’s picture

    I just finished reviewing this patch and it looks good to me. I'm glad we arrived at some sort of middle ground. However, I tried to apply it but it looks like it needs one last re-roll. Given a re-roll, I'd try to commit it today.

    webchick’s picture

    Status:Reviewed & tested by the community» Needs work

    Needs work for that.

    Dries’s picture

    The last submitted patch, 1199946-372.patch, failed testing.

    alexpott’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new192.77 KB
    PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
    [ View ]
    David_Rothstein’s picture

    I intend to respond in more detail to #373/#374 but don't have time right now. I think the way it distinguishes between machine names and IDs is a very useful way to think about the different issues here regarding data/configuration (much better than the distinction I tried to come up with in #362.2/#362.3). But I don't think it quite gets the details or consequences correct.

    Speaking only for myself (I have no idea about all the other people who have raised concerns with this patch but have gone recently quiet) the only thing I see consensus on is that Drupal should allow modules to go from enabled directly to uninstalled, and that the user interface for disabling modules needs to change. But the patch here instead rips out the entire concept of disabled modules in a way that would be very, very difficult to ever add back. It pretty much assumes that the only accetable UI for disabling modules is going to be something like #2081873: Re-implement module disabling in a temporary/debugging capacity that is environment aware and explicit about risks to data integrity (with the limitations inherent therein) and I don't think that would be a very useful UI. Meanwhile, the patch I posted at #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled has none of the controversy of this one and appears to unblock all the same issues as this one. It is an interim step which will require follow up UI changes (just like the patch here will) but it leaves a much wider range of possibilities open for what that UI will look like, and does not assume that the disabled modules feature needs to be destroyed to do it.

    Finally, based on hours of research documented above, I believe I've shown that "disabled modules are broken beyond repair" is an inaccurate description of the current situation.

    I don't support this patch being committed.

    effulgentsia’s picture

    @David_Rothstein: in #369, you said:

    If you try to "turn off" a regular module, your choices would normally be (a) uninstall, or (b) disable. If you try to "turn off" module B in the above example, the choices would change to (a) uninstall and let Drupal automatically remove the dependent configuration for you, or (b) abort and leave it enabled. So you'd never be allowed to get in a situation where module B is disabled while module A still has module B's configuration stored within it.

    1. Whether a module that's an island and has no soft data/config dependencies to or from any other module can be called "regular" is perhaps debatable: it might be a minority, but we can try to gather that data by looking at some core and popular contrib modules.
    2. Regardless of how common or edge case that kind of module is, in #374, @catch clarified that this means there is no need for the enabled/disabled hooks. You said you don't have time now to respond to #373 and #374, but can you quickly respond as to whether you dispute just that statement?
    3. If there's agreement on the above point, then it seems to me the only disagreement is on UI, and that #382 doesn't remove any concept that would be hard to restore (from an API standpoint) in #2081873: Re-implement module disabling in a temporary/debugging capacity that is environment aware and explicit about risks to data integrity.

    Meanwhile, the patch I posted at #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled has none of the controversy of this one and appears to unblock all the same issues as this one.

    According to #375, that patch doesn't solve the problem of data integrity loss occurring while a module is in its disabled state; and such loss can/will create problems for the blocked issues.

    catch’s picture

    Re-enabling modules during the same request as #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled currently does adds yet more complexity and fragility to the current process rather than simplifying it. While the end result of that patch may end up similar in terms of requiring a manual re-enable prior to uninstall it's very far from the simplification allowed here.

    'Broken beyond repair' remains an accurate assessment of the situation with disabled modules, if having a tiny remnant of the current support is remaining, perhaps 'unfit for purpose' is better. But that's playing semantics with the issue title.

    Where we've ended up is that you can temporarily disable a module, if no configuration owned by other modules in the system has any dependencies on it (soft dependencies), only temporarily, and must re-enable it before uninstalling or upgrading. And that additionally we need to prevent uninstall in the case of soft dependencies as well. This is unrecognisable from what we currently allow which is a complete free-for-all with all of the repercussions discussed at length here, and the many, many hundreds if not thousands of volunteer hours wasted.

    If you have a tower block and it's about to fall over with people living in it, perhaps you could put enough scaffolding up and dismantle it piece by piece until it's eventually replaced with a bungalow, relocating people as parts are removed. But it could take years and there's the risk of it toppling or large pieces falling off during any time during that process.

    Demolishing it and starting again is a much safer option.

    beejeebus’s picture

    for the record, i'm completely unconvinced by David. i am, however, impressed with the patient, thorough and logical presentation of his arguments.

    i support this patch landing. so, committers - please, either way, can we move on?

    David_Rothstein’s picture

    2. Regardless of how common or edge case that kind of module is, in #374, @catch clarified that this means there is no need for the enabled/disabled hooks. You said you don't have time now to respond to #373 and #374, but can you quickly respond as to whether you dispute just that statement?

    OK, a quick brain dump - with the caveat that this is not fully thought through or verified.

    Yes, I dispute it. Let's use real examples for clarity and take "data" = "serial IDs" and "configuration = machine names".

    1. Data

    The Comment module stores node IDs and users IDs. When it's disabled those records can go stale. But because they are serial IDs, the scenario in #373 can't happen. All the bugs you can run into with these stale records are of the "isset and empty check" variety. And disabled modules are not the only way to run into them. (Example: #1281114: Database records not deleted for Term Reference Fields after Term is Deleted which has nothing to do with disabled modules and which is not likely to have a comprehensive solution any time soon. Referential integrity is hard.)

    So I don't see any reason to prevent Comment module from being disabled; hence the comment_enable() hook implementation is still needed.

    2. Configuration (plugin disabled)

    Some module provides a views handler, and the module is disabled or uninstalled. That is what #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall deals with, and the likely result for disabled modules would be the module is prevented from being disabled. (That's the zero extra work solution, at any rate.)

    I don't think this is a huge number of modules, nor the kinds of modules people tend to disable in real life.

    3. Configuration (containing module disabled)

    Still using Views, but this time Views itself is disabled. The configuration in the view goes stale, possibly in bad ways (scenario in #373).

    A lot of examples I could think of don't have real problems here (for example if you have an image style called 'thumbnail', delete it and recreate a new one also called 'thumbnail', then reenable the Views module and it's referencing the new version even though it was supposed to be referencing the old version and therefore shouldn't reference anything any more... who cares?)

    One example with problems could be fields; if a field is deleted and a new one created with the same name but a completely different field type. My thoughts are:
    (a) What happens right now in this scenario, even if the Views module is enabled the whole time? Does it actually have code to deal with this?
    (b) The combination of events needed to trigger this seems like an extreme edge case...
    (c) And if #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall needs to add modules that store lots of other modules' configuration (like Views) to the "cannot be disabled" list, I doubt anyone would care. These modules tend to be fundamental to a site's structure so no one would want to disable them anyway.

    Also:

    Where we've ended up is that you can temporarily disable a module... and must re-enable it before uninstalling or upgrading

    I don't see any reason why you should have to re-enable it before upgrading. Hence there's no reason to restrict it to "temporary" disabling.

    catch’s picture

    Comment module has field instances that depend on it (i.e. on the comment bundles/comment entity type), so it is exactly the sort of module that would not be allowed to be disabled per #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall.

    Any module that's dealing with entity types and fields is likely to be in the same position. So while comment_enable() solves that very limited case, I can't think of a module that would only run into that case.

    Looking at the latest patch on #731724: Convert comment settings into a field to make them work with CMI and non-node entities you can see that when the comment/node relationship is modernized to use fields/instances, comment_enable() is now depending on a lot of information that could go stale were it to be disabled.

    alexpott’s picture

    thedavidmeister’s picture

    1. Data

    The Comment module stores node IDs and users IDs. When it's disabled those records can go stale. But because they are serial IDs, the scenario in #373 can't happen. All the bugs you can run into with these stale records are of the "isset and empty check" variety. And disabled modules are not the only way to run into them. (Example: #1281114: Database records not deleted for Term Reference Fields after Term is Deleted which has nothing to do with disabled modules and which is not likely to have a comprehensive solution any time soon. Referential integrity is hard.)

    So I don't see any reason to prevent Comment module from being disabled; hence the comment_enable() hook implementation is still needed.

    1. Module A has some data with a serial ID of 1
    2. Module B references the data 1 of Module A
    3. Module B is disabled
    4. Module A is disabled and uninstalled - Module B's data is now stale
    5. Module A is re-installed
    5. Module A creates *new* data with serial ID of 1 - Module B's data is now stale and irreparable.
    6. Module B is re-enabled and has absolutely no way of knowing/validating that the 1 its stale data references is the "wrong" 1. An isset/empty check is far from sufficient here.
    7. Unfortunately, Module B's data associated with its reference to 1 is not compatible with the Module A's data associated with the ID 1 - Fatal errors are highly likely at this point and potentially rather nasty to debug.

    thedavidmeister’s picture

    I don't think this is a huge number of modules, nor the kinds of modules people tend to disable in real life.

    What? The docs for upgrading from D6 to D7 say disable *all* contrib modules while performing upgrades. There's at least one "real life" example for disabling these modules.

    thedavidmeister’s picture

    3. Configuration (containing module disabled)

    Still using Views, but this time Views itself is disabled. The configuration in the view goes stale, possibly in bad ways (scenario in #373).

    A lot of examples I could think of don't have real problems here (for example if you have an image style called 'thumbnail', delete it and recreate a new one also called 'thumbnail', then reenable the Views module and it's referencing the new version even though it was supposed to be referencing the old version and therefore shouldn't reference anything any more... who cares?)

    One example with problems could be fields; if a field is deleted and a new one created with the same name but a completely different field type. My thoughts are:
    (a) What happens right now in this scenario, even if the Views module is enabled the whole time? Does it actually have code to deal with this?
    (b) The combination of events needed to trigger this seems like an extreme edge case...
    (c) And if #2080823: Disallow module uninstall if there are configurables that depend on that module needs to add modules that store lots of other modules' configuration (like Views) to the "cannot be disabled" list, I doubt anyone would care. These modules tend to be fundamental to a site's structure so no one would want to disable them anyway.

    I don't know what Views does exactly for this use case, but it's kind of irrelevant. It's clear (or can be made clear if we provide official docs) to even beginner developers what an enabled module *should* do with its internal references if the module providing data that it references is uninstalled. It's *not* clear to some of the most advanced developers in our community what a "disabled" module *should* (or even, should be allowed to) do when a module with data it references is disabled or uninstalled.

    I disagree that the examples I'm providing represent extreme edge cases, to me they represent development workflows that seem to be more or less the exact use-cases disabling was invented to accommodate in the first place. As has been pointed out, disabling/uninstalling is potato/potato for "island modules", so it's the *more* complicated/intertwined modules that provide us with use cases for wanting to be able to disable/enable without changing our data. To me, when you say that my examples are "extreme edge cases", what I hear is "the use cases for wanting to disable modules are extreme edge cases". You may disagree with that, but at the end of the day, playing the "how edge case is this, really?" game at all will just stir up a whole lot of unsubstantiated, untestable opinions (of which we have more than enough in this thread already) and achieve very little in the way of surfacing new factual information or real progress towards an RTBC for this patch or any other. I personally believe that this thought experiment style risk analysis is not how we should ever, ever be discussing data loss/corruption caused directly, easily and reproducibly by fundamental parts of the Drupal API, especially when achievable without warning, through the UI.

    If we are going ahead with thought experiments, let's use examples where the corrupt configuration is not a bung image style but involves accidentally divulging user's personal information to the whole world, unexpected and difficult to remedy site downtime/fatal errors and incorrect behaviour of eCommerce stores caused simply by re-enabling a module that "was working yesterday!".

    catch’s picture

    Status:Needs review» Reviewed & tested by the community

    #382 passed the bot, moving back to RTBC.

    thedavidmeister's examples are very good, as is the discussion on edge cases.

    webchick’s picture

    Title:Disabled modules are broken beyond repair so the "disable" functionality needs to be removed» Disabled modules are broken beyond repair so the \"disable\" functionality needs to be removed
    Status:Reviewed & tested by the community» Needs work
    Issue tags:+Needs reroll

    Patch no longer applies.

    hass’s picture

    Bringing one more issue up. If I cannot remove a module directory as it caused a fatal. How can I recover from fatals in the module code if I can no longer access admin section. Are we ending with a permanent defect website for ever? In past I removed this module folder.

    Don't say I should better test... Autoupdate will do this.

    As there is no design issue in "disable" we can won't fix. No need to waste time with reroles.

    thedavidmeister’s picture

    Bringing one more issue up. If I cannot remove a module directory as it caused a fatal. How can I recover from fatals in the module code if I can no longer access admin section. Are we ending with a permanent defect website for ever? In past I removed this module folder.

    Generally one would use a tool such as registry_rebuild to recover from fatals caused by changes to the directory structure - https://drupal.org/project/registry_rebuild

    As there is no design issue in "disable" we can won't fix. No need to waste time with reroles.

    What? this is untrue. Please see #373 and #390 for examples.

    fubhy’s picture

    @hass Please, can you stop trolling this issue? The decision we are making here is hard enough as it is and does not need any emotional FUD thrown into the mix.

    As there is no design issue in "disable" we can won't fix. No need to waste time with reroles.

    That is simply BS and I am pretty sure you know that. Please consider how much time we've already spent on this (and all related) issues and how frustrating your comments are to the people who are actually working on this issue and being productive.

    If you count removing the module directory as a sane way to disable a module then it's already pretty obvious that there is no person in the world that can possibly convince you about the stance me and many others have taken on this issue or even discuss it in a productive manner. So please let us do our work here without making it even harder on us. Thanks.

    hass’s picture

    Title:Disabled modules are broken beyond repair so the \"disable\" functionality needs to be removed» Disabled modules are broken beyond repair so the "disable" functionality needs to be removed

    @fubhy: Don't shoot the messenger, please. Just because I'm showing some upcoming issues that we *will* run into without being able to disable modules, don't make anything better. We cannot uninstall a module, if users are running into a module fatal error. This can prevent access to admin pages, too. Normally I ran update.php and clear caches/registry. Someone said; if I remove the module folder I will run into a crash condition and can only recover by adding the broken module back. But this is not possible as this one is broken, too. Then we are in an "endless" loop of failures.

    In past I just removed the module directory to get rid of the issues (this is also true for development environment where I'm testing). Should I always install a site from scratch in such a case? I do not think so. Running SQL statements to recover sounds not like an option.

    As David noted in #362 and followups that disable function has no general design flaws and his examples really proves this. The issues discussed here will also happen in other cases (e.g bugs) that a module need to recover from, too. #390 can be solved by re-enable and running uninstall hooks of all depended modules on uninstall of the "master" module. This could also be done in the background without user intervention or we say an uninstall hook must be safe to run in disable mode, too. We also have this situation with upgrade hooks. If they need the modules code, they must include the modules file. I know some developers ignored this, but this is a known rule. Therefore we do not need to re-enable the depended modules. Just run their uninstall hooks - disabled or not, doesn't matter here.

    However I see the issue behind #390 I'm strongly against removing this feature if we can solve this edge case issues in other ways as written above (e.g. make hook_uninstall - disable safe).

    thedavidmeister’s picture

    I believe the design flaw of "disabled" modules that I'm providing examples can be summarised:

    The "correct" behaviour of module A that owns data that references another module B's data is *undefined* while module A is disabled and module B either changes state from/to enabled, disabled or uninstalled or module B performs CRUD operations on any data that module A references. It is undefined because the *definition* of a module being disabled means that it cannot run any code that would be normally be necessary to maintain its data integrity, were it enabled. It follows logically that disabling a module that owns any data that references data it does not own *by definition* enables scenarios that can lead to irreparable data loss and/or corruption.

    The only way to prove there is no design flaw in disabling modules is to explain, in terms that we can all agree upon and document clearly in a way that works for *all core, contrib and custom modules*, how module A should behave for each state change of module B while A is disabled.

    Usually, when I have fatal errors in modules so bad I can't even use drush to uninstall, I don't just rip the module out of the file system, I fix the fatal error. But maybe that's just me...

    hass’s picture

    Usually, when I have fatal errors in modules so bad I can't even use drush to uninstall, I don't just rip the module out of the file system, I fix the fatal error. But maybe that's just me...

    Well, but only if you are a developer. We also have a LOT (if not the majority?) of endusers who cannot write code nor debug.

    thedavidmeister’s picture

    Issue summary:View changes

    Updated issue summary.

    thedavidmeister’s picture

    Someone said; if I remove the module folder I will run into a crash condition and can only recover by adding the broken module back. But this is not possible as this one is broken, too. Then we are in an "endless" loop of failures.

    @hass, you just invented this problem out of thin air. We include module files, not require them so removing files from your file system doesn't automatically lead to crashes, before or after this patch is applied - it obviously *could*, but no more than any other PHP-based-system on the planet would react if you add/remove files that the system expects to be able to access.

    This situation already exists in D7, you can easily cause fatals that stop you from being able to enable/disable modules. It doesn't permanently break a site, at absolute worst it means you need to find a local developer to do a little fix-up, probably 15-90 minutes of work in the vast majority of cases, or you can just call your hosting company and ask them to restore last night's backup and everything will most likely work again immediately.

    What you're really looking for is not "disabled" per-se, but a formal "missing" state - which is something that only "sort of" exists now, but likely would be beneficial to expand upon. This is something makes sense for core to implement because the behaviour of "missing" modules is not ambiguous - they're broken and developers should be notified that they have left cruft in the database that needs to be handled carefully. Discussing this any further is also something *well outside* the scope of this issue - open a new issue if you want to further discuss missing modules in the file system that are registered in the system table.

    To be clear, removing files from the file system does not disable modules, it makes them "missing", which may or may not behave like "disabled" depending on the module and your site, if it behaves like "disabled", well, that's a best-case scenario and has more to do with luck than anything planned. It's insane/negligent to try and do this deliberately on a production site as a debugging method. Core can neither encourage nor support it as supporting arbitrarily missing code is impossible.

    #390 can be solved by re-enable and running uninstall hooks of all depended modules on uninstall of the "master" module. This could also be done in the background without user intervention or we say an uninstall hook must be safe to run in disable mode, too. We also have this situation with upgrade hooks. If they need the modules code, they must include the modules file. I know some developers ignored this, but this is a known rule. Therefore we do not need to re-enable the depended modules. Just run their uninstall hooks - disabled or not, doesn't matter here.

    However I see the issue behind #390 I'm strongly against removing this feature if we can solve this edge case issues in other ways as written above (e.g. make hook_uninstall - disable safe).

    @hass, what you're proposing as a way to "fix" #390 is a "hand waving" fairy-tale because Core has no way of knowing about dependencies in the *data* of modules caused by relationships. We know about functional dependencies, sure, these are static and explicitly stated as properties of the module itself, but we have no way of knowing which modules to enable when a module is being uninstalled in order to keep *data relationships* intact and blindly uninstalling modules that list functional dependencies is just as destructive, if not more so, as the situation we're trying to avoid.

    One example of a problem your suggestion introduces is that a functional dependency doesn't tell us what *direction* a data relationship is. If module B depends on modules A and C, and B and C are disabled, with your suggestion, to uninstall A we have to enable and uninstall B which then *enables and uninstalls C!*. This means that uninstalling A can uninstall C that may have absolutely nothing to do with A, the dependency listed by B on C may *not* have anything to do with data, in reality and may simply be using C's API. So now we nuke C's data because A was uninstalled, simply because B had multiple functional dependencies but no actual relationships in the data between B and C. What if C was uninstalled at some point and this is why B was disabled? does this mean that we cannot uninstall module A until C is re-installed? now we have dependency chains that can travel in two directions, rather than simple "parent/child" relationships. This idea of a "master" module is contrived to try and prove a point but is an oversimplification of what is the reality for many sites - that relationships between data forms *networks* of interconnected modules.

    @hass, your suggested "fix" for #390 looks good at first glance, but is more broken than the problem it tries to solve!

    Neither #373 nor #390 have had a solution proposed by anyone to date that works 100% of the time without causing new problems we didn't have previously.

    What *has* been proposed as a serious attempt to re-introduce "disabled" functionality is a validation step, so when you try to disable a module every other module can say "yes" or "no" as to whether to allow the disabling or not. If any module says "no" then the module cannot be disabled.

    The issue I see with this proposal is that already-disabled modules cannot "veto" (because they're disabled) and so the validation process itself becomes less reliable each time you disable a module. The only way to avoid this would be to prevent modules that want to have veto rights from being disabled, essentially leading us back to the conclusion that we already came to a long time ago, which is that "island modules" are the only modules that are safe to disable and those modules are also the easiest to uninstall/reinstall - therefore the benefits of being able to disable modules are minimal but we have to build and maintain a whole new and complex validation system to support the modules that actually don't use the validation system - we also foist the responsibility of data integrity onto contrib instead of core... Drupal has to trust that contrib modules can use the disable_validate hooks correctly, otherwise Core might break...

    Additionally, there are DX issues around this working in the "real world".. a module might say "no" without giving a reason (or the reason might be extremely cryptic), so it might actually be really hard to figure out how to get a veto to say "yes".

    Regardless, the patch does not actually prevent this hypothetical disable-with-validation world being implemented, but it does force any "disabled" functionality that is re-introduced into core to be carefully thought out and reviewed with the benefit of years of hindsight - which can only be a good thing IMO.

    pounard’s picture

    One example of a problem your suggestion introduces is that a functional dependency doesn't tell us what *direction* a data relationship is. If module B depends on modules A and C and B and C are disabled, with your suggestion, to uninstall A we have to enable and uninstall B which then *enables and uninstalls C!*. This means that uninstalling A can uninstall C that may have absolutely nothing to do with A, the dependency listed by B on C may *not* have anything to do with data, in reality and may simply be using C's API. So now we nuke C's data because A was uninstalled, simply because B had multiple functional dependencies but no actual relationships in the data between B and C. What if C is missing? does this mean that we cannot uninstall module A because module C is not installed? now we have dependency chains that can travel in two directions, rather than simple "parent/child" relationships. This whole idea of a "master" module is an oversimplification of what is in reality for many sites a *network* of relationships between the data of interconnected modules.

    This seems like totally non comprehensive explaination, I had to read it 4 times to understand half of it.

    Totally not connected, I do understand why disabled modules can be dangerous for a site state and bring a lot of complexity in module handling, make it hard to resolve dependencies across different states, etc etc etc... The code will gain in maintainability and readability and less complixity removing the disabled state. Even if I do not agree with this change, I do understand why it happens and I respect that.

    But I still don't approve at all that stalled data is the reason of this issue. IMHO any module that can be installed at any point in time should also be able to be enabled at any point in time: that's not core due task but the module itself that should always ensure its data consitency when enabled, event if the cost is leaving everything that has been missed during disabled time in a default state. That's a huge problem of both using an heavily abstracted entity model without ever using any data integrity constraints capacities of the storage backend and wanting to do business stuff with it at the same time: whatever you do with your modules I think that reaching a "safe" state in a Drupal site is just not possible, ever (and even less now that backends begin to be heavily decoupled) because it's a CMS and it never has been meant to keep your data consistent. You just can't have your cake and eat it too.

    thedavidmeister’s picture

    This seems like totally non comprehensive explaination, I had to read it 4 times to understand half of it.

    Sorry. I'll try to be clearer.

    Say Module B declares that it "depends" on Module A, that is, Module A must be enabled before we can enable Module B.

    This could be because (for example):

    - Module B references data in Module A directly
    - Module B uses an API that Module A exposes in a way that Module A references data in Module B

    This means that if a module declares a dependency, and we want to try the approach of enabling disabled modules with declared dependencies so we can uninstall them together, we have to assume that references could go either direction, either pointing "to" a dependent module or "away from" one. Of course, references could be bi-directional and may not even relate to the same data but it doesn't change the consequences, so let's keep the example as simple as possible for the sake of clear discussion.

    This means if you have 3 modules, A, B and C:

    - Module A depends on nothing
    - Module B depends on A and C
    - Module C depends on nothing

    Module A and C are unrelated, let's say for illustrative purposes Module A does a bunch of stuff with some "content" data and Module C provides some authentication method, mapping users in Drupal to external users in some 3rd party system - totally different things.

    Now we uninstall Module A, under the proposed system where we enable all dependencies (in either direction), we have to enable Module B to uninstall Module B and A together.

    Now we have to enable and uninstall Module C because it has a dependency in at least one direction between B and C and B is being uninstalled.

    Now, because we uninstalled Module A, that uninstallation "propagated" to B and then to C - this approach is too destructive for many situations.

    You can't assume that the "functional dependencies" between modules as stated in their info files, or detected through plugins/config actually represents the full scope of all relationships between modules - core doesn't know how modules handle their data, only modules know how they handle their own data and what queries they're running internally. Making it Core's responsibility to enable every module it thinks might need to be uninstalled based on information we *know* is incomplete is asking for trouble.

    The only way Core can be sure that all other modules are able to ensure their data integrity when Module A is uninstalled is to ensure that no modules can ever be uninstalled while at least one module is disabled - we have to enable *all* modules just before uninstallation of *any* module.

    thedavidmeister’s picture

    But I still don't approve at all that stalled data is the reason of this issue. IMHO any module that can be installed at any point in time should also be able to be enabled at any point in time: that's not core due task but the module itself that should always ensure its data consitency when enabled, event if the cost is leaving everything that has been missed during disabled time in a default state.

    Examples #373 and #390 show that it may be impossible for a re-enabled module to detect that it "missed something" while it was disabled. You could run any validation check in the world (other than external automated tests specific to the expected behaviour of your configuration) and if you're not using UUIDs for your references you may never be able to detect the difference between the data you think you're referencing and new, different data that you're now actually referencing post-re-enable.

    #399 outlines a clear, unanswered statement that needs to be countered specifically and explicitly if we are to claim that "disabled modules" are definitely not broken.

    If you're going to state that you have an opinion that modules should always ensure their data consistency when enabled, please explain with at least one working example how a module should do this:

    A. How a re-enabled module can tell the difference between 'foo' and 'foo' or 1 and 1, long after the moment the original 'foo' and 1 were deleted.
    B. In cases where it is literally impossible for a module to *guarantee* data integrity when re-enabled, how should a disabled module behave when other modules are changing state or performing CRUD operations that the disabled module has to react to in order to prevent future data corruption?
    C. If the answer to B is anything other than "do nothing" or "uninstall itself", how is "disabled" different to "enabled"?

    Please do not defer providing the working example to "contrib", "core", "the community", "the future", "someone else", "edge case", just present a quick, dot-point overview of the steps a module should take to ensure data integrity, just enough so that others can discuss details of the plan in a constructive light.

    pounard’s picture

    Thanks for taking the to explain in #403, it is clearer.

    The thing that I didn't understand is what system in the world would attempt to enable dependencies in either direction?

    I'm not really sure were you're going, I just think that:
    1. when you disable the B module, you just disable this single one
    2. when you disable A or C, you have to disable B too
    3. when you uninstall any module, you just disable the depedents but once everyone is disabled there is no interaction possible and keeping just pieces of data is no big deal
    4. uninstall should probably not be propagated, ever. On an operating system, if I use a package manager and uninstall (equivalent to disable for Drupal) some package, let's say A, it will uninstall the B package, but it won't erase any data, it will keep user files, configuration files, no matter how hard it may break anything
    5. If I "purge" (equivalent to uninstall for Drupal) any package, it won't ever erase any other data than those of the package I just purged, even the package had dependents. Admin will know if he need to drop the data, not the OS, the best educated guess Drupal can do here is that Drupal cannot know what thought the site builder, and cannot predict his errors, cannot fix it and will probably fail if it tries.

    To answer to #404 (it's fun, you have both 403 and 404 comments, I think you are either lost or forbidden ahah) I just want to ask a question, what is the difference between enabling a module the first time, and enabling it once again? In both cases, my schema is here (installed at install time) but in the second, there might be some data in database. There is two cases here:
    1. The missing data that is the problem, not the stalled data, case in which the module in both cases (enable and install) will only have to repeat the exact same operation to achieve a stable state (i.e. populating missing values, batch? aggregate insert or update operations?)
    2. The stalled data is the problem, which I may split into two categorie:
    1.1. Data is volatile and non important (statistics), who cares? Just resume operations.
    1.2. Data is important, and linked to some serious business stuff, two cases:
    1.2.1. Whatever it is (a point system? a virtual currency for users? critical, but we did not loose data, no biggie
    1.2.2. Some other related data happened and should modify our results, and it is critical, we're fucked, but the bad idea was to do anything critical with Drupal, not disabling the module. In most case when you reach this state, it's because you have a lot of business stuff happening. I'd say that when reaching this point, you just don't give the power to your end user to disable your module (for exemple, make it required, just as node is in Drupal 7) and that's fine

    Core cannot and will never be able to predict the usage that we may do of it, and responsability goes with module development when we're dealing with business stuff, not with core. Core can't, and I surely really I hope it won't ever try to guess what I am going to code for the next client stuff, because it will surely try to do something generic, slow, stupid and way more catastrophic than just inactivating my module temporarily.

    pwolanin’s picture

    in #227 and perhaps elsewhere a "backup and restore" model or a system for archiving data was suggested. I've created a follow-up to consider adding a UI option to archive config and data at the point when modules are uninstalled #2091577: Add an option to archive config and/or SQL data when uninstalling modules. It would still be one-way, but there are edge cases where keeping the config and/or data might make sense.

    Regardless, I think this patch should go in.

    alexpott’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new199.67 KB
    PASSED: [[SimpleTest]]: [MySQL] 58,266 pass(es).
    [ View ]

    Here is the latest patch from the helper issue. After #2082117: Install default config only when the owner and provider modules are both enabled landed Drupal\system\Tests\Upgrade\ModulesDisabledUpgradePathTest got an interesting error related to #2091615: Installation profiles provide default config that is not only used during installation. After working out why this was so I discovered that previous patches on this issue approach to disabled modules in Drupal 7 to Drupal 8 upgrades was to enable them all. This can not be the correct approach.

    So firstly this means that the instructions in UPGRADE.txt need a complete rewrite. However reading these instructions I feel that the whole approach to upgrades needs careful consideration. If we want to commit this patch to unblock and simplify further work in CMI and dependencies then we need to punt this to a followup. On the other hand when thinking about the upgrade path it is good to bear in mind comments like #194310: Check / run updates of disabled modules which notes that we could have data left over from versions previous to D7 - the mind boggles as to the effect this could have - and we can not just make the assumption that people by that time will have started over if they followed the instructions in UPGRADE.txt to the letter.

    interdiff impossible due to all the rerolls but have a look at #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair to see what's changed in the Drupal\system\Tests\Upgrade\ModulesDisabledUpgradePathTest

    catch’s picture

    Status:Needs review» Reviewed & tested by the community

    I followed the helper issue and discussed this in depth with Alex. RTBC again pending the bot.

    alexpott’s picture

    Status:Reviewed & tested by the community» Needs review

    The patches approach to major upgrades uses hook_requirements to bar upgrades if the D7 site has any modules are disabled. It tells the user which modules are disabled and gives them the following message:

    Drupal 8 no longer supports disabled modules. Please either enable or uninstall them before upgrading.

    fubhy’s picture

    Status:Needs review» Reviewed & tested by the community

    That was a cross-post I think.

    David_Rothstein’s picture

    Note that the scenario in #390 can never happen with the example I brought up in #387, since the Comment module has a dependency on the Node module. You cannot uninstall Node without uninstalling Comment first, so the problem described there cannot occur.

    So that's my general answer to the first part of this:

    The "correct" behaviour of module A that owns data that references another module B's data is *undefined* while module A is disabled and [1] module B either changes state from/to enabled, disabled or uninstalled or [2] module B performs CRUD operations on any data that module A references.
    ....
    The only way to prove there is no design flaw in disabling modules is to explain, in terms that we can all agree upon and document clearly in a way that works for *all core, contrib and custom modules*, how module A should behave for each state change of module B while A is disabled.

    If module A is making heavy use of module B's data, it should declare a dependency on module B. In most cases this is easy and already done. For cases where it's not (Entity Reference comes to mind?) then I think the problem is more that we don't have a convenient way for modules to declare dynamic dependencies. It's a problem with the dependency system, not with disabled modules.

    For the second part (CRUD operations) I think I already brought up above that the problem isn't limited to disabled modules, with #1281114: Database records not deleted for Term Reference Fields after Term is Deleted as an example.

    ...edge cases...

    I think this is important to restate. This issue is still claiming that disabled modules are "broken beyond repair", but at this point, the only remaining arguments for that are based on scenarios that as far as I can tell, have never actually been reported in the Drupal issue queue as having happened to an actual website. To me that's the definition of an edge case. (And even for those, I think the comments above have addressed many of them.)

    Meanwhile, the ability to disable modules is an important tool for site builders, and a feature that most other plugin-based software on the planet supports. Removing it based on such narrow reasons would be a big mistake.

    tstoeckler’s picture

    I think the problem is more that we don't have a convenient way for modules to declare dynamic dependencies. It's a problem with the dependency system, not with disabled modules.

    That's a very interesting take on this issue, I had not seen it in this way before. As this problem still leads to critical data loss I still think we should go ahead with this for now and only consider "Introduce dynamic dependencies" as a follow-up as I fear that might not get done for D8 or it might even turn out that it is not solvable in general in such a dynamic system as Drupal.

    catch’s picture

    Note that the scenario in #390 can never happen with the example I brought up in #387, since the Comment module has a dependency on the Node module. You cannot uninstall Node without uninstalling Comment first, so the problem described there cannot occur.

    In the specific case of comment yes, but I also pointed out that comment already has dynamic dependencies which would prevent it being disabled if we enforced those on module disable. So as an example of a module that can be safely disabled it doesn't work as a concrete case. It's also likely to move to 100% dynamic dependencies soon via reference fields.

    For cases where it's not (Entity Reference comes to mind?) then I think the problem is more that we don't have a convenient way for modules to declare dynamic dependencies. It's a problem with the dependency system, not with disabled modules.

    Yes that's the worst problem we have, and it's the subject of #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall.

    However the vast majority of problems with dynamic/soft dependencies are the interaction of those with the disabling of modules, since it's doing that which breaks the dynamic dependencies. When everything is enabled all the time it works OK with some exceptions (hence field_system_info_alter()).

    A large subset of those issues also affect uninstalling modules too. The issues that don't are those around preserving data so that the module can be re-enabled.

    In terms of the end point, I still don't think David is very far from those in favour of removing the feature and putting it back in later - more with the characterisation of the problem.

    To test this here's a rough outline of where I think we should be aiming to get to.

    1. Once a module is enabled, if it has dynamic dependencies in either direction, it can only be uninstalled or left enabled. This requires adding an API for modules to declare that it has a dynamic dependency on X module (i.e. field module would declare all modules providing entity types, bundles, field, widget and formatter types that are configured in any field or instance).

    2. To avoid a catch-22 where modules can never be uninstalled (which is what field_system_info_alter() does now), we might be able to automate the deletion of the dependent configuration (i.e. removing all views which depend on node if node is uninstalled) as part of the uninstall process - might require an explicit choice to do that on the part of the administrator to avoid nasty surprises.

    3. Modules that do not have dynamic dependencies in either direction can still be disabled. To uninstall the module, it needs to be re-enabled manually first so that APIs are available. #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled is David's alternative approach to this issue for #3. I don't think there's any difference between #1 and #2.

    That's very likely incomplete, but David do you disagree with those three points at all? Or is there a significant point of disagreement that's not covered by those in terms of the specification of where things should end up?

    thedavidmeister’s picture

    #412 - yeah... #373 and #390 seem unsolvable to me without a dynamic, infallible registry of every data dependency in the system. Static, functional dependencies aren't enough here.

    Whether it's feasible, or even possible, to build and maintain such a registry is unknown at this point but a framework for this definitely doesn't exist at the moment.

    thedavidmeister’s picture

    Another example of a problem with disabled modules. A data relationship registry doesn't help solve this.

    Module A has some API that other modules can implement, they can give a machine name to some data that A then uses to provide functionality. It's important that the machine names are unique within the system or Module A will start throwing fatal errors and/or behaving erratically.

    1. Module B implements A's API and names some data 'foo'
    2. Module B is disabled
    3. Module C implements A's API and names some data 'foo'
    4. Module B is re-enabled

    What is supposed to happen at step 4?

    If Module B was enabled then step 3 would fail validation for Module C and the data would need to be saved with a different name.

    If Module B was uninstalled, then there would be no conflict.

    A conflict exists because disabling Module B also disables any validation that would normally prevent conflicts with data that needs to be unique across the whole system.

    So, as well as stale references we have to deal with invisible namespace conflicts.

    This is different to importing data with the same name as something that exists - when we import we make a decision to either rename data that has conflicts *before* it is saved to the db, or treat both datas with a single name as a single item in our system (like how Features works) - here we simply have an unresolvable conflict. Renaming data that already exists in the database because of name conflicts is not really an option, as this would greatly exacerbate the stale references problem.

    thedavidmeister’s picture

    This requires adding an API for modules to declare that it has a dynamic dependency on X module

    Can we always keep track of this? we probably can only guarantee the integrity of our dynamic dependency registry for as long as we have 0 disabled modules in the system.

    Module A has a dynamic dependency on Module B whenever Module B is configured so that one of its settings 'foo' is TRUE:

    1. Module B has 'foo' set to FALSE
    2. Module A is disabled
    3. Module B has 'foo' set to TRUE

    What happens after step 3? Module A is disabled, so it can't "see" that Module B has changed 'foo' and even if it could, it couldn't then update the registry.

    Even if A could see that 'foo' has changed, what action can we realistically take? do we silently/forcibly re-enable Module A at that point? Module B has no idea that when its setting 'foo' becomes TRUE that Module A will re-enable, so we can't present any kind of warning to the end-user on the validation of the form that modifies 'foo'.

    pounard’s picture

    Those scenarios are going way too far... As soon as you introduce flexibility there will always be someone here to break what you've done. Just decide whether or not you want your modules to be disabled, and risk data stall, or not and just uninstall them, and risk data loss. In both cases they are pros and cons.

    David_Rothstein’s picture

    That's very likely incomplete, but David do you disagree with those three points at all? Or is there a significant point of disagreement that's not covered by those in terms of the specification of where things should end up?

    I don't think I disagree that those would be good changes; maybe my differences are more about the priority? I'm not sure this generalized dynamic dependency system will really come into being, so if the disabled modules functionality is removed and only allowed to be added back on the condition of that existing, it might never be added back. Also, trying to add back a big feature like this will be difficult once it's been removed.

    As this problem still leads to critical data loss I still think we should go ahead with this for now and only consider "Introduce dynamic dependencies" as a follow-up as I fear that might not get done for D8 or it might even turn out that it is not solvable in general in such a dynamic system as Drupal.

    I still don't see any real examples of data loss... But in any case, I should also point out that we don't necessarily need a dynamic dependency system to solve specific instances of this. For example, if the Entity Reference module wants to declare a dependency on the Node module whenever there are node reference fields in active use, I don't think it would be that hard to do so in hook_system_info_alter() currently - ugly, but not that hard.

    thedavidmeister’s picture

    Those scenarios are going way too far... As soon as you introduce flexibility there will always be someone here to break what you've done. Just decide whether or not you want your modules to be disabled, and risk data stall, or not and just uninstall them, and risk data loss. In both cases they are pros and cons.

    There are lots of things in Core that are extremely flexible but can't be broken by users using the UI in "normal" ways. It's not right that we throw our hands up in the air and say "oh, flexibility always means things are going to break so we always have to accept a degree of brokenness if we ever want flexibility", because that's demonstrably not true.

    Can I just take this opportunity to point out (again) that there is no indication in either the UI or our documentation that any data corruption either has occurred or may occur in the future. It's not exactly common knowledge within the community how damaging disabled modules can be (although, I'm sure anyone who's attempted a major upgrade of a sufficiently old and complex site has some idea...), so we can't say that the average Drupal developer is weighing up pro's and con's in an informed manner and taking a calculated risk whenever they disable modules.

    We moved the php input filter into contrib https://drupal.org/project/php because we weren't confident that everyone using that functionality was fully aware of the potential repercussions. Moving disabled modules functionality to contrib makes sense for all the same reasons.

    I don't think I disagree that those would be good changes; maybe my differences are more about the priority? I'm not sure this generalized dynamic dependency system will really come into being, so if the disabled modules functionality is removed and only allowed to be added back on the condition of that existing, it might never be added back. Also, trying to add back a big feature like this will be difficult once it's been removed.

    Yes, if this patch lands and disabling modules can't be put back into core in a way that we can all agree isn't broken, "disabling" will likely live in contrib for the foreseeable future.

    I still don't see any real examples of data loss...

    I think that's a fair point, we've been primarily talking about data corruption, but with the implication that the corrupt data can subsequently or immediately lead to broken functionality and/or data loss.

    But in any case, I should also point out that we don't necessarily need a dynamic dependency system to solve specific instances of this. For example, if the Entity Reference module wants to declare a dependency on the Node module whenever there are node reference fields in active use, I don't think it would be that hard to do so in hook_system_info_alter() currently - ugly, but not that hard.

    Without a patch that introduces enabled->uninstalled state changes, this would lead to circular dependencies in a lot of cases, not great :/

    Even if this weren't the case, I still think it beneficial to treat "data dependencies" and "functional dependencies" (or "soft" and "hard" dependencies) as two different concepts.

    pounard’s picture

    Without a patch that introduces enabled->uninstalled state changes, this would lead to circular dependencies in a lot of cases, giving us permanently un-disableable and then, un-uninstallable modules. Even if this weren't the case, I still think it beneficial to treat "data dependencies" and "functional dependencies" (or "soft" and "hard" dependencies) as two different concepts.

    Maybe what we need is just to have an intermediate state between normal module and required module which would be "cannot be disabled, uninstall only". Then let the module developer take care of putting this flag or not on his module. Core cannot prevent data corruption if data is handled by a module, so let the module decide, not core.

    thedavidmeister’s picture

    Maybe what we need is just to have an intermediate state between normal module and required module which would be "cannot be disabled, uninstall only". Then let the module developer take care of putting this flag or not on his module. Core cannot prevent data corruption if data is handled by a module, so let the module decide, not core.

    I believe this is something that has been suggested before, a few times. I think the general synopsis of that was:

    - This requires module developers to know whether any modules, now and in the future, will want to reference the module's data which may be impossible (although you could say, if in doubt, don't flag as "disable safe")
    - Where it's truly possible for a module to be confident that they're "disable safe" (the module has little or no data at all), the argument is that module will also be "uninstall safe" by definition, so disabling provides little extra benefit here. We don't want to have to maintain large, complex systems unless they can provide a lot of extra value for Drupal.
    - It's a common pattern in modern contrib modules to disable *parts* of their functionality, the "exportables" part of ctools provides a cookie-cutter UI for fine-grained disable functionality that can be implemented with about 100 lines of code that are largely copypasta - this approach is usually a lot more useful than a blanket kill-switch for the whole module anyway. Fatal errors aside, it's a much better debug tool than core's built-in "disabled" functionality and doesn't lead to data corruption because the enabled module can act as a "harness" for the data and make ensure its integrity, even if it considers the data disabled.

    So, disabled data is maybe good (modules know to do nothing with it, but it reserves an ID to avoid name conflicts and broken references), but disabled modules are bad?

    catch’s picture

    Yes exactly. Modules that are tricky to disable include relatively straightforward things like providing a field type plugin. There's no actual data provided at all, it's only when that field plugin is in use that it's linked with data.

    catch’s picture

    I'm not sure this generalized dynamic dependency system will really come into being

    Right now resolving that issue is a critical bug report - at least for config entities which is where the bulk of these issues now live. I don't have any plans to put out another release suffering from these issues.

    Dries’s picture

    Assigned:Dries» Unassigned
    Status:Reviewed & tested by the community» Fixed

    I just committed this patch to 8.x. I understand the push-back from David and various other site builders about losing this key feature. I agree that it is very important. But it's also important that we make progress on critical CMI and upgrade path bugs, and that we don't destroy people's data. I feel that it will be easier to bring this feature back in a way that prevents data loss if we can revisit it once some of these are completed. I'm certainly in support of bringing this back. Thanks everyone for the in-depth conversation. It's amazing to see what passion and rigor you all bring to the table.

    webchick’s picture

    Title:Disabled modules are broken beyond repair so the "disable" functionality needs to be removed» Change notice: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
    Priority:Critical» Major
    Status:Fixed» Active
    Issue tags:+Needs change record

    This'll definitely need a change notice.

    David, I really appreciate your tireless efforts of advocating for the site builder audience in this issue. I really hope you'd be willing to head over to #2081873: Re-implement module disabling in a temporary/debugging capacity that is environment aware and explicit about risks to data integrity and help work out a strategy for bringing this feature back in a way that doesn't cause data integrity issues.

    falcon03’s picture

    Title:Change notice: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed» Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
    Priority:Major» Critical
    Status:Active» Fixed
    Issue tags:-Needs change record

    First off, I strongly disagree with removing this feature.

    In any case, to mitigate the effects of its removal we should introduce a new API to backup data on uninstallation and then import it when the module is re-installed, if the user wants to. Does a (critical to me) issue to bring this kind of functionality in exist already?

    thedavidmeister’s picture

    Issue tags:+Needs change record

    I'm going to assume you didn't mean to remove this tag.

    thedavidmeister’s picture

    Title:Disabled modules are broken beyond repair so the "disable" functionality needs to be removed» Change notice: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
    catch’s picture

    Category:bug» task
    Priority:Critical» Major
    Status:Fixed» Active

    Looks like a cross-post on the status.

    I un-postponed the 8 or so critical issues that were postponed on this one.

    @falcon03 #1898794: Allow config entities to be exported. (Resolve regression from views in Drupal 7). is probably the first step towards that. I don't know of a more general issue yet but that's worth opening. However it does have the problem that when you backup date and restore it, you don't know if the site you're importing it into has moved on since it was exported. It's probably not going to be possible to export configuration in Drupal 8 then re-import it into a Drupal 9 site for example.

    YesCT’s picture

    Priority:Major» Critical

    I think change records are critical tasks anyway, and the after they are done, the previous settings are restored. (yep https://drupal.org/core-gates#documentation)

    change record instructions https://drupal.org/change-records

    pwolanin’s picture

    @falcon03: here's a follow-up to discuss archiving data: #2091577: Add an option to archive config and/or SQL data when uninstalling modules

    andypost’s picture

    Needs quick follow-up for broken HEAD https://qa.drupal.org/8.x-status

    dawehner’s picture

    Status:Active» Needs review
    StatusFileSize
    new947 bytes
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch disabled-fix.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]

    Here is a quick fix.

    dawehner’s picture

    StatusFileSize
    new1.85 KB
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch disabled-fix_0.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]

    Maybe this test coverage does not actually work but anyway let's fix that properly

    andypost’s picture

    Status:Needs review» Reviewed & tested by the community

    Manually tested the patch #434 shows that it works!

    webchick’s picture

    Priority:Critical» Major
    Status:Reviewed & tested by the community» Active

    Committed and pushed to 8.x. Thanks!

    Missing change notices are now "major" tasks, since we track a single critical in #2083415: [META] Write up all outstanding change notices before release. So restoring those properties.

    tim.plunkett’s picture

    Issue tags:-sprint, -Needs reroll

    Removing tags

    tim.plunkett’s picture

    Issue summary:View changes

    Updated issue summary.

    Cottser’s picture

    Issue summary:View changes

    Add issue to restore some disable functionality to follow-up issues

    catch’s picture

    Issue summary:View changes
    Issue tags:-beta blocker, -D8 upgrade path

    Removing tags.

    mradcliffe’s picture

    As a consequence of this patch, enabled modules that don't have a schema_version can never be uninstalled/disabled.

    Filed follow-up issue: #2137031: Some modules without schema_version cannot be uninstalled or removed from a Drupal 8 site.

    mradcliffe’s picture

    As a consequence of this patch, enabled modules that don't have a schema_version can never be uninstalled/disabled.

    Filed follow-up issue: #2137031: Some modules without schema_version cannot be uninstalled or removed from a Drupal 8 site.

    webchick’s picture

    webchick’s picture

    Issue summary:View changes
    drumm’s picture

    Issue summary:View changes

    Removing summary and redundant header.

    xjm’s picture

    Issue tags:+Missing change record

    Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

    The patch for this issue was committed on September 19, 2013. This means that the change record for this change has been missing for more than four months.

    xjm’s picture

    Edit: Wrong issue. :P

    Gábor Hojtsy’s picture

    Title:Change notice: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed» Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
    Priority:Major» Critical
    Status:Active» Fixed
    Issue tags:-Needs change record, -Missing change record

    Wrote a change notice for this at https://drupal.org/node/2193013 with the API changes explained (from the issue summary, lightly edited) and intro with my own words.

    Status:Fixed» Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.

    gokulnk’s picture

    Issue summary:View changes

    Pages