Problem/Motivation

Entities with missing modules will result in fatal errors everywhere.

Postponed on #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage

Proposed resolution

Add an uninstall validate method to the module_installer service added in #2324055: Split up the module manager into runtime information and extension information.. Also make uninstall call it if it wasn't called before having a uninstall_validated array on the class listing the modules already validated for uninstall. This method should call a helper on entity manager that checks for any entities provided by that module (handwaving a bit here -- who discovers how an entity provides which module types).

The uninstall method should validate the whole list of modules after all dependencies are resolved.

Then the ModulesUninstallForm can call this validate handler to inform the user why a module can not be uninstalled.

Make the uninstall form handle this. Much better than running (number of entity types) queries on _system_rebuild_module_data...

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add automated tests Instructions
when this is fixed, unpostpone #2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data

User interface changes

  • As of #53, modules that are uninstallable because content entities exist are not shown on the uninstall form. That may or may not be the behavior we want.
  • As of #104, modules that are uninstallable because content entities exist are disabled on the uninstall form and provide a message with a reason.

API changes

  • ModuleInstallerInterface->validateUninstall() has been added.
  • Module uninstall validators can register themselves with the module_install.uninstall_validator tag and implement \Drupal\Core\Extension\UninstallValidatorInterface

Follow-ups:

Files: 
CommentFileSizeAuthor
#116 interdiff-111-115.txt1.2 KBbircher
#116 2278017-115.patch20.3 KBbircher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,177 pass(es). View
#111 2278017-111.patch20.21 KBbircher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,101 pass(es). View
#111 interdiff-107-111.txt11.63 KBbircher
#107 2278017-107.patch18.01 KBbircher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,091 pass(es). View
#107 interdiff-104-107.txt11.13 KBbircher
#104 2278017-104.patch16.24 KBbircher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,088 pass(es). View
#104 interdiff-103-104.txt5.12 KBbircher
#104 interdiff-95-104.txt11.98 KBbircher
#103 interdiff-95-103.txt10.04 KBbircher
#103 2278017-103.patch14.65 KBbircher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,090 pass(es). View
#95 when_a_content_entity-2278017-95.patch10.59 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,125 pass(es). View
#95 interdiff-92-95.txt4.24 KBcilefen
#92 when_a_content_entity-2278017-92.patch10.59 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,126 pass(es). View
#92 interdiff-90-92.txt835 bytescilefen
#90 when_a_content_entity-2278017-90.patch10.62 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,162 pass(es). View
#90 interdiff-89-90.txt1.64 KBcilefen
#89 when_a_content_entity-2278017-89.patch10.85 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,127 pass(es). View
#89 interdiff-75-89.txt1.04 KBcilefen
#87 when_a_content_entity-2278017-85.patch10.76 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,590 pass(es), 682 fail(s), and 170 exception(s). View
#85 when_a_content_entity-2278017-85-do-not-test.patch10.76 KBcilefen
#85 interdiff-75-85.txt980 bytescilefen
#76 when_a_content_entity-2278017-75.patch11 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,840 pass(es). View
#73 when_a_content_entity-2278017-73.patch11.01 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,919 pass(es). View
#73 interdiff-71-73.txt883 bytescilefen
#71 when_a_content_entity-2278017-71.patch11.01 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,925 pass(es). View
#71 interdiff-62-71.txt1.2 KBcilefen
#62 when_a_content_entity-2278017-62.patch11.01 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,927 pass(es). View
#62 interdiff-61-62.txt7.55 KBcilefen
#61 when_a_content_entity-2278017-61.patch9.11 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,904 pass(es). View
#61 interdiff-57-61.txt744 bytescilefen
#57 when_a_content_entity-2278017-57.patch9.06 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,892 pass(es). View
#57 interdiff-53-57.txt735 bytescilefen
#53 when_a_content_entity-2278017-53.patch8.22 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,899 pass(es), 3 fail(s), and 0 exception(s). View
#53 interdiff-50-53.txt2.68 KBcilefen
#50 when_a_content_entity-2278017-50.patch7.86 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,005 pass(es), 85 fail(s), and 0 exception(s). View
#50 interdiff-48-50.txt1.18 KBcilefen
#48 when_a_content_entity-2278017-48.patch7.84 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,812 pass(es). View
#48 interdiff-45-48.txt2.21 KBcilefen
#45 when_a_content_entity-2278017-45.patch7.9 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#45 interdiff-28-45.txt3.99 KBcilefen
#35 pp_1_when_a_content-2278017-28.patch7.49 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,923 pass(es). View
#34 interdiff.txt794 byteseffulgentsia
#34 2278017-34.patch1.53 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,393 pass(es). View
#32 2278017-32.patch1.27 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,323 pass(es), 0 fail(s), and 14 exception(s). View
#28 pp_1_when_a_content-2278017-28.patch7.49 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,900 pass(es). View
#28 interdiff-27-28.txt2.18 KBcilefen
#27 pp_1_when_a_content-2278017-27.patch7.52 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#18 pp_1_when_a_content-2278017-18.patch7.05 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pp_1_when_a_content-2278017-18.patch. Unable to apply patch. See the log in the details link for more information. View
#17 pp_1_when_a_content-2278017-17.patch4.32 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pp_1_when_a_content-2278017-17.patch. Unable to apply patch. See the log in the details link for more information. View
#16 pp_1_when_a_content-2278017-16.patch3.85 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pp_1_when_a_content-2278017-16.patch. Unable to apply patch. See the log in the details link for more information. View
#14 pp_1_when_a_content-2278017-14.patch3.22 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pp_1_when_a_content-2278017-14.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

effulgentsia’s picture

Issue summary: View changes
catch’s picture

Proposed solution works for me.

xjm’s picture

Issue tags: +Configuration system
dawehner’s picture

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.

Berdir’s picture

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.

catch’s picture

Issue tags: +D8 upgrade path
BeyersdorferTJ’s picture

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

naxoc’s picture

Berdir’s picture

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 ?

cilefen’s picture

Title: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken references » [pp-1] When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken references
Issue summary: View changes
Status: Active » Postponed
chx’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes
cilefen’s picture

FileSize
3.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pp_1_when_a_content-2278017-14.patch. Unable to apply patch. See the log in the details link for more information. View

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

chx’s picture

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.

cilefen’s picture

FileSize
3.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pp_1_when_a_content-2278017-16.patch. Unable to apply patch. See the log in the details link for more information. View

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.

cilefen’s picture

FileSize
4.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pp_1_when_a_content-2278017-17.patch. Unable to apply patch. See the log in the details link for more information. View
cilefen’s picture

FileSize
7.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pp_1_when_a_content-2278017-18.patch. Unable to apply patch. See the log in the details link for more information. View

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

chx’s picture

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.

tim.plunkett’s picture

znerol’s picture

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

chx’s picture

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

Could you please tell us how?

znerol’s picture

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

chx’s picture

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

znerol’s picture

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.

chx’s picture

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.

cilefen’s picture

FileSize
7.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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.

cilefen’s picture

FileSize
2.18 KB
7.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,900 pass(es). View

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

chx’s picture

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

xjm’s picture

xjm’s picture

effulgentsia’s picture

Status: Postponed » Needs review
FileSize
1.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,323 pass(es), 0 fail(s), and 14 exception(s). View
+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -460,4 +476,48 @@ protected function updateKernel($module_filenames) {
+  protected function moduleHasContent($module) {
+    $entities = $this->entityManager->getDefinitions();
+    $factory = \Drupal::service('entity.query');
+    foreach ($entities as $entity_type) {
+      if ($module == $entity_type->getProvider() && is_subclass_of($entity_type->getClass(), 'Drupal\Core\Entity\Sql\SqlContentEntityStorage')) {

Out of curiosity, what's the benefit of baking logic about content entities into ModuleInstaller, as the above does, as opposed to using hook_system_info_alter() (as forum.module does in HEAD per #10 and as #31 is proposing to do for fields)? Attaching a patch as a POC of the hook_system_info_alter() approach. Setting to "needs review" for testbot, but will re-postpone because I'm not yet suggesting we deviate from #28, only inquiring about what's wrong with the hook_system_info_alter() approach.

Status: Needs review » Needs work

The last submitted patch, 32: 2278017-32.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,393 pass(es). View
794 bytes
effulgentsia’s picture

Status: Needs review » Postponed
FileSize
7.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,923 pass(es). View

Reuploading #28 and repostponing the issue. I'd love feedback on #32 / #34 though in order to understand why this issue is proposing the direction it is.

chx’s picture

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.

plach’s picture

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.

chx’s picture

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

plach’s picture

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

chx’s picture

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

plach’s picture

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

cilefen’s picture

Title: [pp-1] When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken references » When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken references
Status: Postponed » Needs work
Wim Leers’s picture

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

xjm’s picture

Issue tags: +Triaged D8 critical
cilefen’s picture

Status: Needs work » Needs review
FileSize
3.99 KB
7.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 45: when_a_content_entity-2278017-45.patch, failed testing.

cilefen’s picture

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]].
cilefen’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
7.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,812 pass(es). View
Wim Leers’s picture

Status: Needs review » Needs work

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

cilefen’s picture

FileSize
1.18 KB
7.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,005 pass(es), 85 fail(s), and 0 exception(s). View

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

Wim Leers’s picture

Issue tags: +Needs tests

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

Using array_intersect_key() sounds good.

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
8.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,899 pass(es), 3 fail(s), and 0 exception(s). View
cilefen’s picture

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

cilefen’s picture

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

Status: Needs review » Needs work

The last submitted patch, 53: when_a_content_entity-2278017-53.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
735 bytes
9.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,892 pass(es). View
cilefen’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes
cilefen’s picture

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. 
cilefen’s picture

FileSize
744 bytes
9.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,904 pass(es). View

Fixed a bug found by @chx.

cilefen’s picture

FileSize
7.55 KB
11.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,927 pass(es). View

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

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.

The last submitted patch, 14: pp_1_when_a_content-2278017-14.patch, failed testing.

The last submitted patch, 16: pp_1_when_a_content-2278017-16.patch, failed testing.

The last submitted patch, 17: pp_1_when_a_content-2278017-17.patch, failed testing.

The last submitted patch, 18: pp_1_when_a_content-2278017-18.patch, failed testing.

The last submitted patch, 27: pp_1_when_a_content-2278017-27.patch, failed testing.

cilefen’s picture

Issue summary: View changes

The last submitted patch, 50: when_a_content_entity-2278017-50.patch, failed testing.

cilefen’s picture

FileSize
1.2 KB
11.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,925 pass(es). View

Renamed $modules_no_content to $modules_validated.

plach’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

cilefen’s picture

FileSize
883 bytes
11.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,919 pass(es). View

@plach: Thank you for reviewing.

plach’s picture

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?

Wim Leers’s picture

Pinged chx for #74.

cilefen’s picture

FileSize
11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,840 pass(es). View

reroll

chx’s picture

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

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.

effulgentsia’s picture

you have to filter out the entities with null storage or queryFactory->get() throws an exception

How hard would it be to fix #2337753: ContentEntityNullStorage does not implement a query service? If we don't want to hold this issue up on that, then instead of checking for NullStorage or a base table, how about we wrap that get() in a try/catch with an @todo linking to that issue?

chx’s picture

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.

Wim Leers’s picture

Status: Needs review » Needs work

NW per #79.

Wim Leers’s picture

Title: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference » [PP-1] When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference
Status: Needs work » Postponed
YesCT’s picture

Issue summary: View changes

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.

YesCT’s picture

Issue tags: +blocker

oops. tag.

cilefen’s picture

Title: [PP-1] When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference » When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference
Status: Postponed » Needs work
cilefen’s picture

Wim Leers’s picture

Status: Needs work » Needs review
cilefen’s picture

FileSize
10.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,590 pass(es), 682 fail(s), and 170 exception(s). View
Berdir’s picture

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

cilefen’s picture

FileSize
1.04 KB
10.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,127 pass(es). View

@Berdir: good point

cilefen’s picture

FileSize
1.64 KB
10.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,162 pass(es). View

Removed the now-unneeded EntityQuery service from ContentUninstallValidator.

The last submitted patch, 87: when_a_content_entity-2278017-85.patch, failed testing.

cilefen’s picture

FileSize
835 bytes
10.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,126 pass(es). View

Thank you @dawehner on IRC.

effulgentsia’s picture

Status: Needs review » Needs work

This is looking close. Thank you, @cilefen, for sticking with it.

  1. +++ b/core/core.services.yml
    @@ -289,7 +289,14 @@ services:
    +      - { name: module_install.uninstall_validator, priority: -254 }
    

    Why such a low priority? The only reason I can think of is to avoid a query if some other, cheaper, validator returns FALSE for a different reason, but that seems like a premature optimization at this point, since we don't yet know what other validators we'll have. I'd suggest leaving priority out at the moment. Or else, pick either -10 or -100 and have a comment explaining that a <0 priority is chosen due to the expensiveness of the validator.

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -469,4 +493,25 @@ protected function updateKernel($module_filenames) {
    +      foreach ($this->uninstallValidators as $validator) {
    +        if ($validator->validate($module)) {
    +          $this->uninstallValidated[] = $module;
    +        }
    +        else {
    +          unset($module_list[$key]);
    +        }
    +      }
    

    The else should have a break;, since why keep running validators after one fails.

    Also, is the $this->uninstallValidated cache useful? As written, it seems problematic, because we should only add to it if all validators return TRUE, not just one. But also, the result can change. For example, in a request, a ModuleInstaller service can be constructed, validate() called, but the module not uninstalled. Then, entities added, at which point subsequent validate() calls should return FALSE. Would it make sense to remove the cache entirely from this issue and only add it later, when we have a concrete situation in which it's helpful?

  3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
    @@ -58,5 +60,22 @@ public function install(array $module_list, $enable_dependencies = TRUE);
    +   *   The uninstall validation service to add. It is added at the end of the
    +   *   priority list (lower priority relative to existing ones).
    

    Nothing in this code deals with priority; that's all handled by the service collector. So, I think the second sentence can be removed.

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

    Needs an @return.

  5. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
    @@ -71,8 +83,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
         // Get a list of disabled, installed modules.
         $modules = system_rebuild_module_data();
    -    $uninstallable = array_filter($modules, function ($module) use ($modules) {
    -      return empty($modules[$module->getName()]->info['required']) && drupal_get_installed_schema_version($module->getName()) > SCHEMA_UNINSTALLED;
    +    $modules_validated = $this->moduleInstaller->validateUninstall(array_keys($modules));
    +    $uninstallable = array_filter($modules, function ($module) use ($modules, $modules_validated) {
    +      return empty($modules[$module->getName()]->info['required']) && drupal_get_installed_schema_version($module->getName()) > SCHEMA_UNINSTALLED && in_array($module->getName(), $modules_validated);
         });
    

    While we're in here, let's fix that code comment. There's no such thing as a disabled module anymore. And we should mention something about required and validated.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
@@ -0,0 +1,39 @@
+    $entities = $this->entityManager->getDefinitions();
+    foreach ($entities as $entity_type) {

Also, s/$entities/$entity_types/ or inline the value into the foreach.

cilefen’s picture

Status: Needs work » Needs review
FileSize
4.24 KB
10.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,125 pass(es). View

@effulgentsia Thank you for reviewing.

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

effulgentsia’s picture

Status: Needs review » Needs work

Thanks! I think all that's left is tests, so "needs work" for that.

+++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -69,10 +81,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-    // Get a list of disabled, installed modules.
+    // Get a list all available modules.
+    // Get a list of modules that have been validated as safe to uninstall.

Per #95.5, this could also be improved a bit more, but no need for that to hold up a critical.

xjm’s picture

Issue tags: +Ghent DA sprint
fago’s picture

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

bircher’s picture

Assigned: Unassigned » bircher
effulgentsia’s picture

Why this is restricted to content entities?

Well, the name of the validator is ContentUninstallValidator. The result of a failed validation is that the uninstallation form prevents you from selecting the module. So, we do not want this applying to config entities, since there, we do want you to select the module, and then after selecting it, we delete the config as part of the uninstallation.

If I'D implement an entity being no content, the same validation check should be there - right?

I'm not sure about that. We only have two kinds of entity types in core: config and content. And we want different uninstallation logic for them. So how can we predict what a new kind of entity type in contrib will want? I think rather than adding validation for an unknown kind to core's validator, we should leave it to the contrib module that defines additional kinds to add its own validator if it wants that.

fago’s picture

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() ?

effulgentsia’s picture

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() ?

Works for me. Either in this issue or in a followup, with a preference of followup unless it can be implemented and agreed to quickly, since #2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data is blocked on this.

bircher’s picture

Status: Needs work » Needs review
FileSize
14.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,090 pass(es). View
10.04 KB

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

bircher’s picture

FileSize
11.98 KB
5.12 KB
16.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,088 pass(es). View

Now the tests are added and should pass.

bircher’s picture

Issue summary: View changes
alexpott’s picture

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.

bircher’s picture

FileSize
11.13 KB
18.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,091 pass(es). View

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

fago’s picture

Status: Needs review » Needs work

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.

fago’s picture

Issue summary: View changes

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.

catch’s picture

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.

bircher’s picture

FileSize
11.63 KB
20.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,101 pass(es). View

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.

bircher’s picture

Status: Needs work » Needs review
fago’s picture

Issue summary: View changes

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.

fago’s picture

Status: Needs review » Needs work
fago’s picture

Issue summary: View changes
bircher’s picture

Status: Needs work » Needs review
FileSize
20.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,177 pass(es). View
1.2 KB
fago’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

  • Dries committed 20a8ccf on 8.0.x
    Issue #2278017 by cilefen, bircher, effulgentsia: When a content entity...
Dries’s picture

Status: Reviewed & tested by the community » Fixed

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!

catch’s picture

Status: Fixed » Needs work

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.

catch’s picture

Status: Needs work » Fixed
catch’s picture

Issue tags: +Needs change record

Also no change notice yet.

effulgentsia’s picture

Do we have an existing open issue to also apply this new validation during config deployment (as a config validator)? If not, we should open that as well.

alexpott’s picture

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

cilefen’s picture

fago’s picture

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 run on config import

plach’s picture

@cilefen:

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

cilefen’s picture

@plach Thanks for looking at it. Better now?

plach’s picture

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

Berdir’s picture

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

Status: Fixed » Closed (fixed)

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

cilefen’s picture