Helper issue for #1199946 : Disabled modules are broken beyond repair

The first problem I see here is that if we remove the 'disabled' state we must also remove 'enabled', thus keeping only installed/uninstalled states.

Well.. my problem was not with the yml files, but more like with their wording/meaning. I guess what I'm trying to say is that we need to rename module_enable() to module_install() and refer to everything as just 'installed' and 'uninstalled'.

This should have less fails. It's possible I removed to much code :). Needs to be reviewed very carefully but this is a starting point for the remaining failures.

I'm not 100% sure but I think this patch removes modules that depend on the uninstalled one automaticly? Correct?

If that's the case this shouldn't happen, or should it?

well HEAD doesn't work that way, cause there was a test that checked that which I had to remove to get green light from the bot :p

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.phpundefined
@@ -192,28 +192,6 @@ function testDependencyResolution() {
-    // Try to uninstall the PHP module by itself. This should be rejected,
-    // since the modules which it depends on need to be uninstalled first, and
-    // that is too destructive to perform automatically.
-    $result = module_uninstall(array('php'));
-    $this->assertFalse($result, 'Calling module_uninstall() on a module whose dependents are not uninstalled fails.');
-    foreach (array('forum', 'ban', 'php') as $module) {
-      $this->assertNotEqual(drupal_get_installed_schema_version($module), SCHEMA_UNINSTALLED, format_string('The @module module was not uninstalled.', array('@module' => $module)));

Here it is

I'm willing to help/take over. Talk to you tomorrow on IRC?

Started working on this today.

Following on from #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed, I do think it is important to leave this implementable in contrib, so we should definitely figure that part out. I had previously said that removing a module from the list in hook_module_implements_alter() would do it, but fubhy pointed out to me in IRC that that is no guarantee that code runs. However I'm trying to think of a case where a module has code that would run with all its hooks disabled and I'm not coming up with anything. The one thing I did think of is Domain which has special code in settings.php, but then again disabling Domain won't stop that code from running either so its not a parallel.

Regardless, a more graceful way of allowing contrib to add this functionality would be welcomed.

hook_module_implements_alter() won't disable classes from running - the module would have to be unregistered from the class loader as well. Then any plugins referenced in config are just going to completely disappear but that's what people want apparently.

Exactly, classes, the module's bundle, etc. are are all still there. So the only reliable way would be to prevent them from being introduced to the container and class loader. @see DrupalKernel::initializeContainer()

+++ b/core/includes/config.incundefined
@@ -54,6 +54,12 @@ function config_uninstall_default_config($type, $name) {
+
+  // If this module defines any ConfigEntity types, then delete the manifest
+  // file for each of them.
+  foreach (config_get_module_config_entities($name) as $entity_info) {
+    config('manifest.' . $entity_info['config_prefix'])->delete();

All the manifest stuff was removed as part of ... whatever issue it was where we made you require full config trees again, so you need to update from head :)

I realize it is early in this patch's life to be worrying about UI considerations but something I was thinking is that we may want to remove the checkboxes from the main modules page entirely, and leave the Uninstall tab in place. I think this could help alleviate the concerns about users unintentionally deleting data when they think they are disabling a module. We should also have some text on that page saying something like 'If you think you may want this module's data again, please backup your config directory located at
' Thoughts on this?

Yep. That is exactly what this Patch does.

Oh perfect! fubhy++

One thing with the uninstall tab is it only appears for modules with hook_schema() or hook_uninstall() - unless that changed and I missed it. That's already wrong with config but even more wrong if this is the only way we provide to switch a module off!

Yeah it's definitely not finished yet. Also, the code for the module overview needs to be cleaned up and get the disable functionality fully removed. But this is just a start.. so

Okay, that's just one actual fail left and 4 or 5 minor things that I missed. Will fix the rest tomorrow. Should be green some time in the afternoon.

I realize it is early in this patch's life to be worrying about UI considerations but something I was thinking is that we may want to remove the checkboxes from the main modules page entirely, and leave the Uninstall tab in place.

Yes, the checkboxes are confusing:
Checkboxes are expected to allow switching on and off like a light switch.
While we got used to the fact that a dependency might disable a checkbox, everybody certainly expects this to be possible if there is no indication the module is required by something else, especially right after installing a module.
However, we shouldn't remove them completely without substitution because we still need them as status indicators.

We might want to show a checkbox only for uninstalled modules and otherwise replace them with a green tick mark. This would clearly indicate that once installed they can't simply be unchecked. I think this is a large improvement, see in the screenshot the pre-installed modules vs. the yet to enable Field UI module:

The issue is assigned to you, so I'm providing you just the diff, in case you want to roll it into your next patch:

diff --git a/core/modules/system/system.admin.inc b/core/modules/system/system.admin.inc
index b6c83fb..1891993 100644
--- a/core/modules/system/system.admin.inc
+++ b/core/modules/system/system.admin.inc
@@ -986,7 +986,21 @@ function _system_modules_build_row($info, $extra) {
 
   // If this module is compatible, present a checkbox indicating
   // this module may be installed. Otherwise, show a big red X.
-  if ($compatible) {
+  if (!$compatible) {
+    $form['enable'] = array(
+      '#markup' =>  theme('image', array('uri' => 'core/misc/watchdog-error.png', 'alt' => $status_short, 'title' => $status_short)),
+    );
+    $form['description']['#markup'] .= $status_long;
+  }
+  else if ($extra['enabled']) {
+    $status_short = t('This module is installed.');
+    $status_long = t('This module is installed. To uninstall it please visit the <a href="@link">uninstall page</a>.', array('@link' => url('admin/modules/uninstall')));
+    $form['enable'] = array(
+      '#markup' =>  theme('image', array('uri' => 'core/misc/watchdog-ok.png', 'alt' => $status_short, 'title' => $status_short)),
+    );
+    $form['description']['#markup'] .= theme('system_modules_incompatible', array('message' => $status_long));
+  }
+  else {
     $form['enable'] = array(
       '#type' => 'checkbox',
       '#title' => t('Enable'),
@@ -997,12 +1011,6 @@ function _system_modules_build_row($info, $extra) {
       $form['enable']['#disabled'] = TRUE;
     }
   }
-  else {
-    $form['enable'] = array(
-      '#markup' =>  theme('image', array('uri' => 'core/misc/watchdog-error.png', 'alt' => $status_short, 'title' => $status_short)),
-    );
-    $form['description']['#markup'] .= theme('system_modules_incompatible', array('message' => $status_long));
-  }
 
   // Build operation links.
   foreach (array('help', 'permissions', 'configure') as $key) {

One thing with the uninstall tab is it only appears for modules with hook_schema() or hook_uninstall() - unless that changed and I missed it.

You missed it - twice now, in fact :)

https://drupal.org/node/1199946#comment-6956130 (see comment at the bottom)

Since D7, all modules appear on the uninstall page.

Upgrade path tests are still failing (at least locally)

UpdateModuleHandler is very hard to grok and I am having trouble to get it to work. Will focus on that tomorrow...

Some more fixes...

Huh? Where do the new install/uninstall fails come from?

Uh, found the reason.

#53: 2003966-53.patch queued for re-testing.

I screwed up the InstallUninstallTest. ModuleHandler is actually working already. And module_disable() should not be invoked anywhere anymore. Thanks though :)

That patch should bring us back to only UpgradePathTest failures.

Okay, I need help with t his one... I have no clue why this fails. It's the last failure and it's technically just one... Which is: Forum module fails upon uninstallation. But it doesn't do that when I do it manually. Just in this freaking test. It throws an exception from FieldInstance saying that it could not create a field for a missing UUID. Really, no idea... Been stepping through the code with xdebug for 3 days now, looking at interdiffs to swentel's patch, etc. No clue.

something like #2004316: Exception on field_purge_batch() after deleting a field ?

reroll

#87: 2003966-87.patch queued for re-testing.

rerolled against head

Removed module_install() as adding a deprecated function is plain weird. Based on #305

For some reason this failed on the main issue re-running test here so as to not clog that issue....

Rerolled on top of the massive changes from this morning

Forgot to remove core/modules/field/lib/Drupal/field/Tests/ActiveTest.php

Okay messed up the reroll a bit - but it lead to me finding #2078837: Ensure that we are using SQL storage for taxonomy_update_8007() so all is good

Okay hopefully we're green again...

Chasing head

Rerolled

/me *shakes fist at testbot*

So this has exposed a very interesting bug. During ModulesDisabledUpgradePathTest it tries to enable the editor module. This then discovers the default config for in the standard profile. It tries to install it and kaboom because this config is also dependent on ckeditor as that is the plugin it uses :)

Attached is one possible fix where we only include installation profile module configuration during an installation. Not entirely sure that the fix is legit.

So I've backed out the changes in #106 as this is a separate bug.

Whilst investigating the issue fixed in #106 I discovered that modules enabled in Drupal 7 are being enabled. I don't think we can do this. We should prevent major version upgrade if there are disabled modules.

We need to work on the text in UPGRADE.txt

7. Go to Administer > Site building > Modules. Disable all modules that are not
listed under "Core - required" or "Core - optional". It is possible that some
modules cannot be disabled because others depend on them. Repeat this step
until all non-core modules are disabled.

If you know that you will not re-enable some modules for Drupal 8.x and you
no longer need their data, then you can uninstall them under the Uninstall
tab after disabling them.

And just like that.

Updated issue summary.