Problem

#2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference introduced a new module install validator API, however configuration import does not yet check whether the validators pass. If they fail, module uninstall will throw exceptions.

Proposed resolution

  • Run module install validators from config import.
  • In order to avoid some checks like for configurable fields to be run twice, we want to extend the validator API to support running in two modes: excluding configuration-based checks or not.

Remaining tasks

Do.

User interface changes

-

API changes

- Extend the module uninstall validator by an optional parameter to exclude configuration-based validation checks. No BC break here (addition only).

Files: 
CommentFileSizeAuthor
#8 2392815-8-FixEventSubscriber.patch4.08 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,474 pass(es), 4 fail(s), and 2 exception(s). View
#8 2392815-8-FixImporter.patch2.67 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,480 pass(es), 4 fail(s), and 2 exception(s). View
#8 2392815-8-test-only.patch1.47 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,512 pass(es), 0 fail(s), and 1 exception(s). View

Comments

fago’s picture

xjm’s picture

Issue tags: +Configuration system
cilefen’s picture

Issue summary: View changes
cilefen’s picture

Status: Active » Needs review
FileSize
933 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,457 pass(es). View

This is running the uninstall validators on configuration import, not the API change yet.

cilefen’s picture

I think from here the next steps are to change ModuleInstallerInterface::validateUninstall() to take a second parameter indicating whether to check for configuration-based validity or not (default is true). To make this possible, ModuleUninstallValidatorInterface must have added to it a method that checks whether the given validator is configuration-based or not.

cilefen’s picture

FileSize
3.67 KB
4.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,452 pass(es). View
alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -761,6 +761,12 @@ protected function processExtension($type, $op, $name) {
    +        if ($reasons = $this->moduleInstaller->validateUninstall(array($name), FALSE)) {
    
    +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -500,10 +500,13 @@ protected function updateKernel($module_filenames) {
    +  public function validateUninstall(array $module_list, $configuration_based = TRUE) {
    

    I think instead of calling validateUninstall with a flag we could just add the uninstall validators to the ConfigImporter and only add the configuration based ones. Plus I think this validation should be done before the COnfigImporter actually starts importing.

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
    @@ -74,11 +74,13 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
    +   * @param bool $configuration_based
    +   *   Check with configuration-based validators.
    

    This should @see to the interface where what configuration-based means.

  3. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
    @@ -13,6 +13,14 @@
    +   * Indicate if this validator is configuration-based.
    

    I think this needs more explanation as to what configuration-based actually means.

bircher’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,512 pass(es), 0 fail(s), and 1 exception(s). View
2.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,480 pass(es), 4 fail(s), and 2 exception(s). View
4.08 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,474 pass(es), 4 fail(s), and 2 exception(s). View

Hello

Sorry that I didn't include the previous patch in this one but I think the previous patch does not address the issue in the right place. It runs the uninstall validator on individual modules when they are processed instead of during the validation phase. This is redundant because the uninstalling of modules already validates them and throws the errors. The validation needs to happen in the validation phase.

I discussed the issue with @fago during drupalaton and we concluded that the issue is two fold:

  • The validators are not taken into consideration and thus the import results in exceptions being thrown in the process. (see title of issue)
  • The some validators check the configuration of for example base fields and these checks are already done by other import validation processes, so running all validators results in the same or very similar checks being run twice.

I attached a test that shows that we currently have a problem. (You can not uninstall the system module during the synchronisation, so its not critical, but since the synchronisation uninstalls the modules one by one some others might have been uninstalled before the error is thrown and the site might end up in a funny state anyway, that would probably be another issue)

I attached two patches that run the uninstall validation, one fixing it in the Importer and one in the EventSubscriber. pick the better one.

The second part of the issue is not addressed in this patch yet, but one could argue that it could be a separate follow-up issue about the duplication of configuration validation.

bircher’s picture

There would also be a third option, fixing it in the event subscriber but instead of injecting the ModuleInstaller one could make the one from the Importer available.

The last submitted patch, 8: 2392815-8-test-only.patch, failed testing.

The last submitted patch, 8: 2392815-8-FixImporter.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2392815-8-FixEventSubscriber.patch, failed testing.

bircher’s picture

interesting!

The failing tests for the patches proves that we need to address both problems at the same time.
In fact running the ModuleUninstallValidator during the import validation doesn't make sense because the validation checks the currently active site configuration and not the configuration that will be imported.

What we need to do instead is to make sure that all the validators also have a config import event subscriber that can take the imported configuration into consideration or have an alternative validation that takes the imported configuration and does its checks with that.

More broadly I think it would be nice to have a standard way to validate that a set of configuration is in itself consistent and a way to bypass the validation and enforce it even though it leads to data loss. (for example uninstall the book module even though there is a book outline left, and instead of complaining the process should remove the outline.)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.