Improve hook_system_info_alter documentation to mention ModuleUninstallValidatorInterface

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -84,13 +84,18 @@ function hook_module_implements_alter(&$implementations, $hook) {
+ * Drupal\Core\Extension\ModuleUninstallValidatorInterface to mark a module as

The interface reference needs a leading slash "\".

#2 suggestion incorporated.

Hm, I'm a bit confused here. How exactly would you use this interface to mark something as required? There's no documentation there. I'm assuming you'd have to define a service of some type or something like that? This needs to be better explained rather than just giving a link, I think, if you want people to understand this as an alternative.

Also I don't think the docs here really explain why it's better to use the interface, and how the two options behave. Because defining a service and/or class seems like a lot of developer overhead for a hook that I think would only be called rarely... And does it work exactly the same? For instance, if you went the interface route, would the marked module show up on the Uninstall page as something is not uninstallable, or would it cause them to try to uninstall and only later find out it can't work? That would not be good from a UI perspective.

Yes, there should be documentation and an example for developers on how to actually do this. It does not belong here, though, but in a place more specific to the ModuleUninstallValidator. Does anyone has suggestions on that (maybe in ModuleUninstallValidatorInterface)?

True on not explaining why this is discouraged. We ought to do that.

Yes, these show up as ordinary dependencies on the module uninstall UI with the text of the explanation provided in the implementation of ModuleUninstallInterface. For example "This module cannot be uninstalled right now because module Foo contains content that depends on it.".

Agreed that it doesn't belong in the hook docs, but the hook docs should link to wherever it does belong... on the interface that is linked to seems like a logical place.

#2392293: Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface will help whoever works on this issue.

Added a link to Drupal\Core\Extension\ModuleUninstallValidatorInterface and provided some more information on how to use this instead of $info['required'].
This is part of a Drupal code sprint. (DUGBE0609). Disclaimer: This is my first patch.

This seems like a useful clarification, considering the slowdown this causes on module install and uninstall.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -14,6 +14,13 @@
   +   * For example: can be used to replace $info['required'] to see if a module can
   

   Needs a newline after 80 cols and a newline after the short description.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -14,6 +14,13 @@
   +   *  $reasons[] = $this->t('Can never be uninstalled.');
   

   Needs to be indented by two spaces.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -14,6 +14,13 @@
       *
   

   Missing @endcode.

4. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -131,13 +131,19 @@ function hook_module_implements_alter(&$implementations, $hook) {
   + * Alter the information parsed from module and theme .info.yml files.
   

   Ah, nice catch!

5. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -131,13 +131,19 @@ function hook_module_implements_alter(&$implementations, $hook) {
   + * is discouraged. See \Drupal\Core\Extension\ModuleUninstallValidatorInterface for more
   + * information and examples on how to use this.
   

   80 cols, we also already have the @see at the bottom so this last sentence may not be needed.

6. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -14,6 +14,13 @@
   +   * For example: can be used to replace $info['required'] to see if a module can
   +   * be deleted.
   

   How about "This can be used to replace $info['required'] in hook_system_info_alter() to mark a module as required.", just to clarify what we are talking about when we say $info?

7. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -14,6 +14,13 @@
   +   *  $reasons[] = $this->t('Can never be uninstalled.');
   

   Maybe "The example module can never be uninstalled" would clarify this slightly?

8. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -131,13 +131,19 @@ function hook_module_implements_alter(&$implementations, $hook) {
   + * Note that it is preferable for performance reasons to use
   

   Let's clarify how this affects performance?

Updated taking the review in comment #11 into account.

Looks good now, thanks!

Adding beta evaluation

Code wrapping wasn't optimal for the hook_system_info_alter docblock, attached patch fixes this. I'm leaving this set to RTBC though.

Unrelated test fail: Link to the Drupal project appears. Other UpdateTestBase.php 83 Drupal\update\Tests\UpdateTestBase->standardTests()
"Up to date" found Other UpdateCoreTest.php 59 Drupal\update\Tests\UpdateCoreTest->testNoUpdatesAvailable()
Check icon was found.

+++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
@@ -15,6 +15,15 @@
+   * For example, this can be used to replace $info['required'] in
+   * hook_system_info_alter() to mark a module as required.
+   *
+   * @code
+   * if ($module == 'example') {
+   *   $reasons[] = $this->t('The example module can never be uninstalled');
+   * }
+   * @endcode
+   *

I think this example is just ... ok. We could use a better one.

+++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
@@ -15,6 +15,15 @@
+   * For example, this can be used to replace $info['required'] in
+   * hook_system_info_alter() to mark a module as required.

We have already been directed to this documentation as the replacement to hook_system_info_alter(). This is actually the official place to mark a module as required, so the wording should be more forceful and no need to mention hook_system_info_alter().

Both good points! How about just "This can be used to mark a module as required" followed by an example?

As to the example, usually a module won't be required unconditionally, maybe have a look at some of the implementations of ModuleUninstallValidatorInterface - usually this is because there is existing content provided by the module so let's incorporate that in the example.

How about just "This can be used to mark a module as required" followed by an example?

I think a clear example would eliminate the need for the full function description but let's see.

Looks good now!

Hm. I'm not really sure this documentation is quite ready yet...

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -131,13 +131,18 @@ function hook_module_implements_alter(&$implementations, $hook) {
+ * process. Use \Drupal\Core\Extension\ModuleUninstallValidatorInterface to mark
+ * a module as required.
+ *

Um, so how would you do this? Is it a particular method that you would use to "mark a module as required"? If so, refer to that method, maybe. There is zero documentation on that interface telling what you need to do in order to make your class implementing this interface be recognized either -- presumably it's not just a matter of making a class that implements the interface and putting it in some random place. So I think this is really incomplete.

You are so right.

Um, so how would you do this? Is it a particular method that you would use to "mark a module as required"? If so, refer to that method, maybe.

It is actually even more than that. You must create a service definition too.

@jhodgdon So we need a real "how do you use this whole thing" docblock. Would that kind of thing go in the class's block?

I think if we put a sample example.services.yml definition on the interface and move the documentation to the interface docs themselves instead of to the validate() method - and link to that in the @see, this will be a bit clearer:

services:
  example.uninstall_validator:
    class: Drupal\example\ExampleUninstallValidator
    tags:
      - { name: module_install.uninstall_validator }
    arguments: ['@entity.manager', '@string_translation']
    lazy: true

Yes, it seems like the "how to use this interface" documentation belongs in the interface, and yes I would put it at the interface doc block level rather than on the validate() method. The validate() method docs should describe how to use the validate() method, not how to define a service.

I do not think you should put a sample services.yml block there. Just say that you need to define a service tagged "module_install.uninstall_validator" and link to the Services topic (which has the details about how to define a service). If there is an example class/service in Core and a *.services.yml file that has an example, that could also be useful to link to.

This is not finished but I thought I'd share my thoughts.

Looking good! A few thoughts/nitpicks on what is in the current patch:

1. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -9,12 +9,34 @@
   + * a Drupal service (see the @link container Services and container
   + * topic @endlink) that is tagged module_install.uninstall_validator.
   

   @link ... @endlink all needs to be on the same line. The 80 character wrapping can be off so that this happens.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -9,12 +9,34 @@
   +   * This method must either return a list of reasons why a module cannot be
   +   * uninstalled in the current state of the system, or return an empty list,
   +   * which means that it is safe to uninstall the module.
   

   This should be combined with the existing @return documentation for this method, which pretty much actually already says this. Documentation about the return value belongs in the @return anyway.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -9,12 +9,34 @@
   +   * Example usage:
   

   This is not really an example *usage* but an example *implementation* of the interface method, right?

4. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -131,13 +131,18 @@ function hook_module_implements_alter(&$implementations, $hook) {
   + * Setting the $info['required'] key in implementations of this hook is
   + * discouraged as this will slow down the module installation and uninstallation
   + * process. Use \Drupal\Core\Extension\ModuleUninstallValidatorInterface to mark
   + * a module as required.
   

   I'm guessing/hoping this is the "not finished" part of the patch. ;)

#30-1 Is this shortened version ok with everybody?
#30-2 I wonder if we need a full description at all.
#30-3 ok
#30-4, I don't know what we are trying to communicate in module.api.php.

#30-1 Is this shortened version ok with everybody?
==> Yes (for me anyway).

#30-2 I wonder if we need a full description at all.
==> Probably not.

#30-3 ok
==> Looks good.

#30-4, I don't know what we are trying to communicate in module.api.php.

==> Well. That was the whole point of this issue. ;) What I think needs to get communicated (quoting from the issue summary):
- Using hook_system_info_alter() to set a module as required is surprisingly expensive [current patch does this]
- What people should do instead [current patch doesn't really do this IMO]

The problem is that the current patch points people to the interface, but doesn't really explain how to mark a module as "required" by another module. I mean... I can kind of see how you would do it, but maybe in the alter hook docs we should say something like:

Instead, define an uninstall validator (see \Drupal\Core\Extension\ModuleUninstallValidatorInterface for details) and return a message saying that your module is required...

Hm... Actually, this doesn't even make sense to me. When I tried to write the preceding lines, I ran into a conceptual problem.

It seems to me that hook_system_info_alter() provides a (bad) way for my module B to alter some other module A's information to say that module A requires module B. Because if I wanted to say that my module B requires module A, I would just put that into the module_b.info file, no alter hook required. Right?

But ... ok, it is starting to make more sense, but I'm not sure how to word it. So the uninstall validator way to do this would be if module B was being checked to see whether it could be uninstalled, you would see if module A was installed, and output some message saying that module A requires it.

Right?

So, that is what we want to communicate, I believe... really hook_system_info_alter() seems much more maintainable and understandable. Who cares if module install/uninstall is a bit slower really? How often do you really do that?

But ... ok, it is starting to make more sense, but I'm not sure how to word it. So the uninstall validator way to do this would be if module B was being checked to see whether it could be uninstalled, you would see if module A was installed, and output some message saying that module A requires it.

Both the hook and the new interface are for runtime checking, so these are for checking more complicated situations, like "if this other module is installed and has content, then I can't be uninstalled".

Expanding on #33: If a module flat-out requires another module, that goes in the .info file.

Well the problem is you cannot put in module B's module something like "Module A should really require me", which is why module B would consider implementing hook_system_info_alter() right?

I would say it is more like "Module A requires module B in some situations, but not by default."

OK. I still don't think that the text added in the hook docs really tell you what to do...

I totally agree. I wasn't making a case that it does.

OK, well this is pretty similar to the previous patch... I still am not sure that it tells me how to replace setting the required bit. Maybe if the example code in the interface did this, I would be more convinced... or maybe it's obvious? It wasn't to me, especially since the example code is fairly generic -- seems to be instituting a check across all entity types, which presumably the entity system is already doing for us. So it is not so useful for a developer who needs to do something more specific.

So it seems to me really that the reason you'd want to use the interface is that it allows you to have more nuanced messages rather than just saying that a module is required.

Also, it doesn't really have exactly the same result, does it? We're talking about my module B wanting to say that another module A should require B, in the hook. Whereas the interface (defined by module B) allows me to say things like:
- Module B (myself) shouldn't be uninstalled because there's something on the system that needs me
- Module A (another module) shouldn't be uninstalled because I need it [I would just put this in the .info file but it's contingent on something else on the system presumably]
- Module B (myself) shouldn't be uninstalled because module A really needs me and doesn't know it by itself [this is the hook case, I think?]
- A bunch of modules shouldn't be installed because there is something on the system that needs them (this is the case illustrated by the example in the patch]

So I don't think the example is really appropriate for this issue, which is about what to put in the hook docs.

So it seems to me really that the reason you'd want to use the interface is that it allows you to have more nuanced messages rather than just saying that a module is required.

In addition, setting $info['required'] is a performance hit of some kind.

OK. Well, can we have an example that illustrates how to do the equivalent of $info['required'][] = 'module_name' then (which is what this issue is about documenting), rather than the example that is currently in this patch, or is that a bad idea for some reason?

The equivalent to setting $info['required'] is the example in this patch.

This example from RequiredModuleUninstallValidator would seem like a great example but it really is not:

 /**
   * {@inheritdoc}
   */
  public function validate($module) {
    $reasons = [];
    $module_info = $this->getModuleInfoByModule($module);
    if (!empty($module_info['required'])) {
      $reasons[] = $this->t('The @module module is required', ['@module' => $module_info['name']]);
    }
    return $reasons;
  }

This is now the place where the required key is forward-ported to the new API. No developer is ever going to implement this API in this way, because it is already done. It would be much more common to see something like this (from ContentUninstallValidator):

  /**
   * {@inheritdoc}
   */
  public function validate($module) {
    $entity_types = $this->entityManager->getDefinitions();
    $reasons = array();
    foreach ($entity_types as $entity_type) {
      if ($module == $entity_type->getProvider() && $entity_type instanceof ContentEntityTypeInterface && $this->entityManager->getStorage($entity_type->id())->hasData()) {
        $reasons[] = $this->t('There is content for the entity type: @entity_type', array('@entity_type' => $entity_type->getLabel()));
      }
    }
    return $reasons;
  }

Oh duh. I must have been still on vacation yesterday. I was confusing 'required' with "depends on another module", instead of "don't uninstall this". Doh!

So this looks good. Thanks, and sorry for my confusion!

Committed and pushed to 8.0.x. Thanks!