Problem/Motivation

In #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference we introduced a ModuleUninstallValidatorInterface that allows preventing modules from being uninstalled. Currently book field filter and forum implement hook_system_info_alter() to achieve the same.

Proposed resolution

Transform the logic of the hook_system_info_alter() implementations to uninstall validator services.

Remaining tasks

Transform the logic of the hook_system_info_alter() implementations to uninstall validator services.

User interface changes

Modules show up on the uninstall page having the checkbox disabled and showing a reason for which the module can not be uninstalled.

API changes

Instead of using hook_system_module_info_alter() you can now register services tagged with

+      - { name: module_install.uninstall_validator }

which validate whether you can uninstall a module.

This is an API change, but its not the case that many contrib modules break it.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the uninstall validator is more specific than hook_system_info_alter which has buggy side effects.
Issue priority Major because it solves other Major bugs such as #2387983: PluginNotFoundException when enabling module that provides text filter
Prioritized changes The main goal of this issue is to make it more stable.

CommentFileSizeAuthor
#65 2392293.65.patch77.36 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,919 pass(es). View
#65 63-65-interdiff.txt956 bytesalexpott
#63 2392293.63.patch77.35 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,223 pass(es), 1 fail(s), and 0 exception(s). View
#63 62-63-interdiff.txt5.06 KBalexpott
#63 Screen Shot 2015-05-27 at 17.02.03.png118.5 KBalexpott
#63 Screen Shot 2015-05-27 at 16.58.52.png131.63 KBalexpott
#62 2392293.61.patch75.79 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,238 pass(es). View
#62 59-61-interdiff.txt1.67 KBalexpott
#59 2392293.59.patch74.12 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,219 pass(es), 1 fail(s), and 0 exception(s). View
#59 55-59-interdiff.txt25.18 KBalexpott
#55 interdiff-2392293-53-55.txt4.15 KBbircher
#55 2392293-55.patch69.91 KBbircher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,082 pass(es). View
#53 2392293-53.patch69.88 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#51 2392293-51.patch69.99 KBbircher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,425 pass(es). View
#49 interdiff-2392293-47-49.txt3.4 KBbircher
#49 2392293-49.patch69.99 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,387 pass(es), 2 fail(s), and 0 exception(s). View
#47 2392293-57.patch69.99 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#47 interdiff-2392293-45-57.txt12.6 KBbircher
#45 interdiff.txt2.38 KBtim.plunkett
#45 refactor-2392293-45.patch69.66 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,500 pass(es). View
#44 interdiff-2392293-42-44.txt941 bytesbircher
#44 2392293-44.patch69.59 KBbircher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,449 pass(es). View
#42 2392293-42.patch68.67 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,447 pass(es), 2 fail(s), and 0 exception(s). View
#42 interdiff2392293-39-42.txt9.28 KBbircher
#39 2392293-39.patch66.16 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,605 pass(es), 2 fail(s), and 4 exception(s). View
#35 refactor-2392293-35.patch60.92 KBgobinathm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,352 pass(es), 0 fail(s), and 4 exception(s). View
#35 interdiff-2392293-30-35.txt706 bytesgobinathm
#30 refactor-2392293-30.patch60.91 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,244 pass(es), 0 fail(s), and 4 exception(s). View
#30 interdiff-28-30.txt2.35 KBcilefen
#28 refactor-2392293-28.patch59.17 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,157 pass(es), 0 fail(s), and 6 exception(s). View
#28 interdiff-25-28.txt899 bytescilefen
#25 interdiff.txt1.26 KBtim.plunkett
#25 refactor-2392293-25.patch58.46 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,053 pass(es), 0 fail(s), and 7 exception(s). View
#23 interdiff.txt599 bytestim.plunkett
#23 refactor-2392293-23.patch58.34 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#21 refactor-2392293-21.patch58.34 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#16 interdiff.txt2.36 KBtim.plunkett
#16 2392293-hook_system_info_alter-15.patch57.93 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,562 pass(es), 0 fail(s), and 6 exception(s). View
#15 2392293-hook_system_info_alter-14.patch57.77 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#15 2392293-hook_system_info_alter-required_modules-14.patch4.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,359 pass(es), 0 fail(s), and 7 exception(s). View
#9 interdiff.txt4.22 KBtim.plunkett
#9 2392293-system_info_alter-9.patch53.78 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,532 pass(es), 1 fail(s), and 5 exception(s). View
#6 2392293-system_info_alter-6.patch49.56 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,533 pass(es), 5 fail(s), and 5 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

xjm’s picture

catch’s picture

Category: Task » Bug report
Priority: Normal » Major
bircher’s picture

Issue summary: View changes
fago’s picture

As written in the dup #2392363: [META] Make use of module uninstall validators, I'd suggest to do a change notice also:
> - For communicating the new API provide a change notice and point to a nice example implementation.

tim.plunkett’s picture

Working on this.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
49.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,533 pass(es), 5 fail(s), and 5 exception(s). View

That was actually pretty fun.

I was able to fully inject everything and unit test the new classes.

tim.plunkett’s picture

The only thing I was unsure of were all the MAINTENANCE_MODE checks. All of the ones I converted had a check for that first, due it "not being safe" to use entity.query, entity_load_multiple_by_properties, or filter_formats.

I removed those checks for now. What was the concern there? Seems like you could really break your site in HEAD right now by uninstalling modules while in MAINTENANCE_MODE.

Status: Needs review » Needs work

The last submitted patch, 6: 2392293-system_info_alter-6.patch, failed testing.

tim.plunkett’s picture

Title: Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface » Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface
Status: Needs work » Needs review
FileSize
53.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,532 pass(es), 1 fail(s), and 5 exception(s). View
4.22 KB

hook_system_info_alter() doesn't interfere with direct calls to $module_handler->uninstall().
But ModuleUninstallValidatorInterface prevents that API call from working. Those tests are still failing.

Because of #2392695: Move module dependency messages from the install form to the uninstall form, we need to adjust some test expectations for where to find their required messages.
Also, these can contain HTML.

cilefen’s picture

Status: Needs review » Needs work

The last submitted patch, 9: 2392293-system_info_alter-9.patch, failed testing.

cilefen’s picture

tim.plunkett’s picture

CommentUninstallTest::testCommentUninstallWithField() ensures that a field exists, and then uninstalls comments, asserting that uninstalling comment will trigger field deletion.

Yet our new FieldUninstallValidator explicitly prevents that (and previously, field_system_info_alter() claimed it made the module required!)

So which is it?

cilefen’s picture

Should we add a @see to the docblock of hook_system_info_alter() telling developers to use ModuleUninstallValidatorInterface in cases like these?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,359 pass(es), 0 fail(s), and 7 exception(s). View
57.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Confirmed with @alexpott that the tests are wrong. Modules shouldn't be able to be uninstalled if they are required, or meet any of these conditions. To prove that these test failures aren't really the fault of this issue, here's a RequiredModuleUninstallValidator on it's own, and included in the test.

Ideally the required_modules patch will contain all of the failures of the above test fail (minus the single forum module error).

tim.plunkett’s picture

FileSize
57.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,562 pass(es), 0 fail(s), and 6 exception(s). View
2.36 KB

Let's increase the helpfulness of that message.

The last submitted patch, 15: 2392293-hook_system_info_alter-14.patch, failed testing.

The last submitted patch, 15: 2392293-hook_system_info_alter-required_modules-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: 2392293-hook_system_info_alter-15.patch, failed testing.

catch’s picture

The only thing I was unsure of were all the MAINTENANCE_MODE checks. All of the ones I converted had a check for that first, due it "not being safe" to use entity.query, entity_load_multiple_by_properties, or filter_formats.

I removed those checks for now. What was the concern there? Seems like you could really break your site in HEAD right now by uninstalling modules while in MAINTENANCE_MODE

MAINTENANCE_MODE means two or three different things:

1. That the site is 'offline' - everything should work fine in that case, and you might be uninstalling modules.

2. That you're on update.php - now that this is a route, it's the same as #1.

3. That you've hit an error condition and are in the maintenance theme - at which case we don't know how much of Drupal is working or not.

I think the check was only for the case if you hit #3, and something triggered a system info rebuild in that state. Trying to uninstall a module in the state should be impossible, so just removing the checks seems fine.

cilefen’s picture

Status: Needs work » Needs review
FileSize
58.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Reroll

Status: Needs review » Needs work

The last submitted patch, 21: refactor-2392293-21.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
58.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
599 bytes

Thanks for the reroll @cilefen! The code we're moving around has changed, adjusting for that.

Status: Needs review » Needs work

The last submitted patch, 23: refactor-2392293-23.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
58.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,053 pass(es), 0 fail(s), and 7 exception(s). View
1.26 KB

And adjusting the test as well.

Status: Needs review » Needs work

The last submitted patch, 25: refactor-2392293-25.patch, failed testing.

cilefen’s picture

This is one of the failing tests. It looks as though fields are not deleted, or not deleted before the uninstall validators execute.

  // CommentUninstallTest
  function testCommentUninstallWithField() {
    // Ensure that the field exists before uninstallation.
    $field_storage = FieldStorageConfig::loadByName('comment', 'comment_body');
    $this->assertNotNull($field_storage, 'The comment_body field exists.');

    // Uninstall the comment module which should trigger field deletion.
    $this->container->get('module_installer')->uninstall(array('comment'));

    // Check that the field is now deleted.
    $field_storage = FieldStorageConfig::loadByName('comment', 'comment_body');
    $this->assertNull($field_storage, 'The comment_body field has been deleted.');
  }
cilefen’s picture

Status: Needs work » Needs review
FileSize
899 bytes
59.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,157 pass(es), 0 fail(s), and 6 exception(s). View

Fixes ForumUninstallTest by removing the Forums vocabulary terms before module uninstall is attempted.

cilefen’s picture

+++ b/core/modules/forum/src/Tests/ForumUninstallTest.php
@@ -140,6 +140,13 @@
+    // Delete all terms in the Forums vocabulary. Uninstalling the form module

Reviewing myself: "form" should be "forum".

cilefen’s picture

FileSize
2.35 KB
60.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,244 pass(es), 0 fail(s), and 4 exception(s). View

Upends CommentUninstallTest::testCommentUninstallWithField() to expect an exception on module uninstall if a field exists.

Status: Needs review » Needs work

The last submitted patch, 30: refactor-2392293-30.patch, failed testing.

The last submitted patch, 28: refactor-2392293-28.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/comment/src/Tests/CommentUninstallTest.php
@@ -36,19 +37,22 @@ protected function setUp() {
+    } catch (ModuleUninstallValidatorException $e) {

On the next reroll/fix, please put `catch` on its own line

cilefen’s picture

In #30, CommentUninstallTest::testCommentUninstallWithoutField() is causing an exception from the validator because a field exists, but the point of the test is to have removed the field before attempting uninstall.

gobinathm’s picture

Assigned: Unassigned » gobinathm
FileSize
706 bytes
60.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,352 pass(es), 0 fail(s), and 4 exception(s). View

Updated Patch (moving catch to new line)

gobinathm’s picture

Status: Needs work » Needs review
cilefen’s picture

+++ b/core/modules/comment/src/Tests/CommentUninstallTest.php
@@ -36,19 +37,23 @@ protected function setUp() {
+    try {
+      $this->container->get('module_installer')->uninstall(array('comment'));
+      $this->fail("Expected an exception when uninstall was attempted.");
+    } ¶
+    catch (ModuleUninstallValidatorException $e) {
+      $this->pass("Caught an exception when uninstall was attempted.");
+    }

@gobinathm Thank you for the interdiff. There is just one issue: there is now a trailing space after the try{} block that must be removed. On a personal note, I am so inept at trailing spaces that I have my IDE remove them when saving.

After that, I wonder if you have any thoughts on the paradox I mentioned in #34?

Status: Needs review » Needs work

The last submitted patch, 35: refactor-2392293-35.patch, failed testing.

bircher’s picture

Assigned: gobinathm » bircher
FileSize
66.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,605 pass(es), 2 fail(s), and 4 exception(s). View

re-roll

bircher’s picture

Status: Needs work » Needs review

also re-test

Status: Needs review » Needs work

The last submitted patch, 39: 2392293-39.patch, failed testing.

bircher’s picture

Status: Needs work » Needs review
FileSize
9.28 KB
68.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,447 pass(es), 2 fail(s), and 0 exception(s). View

fixing tests

Status: Needs review » Needs work

The last submitted patch, 42: 2392293-42.patch, failed testing.

bircher’s picture

Status: Needs work » Needs review
FileSize
69.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,449 pass(es). View
941 bytes

fixed the string.

tim.plunkett’s picture

FileSize
69.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,500 pass(es). View
2.38 KB

I would consider this RTBC, but I wrote a big chunk of this. The changes since my patch in #25 are good.

One tweak. In #2454287: Make a couple of services lazy, the existing ModuleUninstallValidatorInterface classes were marked lazy. Doing the same for the new ones (and fixing the newline problem at the same time)

Wim Leers’s picture

Status: Needs review » Needs work

Just nitpicks!

  1. +++ b/core/lib/Drupal/Core/Extension/RequiredModuleUninstallValidator.php
    @@ -0,0 +1,54 @@
    +  /**
    +   * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
    +   *   The string translation service.
    +   */
    +  public function __construct(TranslationInterface $string_translation) {
    

    Missing the "Constructs new …" stuff that we have for other constructors.

  2. +++ b/core/modules/book/src/BookUninstallValidator.php
    @@ -0,0 +1,96 @@
    + * there are any book outline stored.
    

    s/are/is/

  3. +++ b/core/modules/book/src/BookUninstallValidator.php
    @@ -0,0 +1,96 @@
    +  /**
    +   * @param \Drupal\book\BookOutlineStorageInterface $book_outline_storage
    +   *   The book outline storage.
    +   * @param \Drupal\Core\Entity\Query\QueryFactory $query_factory
    +   *   The entity query factory.
    +   * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
    +   *   The string translation service.
    +   */
    +  public function __construct(BookOutlineStorageInterface $book_outline_storage, QueryFactory $query_factory, TranslationInterface $string_translation) {
    

    See earlier.

  4. +++ b/core/modules/book/src/BookUninstallValidator.php
    @@ -0,0 +1,96 @@
    +        // The book node type is provided by the Book module. Prevent uninstall if
    

    80 cols.

  5. +++ b/core/modules/book/src/BookUninstallValidator.php
    @@ -0,0 +1,96 @@
    +  protected function hasOutlineBooks() {
    

    hasBookOutlines()

    ?

  6. +++ b/core/modules/config/src/Tests/ConfigImportAllTest.php
    @@ -108,6 +108,12 @@ public function testInstallUninstall() {
    +    // Can not uninstall editor and its dependencies as it provides filter plugins.
    

    80 cols.

    s/editor/Editor/

  7. +++ b/core/modules/field/src/FieldUninstallValidator.php
    @@ -0,0 +1,78 @@
    +   * @var \Drupal\Core\Entity\EntityStorageInterface
    

    Should be \Drupal\Core\Config\Entity\ConfigEntityStorageInterface.

  8. +++ b/core/modules/field/src/FieldUninstallValidator.php
    @@ -0,0 +1,78 @@
    +  public function __construct(EntityManagerInterface $entity_manager, TranslationInterface $string_translation) {
    

    Again here.

  9. +++ b/core/modules/field/src/FieldUninstallValidator.php
    @@ -0,0 +1,78 @@
    +      // Provide an explanation message (only mention pending deletions if there
    +      // remains no actual, non-deleted fields)
    

    s/remains/remain/

    Missing trailing period.

  10. +++ b/core/modules/field/src/FieldUninstallValidator.php
    @@ -0,0 +1,78 @@
    +        $reasons[] = $this->t('Fields type(s) in use');
    ...
    +        $reasons[] = $this->t('Fields pending deletion');
    

    The reasons listed for book have trailing periods, these should too.

  11. +++ b/core/modules/filter/src/FilterUninstallValidator.php
    @@ -0,0 +1,101 @@
    + * Prevents uninstallation of modules that provide filter plugins that are being
    + * used in a filter format.
    

    Prevents uninstallation of modules providing used filter plugins.

    (This makes it fit on a single line.)

  12. +++ b/core/modules/filter/src/FilterUninstallValidator.php
    @@ -0,0 +1,101 @@
    +   * @var \Drupal\Core\Entity\EntityStorageInterface
    

    \Drupal\Core\Config\Entity\ConfigEntityStorageInterface

  13. +++ b/core/modules/filter/src/FilterUninstallValidator.php
    @@ -0,0 +1,101 @@
    +        $reasons[] = $this->t('Provides a filter plugin that is in use in the following filter formats: %formats', ['%formats' => implode(', ', $used_in)]);
    

    Trailing period in reason, just like elsewhere.

  14. +++ b/core/modules/forum/src/ForumUninstallValidator.php
    @@ -0,0 +1,137 @@
    +   * @var \Drupal\Core\Entity\EntityStorageInterface
    

    \Drupal\Core\Config\Entity\ConfigEntityStorageInterface

  15. +++ b/core/modules/forum/src/ForumUninstallValidator.php
    @@ -0,0 +1,137 @@
    +        $reasons[] = $this->t('To uninstall Forum first delete all <em>Forum</em> content.');
    ...
    +          $reasons[] = $this->t('To uninstall Forum first delete all <a href="!url">%vocabulary</a> terms.', [
    ...
    +          $reasons[] = $this->t('To uninstall Forum first delete all %vocabulary terms.', [
    

    s/Forum first/Forum, first/

bircher’s picture

Status: Needs work » Needs review
FileSize
12.6 KB
69.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Status: Needs review » Needs work

The last submitted patch, 47: 2392293-57.patch, failed testing.

bircher’s picture

Status: Needs work » Needs review
FileSize
69.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,387 pass(es), 2 fail(s), and 0 exception(s). View
3.4 KB

when adding commas and renaming the function the unit test needs to be updated too of course...

Status: Needs review » Needs work

The last submitted patch, 49: 2392293-49.patch, failed testing.

bircher’s picture

Status: Needs work » Needs review
FileSize
69.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,425 pass(es). View

catching more commas...

catch’s picture

bircher’s picture

FileSize
69.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

simple re-roll since the old patch doesn't apply any more.

error: patch failed: core/core.services.yml:430
error: core/core.services.yml: patch does not apply

Status: Needs review » Needs work

The last submitted patch, 53: 2392293-53.patch, failed testing.

bircher’s picture

Status: Needs work » Needs review
FileSize
69.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,082 pass(es). View
4.15 KB

Replacing String::format() with SafeMarkup::format() makes the unit tests pass.

dawehner’s picture

  1. +++ b/core/modules/book/src/BookUninstallValidator.php
    @@ -0,0 +1,98 @@
    +        $reasons[] = $this->t('To uninstall Book, delete all content that is part of a book.');
    ...
    +          $reasons[] = $this->t('To uninstall Book, delete all content that has the Book content type.');
    

    Should we provide a link to admin/content?

  2. +++ b/core/modules/field/src/FieldUninstallValidator.php
    @@ -0,0 +1,80 @@
    +        $reasons[] = $this->t('Fields type(s) in use.');
    ...
    +        $reasons[] = $this->t('Fields pending deletion.');
    

    Should we explain which field types are in use?

tim.plunkett’s picture

  1. +++ b/core/modules/book/book.module
    @@ -576,33 +576,3 @@ function book_node_type_update(NodeTypeInterface $type) {
    -      $info['explanation'] = t('To uninstall Book, delete all content that is part of a book.');
    ...
    -        $info['explanation'] = t('To uninstall Book, delete all content that has the Book content type.');
    
    +++ b/core/modules/field/field.module
    @@ -146,40 +146,6 @@ function field_cron() {
    -        $explanation = t('Fields type(s) in use');
    ...
    -        $explanation = t('Fields pending deletion');
    

    These are existing strings, I don't think this issue is the place to improve them.

dawehner’s picture

These are existing strings, I don't think this issue is the place to improve them.

I'm sorry I wasn't aware of that.

Here a general question: Is it just me or do the various unit tests just ensure that the code written like that, is written like that? For me, it feels wrong to have not integration level testing.

alexpott’s picture

FileSize
25.18 KB
74.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,219 pass(es), 1 fail(s), and 0 exception(s). View

@dawehner there is integration level testing - for example ForumUninstallTest. I've reintroduced BookUninstallTest so we don't lose integration testing there either.

Also I discovered a problem with escaped html on the modules uninstall page. Considering all the validation reasons are run through t() i think it is safe to do:

array('@module' => $form['modules'][$module]['#module_name'], '!reasons' => SafeMarkup::checkAdminXss(implode('; ', $form['modules'][$module]['#validation_reasons']))));

when we add them together for display.

Also in the same part of the code because we're adding them together with a ; I think we need to remove all the final punctuation from the validation messages.

I've also re-instated the test in StandardTest to ensure editor can be uninstalled and re-installed. Plus I've fixed ConfigImportAllTest so it can disable all modules apart from Standard, System and User.

Status: Needs review » Needs work

The last submitted patch, 59: 2392293.59.patch, failed testing.

catch’s picture

Also I discovered a problem with escaped html on the modules uninstall page. Considering all the validation reasons are run through t() i think it is safe to do:

array('@module' => $form['modules'][$module]['#module_name'], '!reasons' => SafeMarkup::checkAdminXss(implode('; ', $form['modules'][$module]['#validation_reasons']))));

when we add them together for display.

Also in the same part of the code because we're adding them together with a ; I think we need to remove all the final punctuation from the validation messages.

Only issue with this is it could look odd for RTL languages or any that normally wouldn't use a semicolon. I can only really think of an unordered list or similar as an alternative though. Not sure how that'd end up looking.

Didn't do an in depth review of the patch yet, but overall it looks good to me.

alexpott’s picture

Status: Needs work » Needs review
Related issues: +#2494987: [meta-6] Reduce cold cache memory requirements
FileSize
1.67 KB
75.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,238 pass(es). View

Fixed test and made the exception message use the ; reason separator to be consistent with the uninstall modules page.

Doing this should also reduce memory usage on #2494987: [meta-6] Reduce cold cache memory requirements because rebuilding the system module data will be cheaper.

alexpott’s picture

Issue summary: View changes
FileSize
131.63 KB
118.5 KB
5.06 KB
77.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,223 pass(es), 1 fail(s), and 0 exception(s). View

I think it looks great with an unordered list. Much much easier to read...

versus

Status: Needs review » Needs work

The last submitted patch, 63: 2392293.63.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
956 bytes
77.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,919 pass(es). View

Fixing the test failure.

bircher’s picture

Assigned: bircher » Unassigned

It looks very nice with the unordered list.

catch’s picture

diff --git a/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
index 5e62b0e..eeabca5 100644
--- a/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
+++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
@@ -20,6 +20,10 @@
    *
    * @return string[]
    *   An array of reasons the module can not be uninstalled, empty if it can.
+   *   Each reason should not end with any punctuation since multiple reasons
+   *   can be displayed together.
+   *
+   * @see theme_system_modules_uninstall()
    */
   public function validate($module);

So I think this is pretty much ready, but if we're using the unordered list, do we still need to make this change? I can see we might still implode them in the exception message but that's not end-user facing. On the other hand it doesn't do any harm really.

Apart from that question and one more look over, I think this is RTBC.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Discussed with alexpott in irc: he thinks it's useful to keep this restriction for the exception messages or if we used them in other contexts.

That's fair enough, so RTBC from me.

dawehner’s picture

Issue summary: View changes

I really like how this issue moves an implicit "API" to an explicit API

  • catch committed 5803f90 on 8.0.x
    Issue #2392293 by bircher, tim.plunkett, alexpott, cilefen, gobinathm:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

#69 yes I really like that as well. It's nice to get from the 7.x stop-gap we added for field module (marking itself required) to this.

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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