When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference

Proposed solution works for me.

This is certainly a big issue, though similar problems existed back in D7, didn't they? So at least peopled dealt with that problem earlier as well.

Yes, this already existed in 7.x, although in 8.x, it's going to be a bit.. rougher. Due to interface and methods, trying to display or edit, maybe even delete nodes with a missing node type will likely end in fatal errors and not some notices and warnings.

For a clean enviroment all entities should be deleted when their entity type gets deleted. To make a module required as soon as it defines an entity type sound like a really dirty quick fix to me (and it might be difficult to categorize it as this).

So it looks like #2224581: Delete forum data on uninstall added a custom hack to do this for the forum node type. And #1002164: The Book module can be uninstalled with nodes with a book node type still existing is about to do the same custom hack for the book node type.

Should we try to make that into a general solution here, possibly without the making-modules-required hack ?

This is a sketch of how this might work. It is rolled against #2324055: Split up the module manager into runtime information and extension information..

Thanks much for working on this! protected function validateUninstall should be public so that the form can call it ahead of the time and offer only uninstallable modules.

This is rolled against #2324055: Split up the module manager into runtime information and extension information. #40 and sets the method visibility to public and puts it in the interface. It still does not contain the real entity work.

It is not obvious to me how to differentiate a content entity from a configuration entity.

Thank you SO MUCH for working on this.

$content = $factory->get($module) should be $content = $factory->get($entity_type->id()). Not all entity types are the same as their module (although often they are). For example, taxonomy vs taxonomy_term and taxonomy_vocabulary.

You hardly need to check for SqlContentEntityStorage: the entity query is storage agnostic and so is the whole issue. But I guess you are well aware of that and only used this in place of checking for content entities.

So to check whether an entity is a content entity or not, the standard way is $entity_type->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface'). At least this exists 4 times already but of course it's impossible to find if you don't know what to look for specifically. This should be documented somewhere but where? Perhaps we should provide wrappers, only three interfaces are ever used... (Drupal\Core\Config\Entity\ConfigEntityInterface (6 times), Drupal\Core\Entity\FieldableEntityInterface (8 times) are the other two). How nice it'd be $entity_type->is('content') or $entity_type->isContent() even. This is a normal task followup, I feel.

See #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements()

+++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -58,5 +58,13 @@ public function install(array $module_list, $enable_dependencies = TRUE);
+  /**
+   * Determines which modules can be uninstalled.
+   *
+   * @param array $module_list
+   *   The modules to uninstall.
+   */
+  public function validateUninstall($module_list);

This should not be the responsibility of the ModuleInstaller. Rather the ModuleHandler is responsible for making runtime information available to any interested service (including to the module installer).

As the module installer performs the rather disruptive task of adding/removing modules, it only should be injected into/used by code which intends to perform such a task. Validation should also be possible without having to depend on module installer.

> Validation should also be possible without having to depend on module installer.

Could you please tell us how?

validateUninstall() has no dependencies on the installer, it can easily live on ModuleHandler.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -246,6 +259,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+    $module_list = $this->validateUninstall($module_list);

This then becomes to $this->moduleHandler->validateUninstall($module_list). For other examples see the cross branch interdiff linked in #2324055-50: Split up the module manager into runtime information and extension information..

> validateUninstall() has no dependencies on the installer

But it has an entity manager dependency which in turn depends on the module handler like any other plugin manager. Also it is not a runtime operation but an uninstall time only.

So in your opinion validateUninstall() needs to be on ModuleInstaller because if it were on ModuleHandler, then there would be a dependency loop? Note that in fact ModuleInstaller also has a dependency on ModuleHandler, so I fear this would not help either.

It would: you can inject module_handler into module_installer and entity.manager both without problems. The other direction would be problematic but it isn't so.

This is a reroll, again against #2324055: Split up the module manager into runtime information and extension information.. It needs the work suggested in #19 after, if there is a consensus on who injects who.

Changes from #19. This patch doesn't break Drupal but it does not yet work.

I think the only thing we had was misunderstanding not disagreement: there are no choices in what gets injected into what so it's kind of hard to disagree with it :)

Closley related issue: #2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data.

You are running 1 query per entity type on every cache rebuild (at least). How's that a good idea? Not to mention of firing the entity system while the module system is being rebuilt. That's a recipe for brittleness.

Why don't we add an hasData() method to the storage class? (Actually I thought it was already there)
That way it can be implemented in a storage-agnostic way.

That won't help the number of queries...

Actually I was referring to the previous approach too...

@plach we have countFieldData() for fields but not an actual hasData exactly because that can be done with the query we are using here.

The fact that we can do that with an entity query does not mean the ideal DX implies using an entity query. We could certainly use the EQ as the default method implementation, we use the same approach to implement entity loading by property (field) values IIRC. Anyway, let's not waste time on this, it was just a small remark I had after reviewing the last patches here, definitely not something that should block this issue :)

Initial review.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -41,6 +42,17 @@ class ModuleInstaller implements ModuleInstallerInterface {
      /**
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   +   */
   +  protected $entityManager;
   

   Incomplete docblock.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -41,6 +42,17 @@ class ModuleInstaller implements ModuleInstallerInterface {
   +  /**
   +   * Modules that can be uninstalled.
   +   * @var array
   +   */
   +  protected $uninstallValidated;
   

   Missing newline.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -460,4 +476,48 @@ protected function updateKernel($module_filenames) {
   +        $this->uninstallValidated = $module;
   

   $module is a string, $this->uninstallValidated is an array. This can't be correct.

4. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -460,4 +476,48 @@ protected function updateKernel($module_filenames) {
   +    $factory = \Drupal::service('entity.query');
   

   Should be injected?

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
   @@ -58,5 +58,13 @@ public function install(array $module_list, $enable_dependencies = TRUE);
   +  public function validateUninstall($module_list);
   

   array typehint misssing

I am not sure why this fails to install, Drupal\Core\Entity\Query\QueryFactory implements Drupal\Core\Entity\Query\QueryFactoryInterface.

Starting Drupal installation. This takes a few seconds ...                  [ok]
Recoverable fatal error: Argument 5 passed to Drupal\Core\Extension\ModuleInstaller::__construct() must be an instance of Drupal\Core\Entity\Query\QueryFactoryInterface, instance of Drupal\Core\Entity\Query\QueryFactory given in Drupal\Core\Extension\ModuleInstaller->__construct() (line 79 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Extension/ModuleInstaller.php).
Drush command terminated abnormally due to an unrecoverable error.       [error]].

#47 + #48: is that because \Drupal\Core\Entity\Query\QueryFactory doesn't implement QueryFactoryInterface? I wonder why? Seems wrong indeed.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -41,6 +43,24 @@ class ModuleInstaller implements ModuleInstallerInterface {
   +   * The entity manager.
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   ...
   +   * The entity query factory.
   +   * @var \Drupal\Core\Entity\Query\QueryFactory
   ...
   +   * Modules that can be uninstalled.
   +   * @var array
   

   Nit: needs newline between the single-line comment and the @var declaration.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -460,4 +487,47 @@ protected function updateKernel($module_filenames) {
   +      if ($module == $entity_type->getProvider() && is_subclass_of($entity_type->getClass(), 'Drupal\Core\Entity\Sql\SqlContentEntityStorage')) {
   

   This should check if \Drupal\Core\Entity\ContentEntityInterface is implemented, not SqlContentEntityStorage. There are several such examples to be found if you grep D8 for ContentEntityInterface' (yes, with that trailing single quote).

3. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -72,6 +84,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      // @todo use validateUninstall here also.
   +      //$modules = $this->moduleInstaller->validateUninstall($modules);
   

   Needs to be resolved before this can be RTBC :)

Thank you @Wim Leers. For #3, I think we could do an array_intersect_key() on the results of system_rebuild_module_data().

Yep, #49.3 is the only remaining problem… besides this still needing tests, of course.

Using array_intersect_key() sounds good.

I saw hasData() being discussed here above... Cross-linking to separate issue where hasData() is being implemented: #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -460,4 +490,47 @@ protected function updateKernel($module_filenames) {
+      if ($module == $entity_type->getProvider() && is_subclass_of($entity_type->getClass(), 'Drupal\Core\Entity\ContentEntityInterface') && $entity_type->getDataTable()) {

Explanation: Using ContentEntityInterface, you have to filter out the entities with null storage or queryFactory->get() throws an exception.

The tests should go in the existing Drupal\system\Tests\Extension\ModuleHandlerTest or in a new Drupal\system\Tests\Extension\ModuleInstallerTest.

At this stage, we have minimally working code. I had to modify ConfigImportAllTest to remove all shortcuts so that module will uninstall when asked. Here is what drush does in this situation:

There are shortcuts:

$ drush -y pm-uninstall shortcut
There was a problem uninstalling shortcut.                                                                                          [error]

Shortcuts were removed in the UI:

$ drush -y pm-uninstall shortcut
shortcut was uninstalled successfully. 

Fixed a bug found by @chx.

After some discussion with @chx on IRC, we decided to go with a validation service.

Although we have identified possible improvements for example rewriting validateUninstall to something like:

  public function validateUninstall(array $module_list) {
    return array_filter($module_list, function ($module) {
      if (!isset($this->uninstallValidated[$module])) {
        $valid = TRUE;
        foreach ($this->uninstallValidators as $validator) {
          if (!$valid = $validator->validate($module)) {
            break;
          }
        }
        $this->uninstallValidated[$module] = $valid;
      }
      return $this->uninstallValidated[$module];
    });
  }

but also moving the code in uninstall form checking for requiredness and installedness into a separate service (or two? or is that excessive) those can be done in followups. Once we have this in we can nuke all (existing and forthcoming) hook_system_info_alter easily.

Renamed $modules_no_content to $modules_validated.

+++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
@@ -0,0 +1,49 @@
+      if ($module == $entity_type->getProvider() && is_subclass_of($entity_type->getClass(), 'Drupal\Core\Entity\ContentEntityInterface') && $entity_type->getDataTable()) {

Core SQL entity storage supports table layouts with no data table, so we should probably check for the base table instead, if the reason is the one explained in #54.

@plach: Thank you for reviewing.

Now that I think about it: how would the interdiff above play with a Mongo storage? I am not entirely sure this is the correct approach: the Contact message entity type has currenty null storage, but there's a contrib module to swap in a regular SQL storage. In a similar scenario I think the check above might be unreliable. Can't we replace it with a check to verify whether the storage is an instance of the Null storage?

Pinged chx for #74.

reroll

Isn't base_table coming from the annotation? What does that have to do with the storage? (strange thing, that, but w/e) Also if it turns out it's necessary mongo will lie through its teeth about how beautiful tables it uses for storage. It is a very unscrupulous module -- it ties into Drupal as a SQL driver already (mostly extending the fake driver) -- so lying about storage is not a big deal.

After a lot of consideration the only way is #37: add a hasData method to EntityStorageBase the exact same code we have in this issue and then NullStorage can override it with a simple return FALSE. Because it is possible that a storage service doesn't really have query capabilities but can say something about its own emptiness. To elaborate, most (I'd say all, I did research in a core issue prior) key-value stores have an iterable keyspace but the store itself doesn't have query capabilities (since you are likely to store the entity serialized some way). So while entity queries could be refused it is very easy to see whether the keyspace is empty or not.

NW per #79.

Actually, I think this means that per #52, this is now blocked on #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage, which adds that ::hasData() method.

adding blocker tag, since this blocks #2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data, so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2338873.

oops. tag.

This is a bit naive and it doesn't work, but I thought this was what I would get from #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage.

You still want want to check for the ContentEntityInterface I think.

@Berdir: good point

Removed the now-unneeded EntityQuery service from ContentUninstallValidator.

Thank you @dawehner on IRC.

@effulgentsia Thank you for reviewing.

1: done
2: done
3: done
4: done
5: more could be done here
6: done

+++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
@@ -0,0 +1,39 @@
+      if ($module == $entity_type->getProvider() && $entity_type instanceof ContentEntityTypeInterface && $this->entityManager->getStorage($entity_type->id())->hasData()) {
+        return FALSE;

Why this is restricted to content entities? If I'D implement an entity being no content, the same validation check should be there - right?

I suppose it's not needed for config entities right now as handled already via the config system, but even if so - would it be harmful to handle it via entities as well? If so, we could still blacklist config entities instead of white listing content entities.

Approach and patch looks good to me else.

So how can we predict what a new kind of entity type in contrib will want?

Good point. However, you do not necessarily have to introduce a new kind of entity type, you could just opt to implement an entity which is neither content or config. In that case, we should apply the same check though or we'd have the same issue as we have with content entities now.

Thus imo, we should default to include the Validator for non-content non-config entities . But as you clarified, there needs a way to customize the behaviour for an entity type. So what about introducing a setting as part of the entity type, which has respective hard-coded defaults for config and content? E.g. $entity_type->forbidsNonEmptyUninstall() ?

I was working on adding test coverage but discovered that just hiding the module with confiuration from the UI doesn't solve all problems.

There is still some problem with the test but since its commented out it should still work and can be reviewed and then put back to needs work.

To answer the question about what to validate, since we add a validator service here one could add another special-entity-validator, or anything really to prevent modules from being uninstalled...

Now the tests are added and should pass.

This is beginning to look really good.

1. +++ b/core/modules/system/system.admin.inc
   @@ -312,6 +312,12 @@ function theme_system_modules_uninstall($variables) {
   +      $disabled_message = format_plural(count($form['modules'][$module]['#validation_reasons']),
   

   format_plural is deprectated use \Drupal::translation()->formatPlural() - I know there are other usages of format_plural in this file but we should avoid added new usages.

2. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -0,0 +1,46 @@
   +  use StringTranslationTrait;
   

   We should also pass the string translation service into the constructor and call $this->setStringTranslation() - will need to update service definition.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -11,6 +11,7 @@
   +use Drupal\Core\Extension\UninstallValidatorInterface;
   

   Same namespace not needed.

4. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -315,7 +338,7 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   -    // Only process modules that are enabled. A module is only enabled if it is
   +    // Only process modules that are enabled. A module is only disabled if it is
   

   Not sure why this change is here - it is also wrong - modules are installed and uninstalled. disable/enable is no longer used (yes we have a lot of docs to fix)

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -469,4 +492,23 @@ protected function updateKernel($module_filenames) {
   +          $reasons = array_merge($reasons, $validation_reasons);
   

   I think we need to key this by the module - and the each module might have several reasons.

6. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -469,4 +492,23 @@ protected function updateKernel($module_filenames) {
   +    if ($reasons) {
   +      return $reasons;
   +    }
   +    return NULL;
   

   I think we should just return an array empty - ie. there are no reasons to not uninstall.

7. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
   @@ -7,6 +7,8 @@
   +use Drupal\Core\Extension\UninstallValidatorInterface;
   

   Same namespace - not needed.

8. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -0,0 +1,46 @@
   +        $reasons[] = $this->t('There is content for the entity type: @entity_type', array('@entity_type' => $entity_type->getLabel()));
   
   +++ b/core/modules/system/system.admin.inc
   @@ -312,6 +312,12 @@ function theme_system_modules_uninstall($variables) {
   +    if (!empty($form['modules'][$module]['#validation_reasons'])) {
   +      $disabled_message = format_plural(count($form['modules'][$module]['#validation_reasons']),
   +        'The following reason prevents @module from being uninstalled: @reasons',
   +        'The following reasons prevents @module from being uninstalled: @reasons',
   +        array('@module' => $form['modules'][$module]['#module_name'], '@reasons' => implode(', ', $form['modules'][$module]['#validation_reasons'])));
   +    }
   

   Missing test coverage of this text appearing on the module uninstall page

9. +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
   @@ -200,6 +200,48 @@ function testUninstallProfileDependency() {
   +    // Create a fake dependency.
   +    // entity_test will depend on help.
   

   This comment could be expanded to describe why you are doing this.

10. +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
    @@ -200,6 +200,48 @@ function testUninstallProfileDependency() {
    +    // uninstalling help needs entity_test to be un-installable.
    

    Uninstalling... (capital U).

11. +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
    @@ -200,6 +200,48 @@ function testUninstallProfileDependency() {
    +    $result = $this->moduleInstaller()->uninstall(array('entity_test'));
    +    $this->assertTrue($result, 'ModuleHandler::uninstall() returns TRUE.');
    +
    +    $result = $this->moduleInstaller()->uninstall(array('help'));
    +    $this->assertTrue($result, 'ModuleHandler::uninstall() returns TRUE.');
    

    Here we should just uninstall help and assert that entity_test has also been uninstalled.

1) Ok, I copied it from the line above. But refactoring the required_by into reasons gets rid of the other one too.
2) fixed
3) fixed
4) I'll leave it alone then, can be set right in another patch
5) fixed
6) fixed
7) fixed
8) fixed
9) fixed
10) fixed
11) fixed

So what about introducing a setting as part of the entity type, which has respective hard-coded defaults for config and content? E.g. $entity_type->forbidsNonEmptyUninstall() ?

As discussed with alexpott, this is probably enough of an edge case to require the entity-type implementing module to care about its own module uninstall validation logic. Howsoever, I'm fine with not handling it here.

Took a look at the patch and gave it a test run. It looks pretty good already.

1. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -0,0 +1,47 @@
   +<?php
   +/**
   

   Missing new line after <?php

2. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -0,0 +1,47 @@
   +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
   +   *   The entity manager.
   ...
   +  public function __construct(EntityManagerInterface $entity_manager, TranslationInterface $string_translation) {
   

   Incomplete @param docs.

3. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -0,0 +1,47 @@
   +      if ($module == $entity_type->getProvider() && $entity_type instanceof ContentEntityTypeInterface && $this->entityManager->getStorage($entity_type->id())->hasData()) {
   

   This uses hasData() but it's only available on dynamically fieldable entities. We should probably move it to EntityStorageBase such we can safely check it here.

4. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -0,0 +1,47 @@
   +    return NULL;
   

   Unnecessary, could be omitted.

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -306,6 +320,16 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +    // Use the validators and print the reason
   ...
   +      foreach ($reasons as $module_reason) {
   +        foreach($module_reason as $reason) {
   +          drupal_set_message($reason, 'error');
   

   I'm not sure about displaying messages here - this is an API method which should not write messages in case of errors. It could throw exceptions but the defined error behaviour is returning FALSE, so probably it should just do that.
   Modules/drush who need to know the reason could still just invoke uninstall validation on their own - as the UI does.

6. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -469,4 +493,23 @@ protected function updateKernel($module_filenames) {
   +  public function validateUninstall(array $module_list) {
   +    $reasons = array();
   

   This makes sense to have, but we are introducing a new API here without adding support for the other validations as part of it. According to the docs I'd expect other uninstallation problems to be returned also.
   I'd suggest opening a follow-up for converting the other uninstallation checks to the new API as well.

7. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -469,4 +493,23 @@ protected function updateKernel($module_filenames) {
   +    return $reasons;
   +  }
   

   Looks like it returns an empty array instead of NULL. Maybe that would be a good return value for the "all fine" case anyway, i.e. for entity validation you always get a list of validation fails and you have to check whether it's empty.

8. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
   @@ -58,5 +58,24 @@ public function install(array $module_list, $enable_dependencies = TRUE);
   +   * Adds module uninstall validator services.
   

   It doesn't add services, it adds uninstall validators. Whether it's a service or not does not matter to us.

9. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
   @@ -58,5 +58,24 @@ public function install(array $module_list, $enable_dependencies = TRUE);
   +   * @param array $module_list
   

   Should be string[]

10. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
    @@ -58,5 +58,24 @@ public function install(array $module_list, $enable_dependencies = TRUE);
    +   * @return array
    

    string[]|null

11. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
    @@ -58,5 +58,24 @@ public function install(array $module_list, $enable_dependencies = TRUE);
    +   *   Reasons for which the module list can not be uninstalled or NULL
    

    Misses trailing point. Should clarify when it does it return NULL.

12. +++ b/core/lib/Drupal/Core/Extension/UninstallValidatorInterface.php
    @@ -0,0 +1,24 @@
    +<?php
    +/**
    

    Missing NL after php

13. +++ b/core/lib/Drupal/Core/Extension/UninstallValidatorInterface.php
    @@ -0,0 +1,24 @@
    + * Common interface for module uninstall validators.
    ...
    +interface UninstallValidatorInterface {
    

    If the interface is about modules only, maybe the name should reflect that and be ModuleUninstallValidatorInterface ?

14. +++ b/core/lib/Drupal/Core/Extension/UninstallValidatorInterface.php
    @@ -0,0 +1,24 @@
    +   * @return array
    +   *   array of reasons the module can not be uninstalled or NULL if it can.
    

    Sentence need to start capitalized. Types should be string[] also.

15. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
    @@ -69,7 +81,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    // Get a list of disabled, installed modules.
    +    // Get a list all available modules.
    

    missing "of"

16. +++ b/core/modules/system/src/Tests/Module/UninstallTest.php
    @@ -43,9 +43,22 @@ function testUserPermsUninstalled() {
    +    // Add a node to prevent node from being uninstalled
    

    Missing trailing point.

17. +++ b/core/modules/system/tests/modules/module_test/module_test.module
    @@ -27,6 +27,10 @@ function module_test_system_info_alter(&$info, Extension $file, $type) {
    +    elseif ($file->getName() == 'entity_test') {
    +      // Make entity test module depend on help module.
    +      $info['dependencies'][] = 'help';
    

    Is that still necessary? Looks like it's done otherwise in the test case.

Then there is the issue that noData() is not required for content entities, opened a follow-up for moving it up to EntityStorageInterface (API change) #2391829: ContentUninstallValidator relies on the not required ContentEntityStorage::hasData() method.

Another good reason to add the uninstall validate method and stop using hook_system_info_alter() -> #2387983: PluginNotFoundException when enabling module that provides text filter.

5) I changed it to thorw a new exception.

6) Yes it will check all ModuleUninstallValidators that were attached, at the moment just this one, then in a followup we process all of them and the issue catch pointed out would be solved with it.

17) Yes we need to test that you can not uninstall a module that has a module which wants to uninstall a dependent module that has a reason for not being able to be uninstalled. Or in other words: we test that the dependencies are resolved before the validation.

All the others points should be fixed.

Thanks. Imo this is about to be ready. Only found two nit-picks:

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -320,15 +320,14 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +    // Use the validators and throw an exception with the reasons
   

   Still misses trailing point.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorException.php
   @@ -0,0 +1,11 @@
   +
   +class ModuleUninstallValidatorException extends \InvalidArgumentException { }
   

   Misses docs.

Also opened #2392363: [META] Make use of module uninstall validators.

Thanks!

I'm glad to see this fixed, and that it will make other things more robust such as #110. Committed 8.0.x. Thanks all!

Just a couple of wording nitpicks.

The functionality itself looks great and really glad we're doing this.

1. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -0,0 +1,47 @@
   +        $reasons[] = $this->t('There is content for the entity type: @entity_type', array('@entity_type' => $entity_type->getLabel()));
   

   I like the variable name. We can't install your module because reasons.

   How doable is it to link to entity type admin page here?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -306,6 +320,15 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +      throw new ModuleUninstallValidatorException(format_string('The following reasons prevents the modules from being uninstalled: @reasons', array(
   

   Shouldn't we do a message per module here? Otherwise it'll be a bit of detective work to figure out which module is responsible for what - not always obvious.

3. +++ b/core/modules/system/system.admin.inc
   @@ -303,14 +303,16 @@ function theme_system_modules_uninstall($variables) {
   +      $form['modules'][$module]['#validation_reasons'][] = \Drupal::translation()->translate('Required by: @modules', array('@modules' => implode(', ',$form['modules'][$module]['#required_by'])));
   

   Nice touch.

4. +++ b/core/modules/system/system.admin.inc
   @@ -303,14 +303,16 @@ function theme_system_modules_uninstall($variables) {
   +        'The following reasons prevents @module from being uninstalled: @reasons',
   

   reasons prevent.

Cross-posted. That can be a follow-up. Opened #2392533: Grammatical error in ModuleUninstallValidatorException message.

Also no change notice yet.

@effulgentsia I think this is partially covered by #2392351: When an entity bundle config gets deleted, entities of that bundle break - but we also have non-bundable entities and non config entity bundled entities to worry about. One issue that we can't pre-fire all uninstall validators before starting the import - because for example field delete is actually handled by import.

Draft change record: https://www.drupal.org/node/2392677

ad #123,#124:
I agree we should run them to make sure non-config module uninstall validation tasks are run as well. Opened #2392815: Module uninstall validators are not used to validate a config import

@cilefen:

The CR is a bit sparse, can we briefly explain what a validator is and add before/after code examples?

@plach Thanks for looking at it. Better now?

Thanks, published the CR with a small change to enable PHP code syntax highlighting.

Can someone who worked on this confirm that we can remove menu_link_content_uninstall() now? See #2312389: Remove menu_link_content_uninstall()

Hi,

I know this is rather late. Is there are way the 'ModuleUninstallValidatorException' exception can be caught so custom modules can implement the clean up using hook_uninstall? Would making ModuleInstaller a plugin be helpful so we can implement it in the custom modules?

Thanks,

Michael

Open a new issue please.