Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface

Would fix #2387983: PluginNotFoundException when enabling module that provides text filter which is a major bug, so bumping priority.

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.

Working on this.

That was actually pretty fun.

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

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.

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.

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

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?

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

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

Let's increase the helpfulness of that message.

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.

Reroll

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

And adjusting the test as well.

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.');
  }

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

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

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

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

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.

Updated Patch (moving catch to new line)

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

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)

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/

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?

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.

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.

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

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.

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.

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

versus

Fixing the test failure.

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.

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.

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

#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!

A small followup is required I think #2510310: Improve hook_system_info_alter documentation to mention ModuleUninstallValidatorInterface