Don't render main help pages on modules page just to generate help links - can lead to high memory usage on form submit

I've checked all the core implementations of hook_help() and all provide a module help so I think the patch attached is the way to go.

Yeah and in case they don't, so what, its not like a huge problem.

+1

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -255,17 +255,16 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
     if ($this->moduleHandler->moduleExists('help') && $module->status && in_array($module->getName(), $this->moduleHandler->getImplementations('help'))) {

Looks like getImplements already cover the part that module has a hook. Not sure why we need to check if we really have content to display help icon.

Because hook_help() has two different purposes. One is about displaying a generic help text about the module and the other is providing context specific help for specific pages. This checks that it really provides such a help text.

Might make sense to run this by @jhodgdon... I know that it's a documentation gate for core modules to have such a help page, but it's also a small UX regression if we point people to non-existing documentation pages.

I think that this is OK, especially if #2488032: Integrate help test into module uninstall test gets in, which will require a help page to exist for all visible modules. (Hint hint, that one needs a review.)

Although #13 would only apply to Core. Other modules are free to have hook_help() for top-of-page-help-block help, and not have a main help topic page. Anyway I also agree that if the page 404s it's not the end of the world, as long as the link maker doesn't crash on field_help() and make it have an exception or something like that. Can we check that -- like delete one of the hook_help() main topic sections for a field module, such as Text or Telephone or whatever we still have as a stand-alone module, and verify the Field help page can still be loaded?

Does that make sense? What I'm asking is: If for instance telephone_help() exists but it doesn't have a main help topic page as part of the implementation, will the field_help() page still load? If not then this is not a good solution, because it breaks Field help page. If so then I think it's OK because the worst that will happen is someone clicks on a link that's broken for a contrib field module help page.

What exactly do you mean with "still load"? If you go to admin/help/somemodule and somemodule doesn't have a help text, then you get "No help is available for module XY." And if it doesn't have a hook_help() implementation at all, then you get a 404.

Good. I was more concerned that with the change to field_help(), if a module had a hook_help() but not a main help page topic within it, that the Field help page might have an error.

OK so my comments above were off the mark because I didn't look carefully enough at what this patch was doing. I was assuming an issue that had field_help() in the title would be fixing field_help(), not just making sure it wasn't actually called during the Modules page generation.

Now that I've looked at the patch more carefully... I have a couple of questions/comments, so sorry but I think this needs a bit more work at least a bit more review and testing.

1. +++ b/core/modules/help/help.api.php
   @@ -22,7 +22,8 @@
   + * Extend page. If a module implements hook_help() the help system expects
   + * module help to be provided.
     *
   

   Just above here, in this docs the overview page for a module is called the "module overview help" not "module help". Let's keep the wording consistent in the docs for this hook?

2. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -255,17 +255,16 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +    // Generate link for module's help page, assume there is one since firing
   +    // hook_help() for each module can be costly.
        $row['links']['help'] = array();
   

   Total nitpick: this should be two sentences. As it is, it's a comma splice.

   And without the context of what used to be in this module... really this comment will not make much sense. Maybe it should be changed to say something like "Assume that if a hook_help() implementation exists then the module provides an overview page, rather than checking to see if the page exists, which is costly."

   But see... I think we put this check in for a reason. If there is a hook_help() implementation but no module overview in it, will this link generation step crash? Has someone actually tested this by making a module with a hook_help() that lacks an overview page, and verified that the module overview page still loads?

Hi @jhodgdon, tried what you asked on #18 and looks like working fine. Here is the steps I followed:

A. Without patch:
1. Enabled action module.
2. Checked module page - can see the link and it takes me to action help page(admin/help/action) which has help text.
3. Removed 'help.page.action' case in action_help().
4. Checked module page - Don't see the 'help link' anymore.

B. With patch:
1. Enabled action module.
2. Checked module page - can see the link and it takes me to action help page(admin/help/action) which has help text.
3. Removed 'help.page.action' case in action_help().
4. Checked module page - Still can see the 'help link' and it takes me to action help page(admin/help/action) which has help text 'No help is available for module Actions.'.

Fixed both documentation comments as well.

Also I opened #2495363: field_help() uses nearly 10mb of memory as it tries to render all modules' fields, widgets and formatter as a follow-up of this issue.

So the HelpController already has code to cope with a module implementing hook_help() but not providing a module overview page.

      $temp = $this->moduleHandler()->invoke($name, 'help', array("help.page.$name", $this->routeMatch));
      if (empty($temp)) {
        $build['top']['#markup'] = $this->t('No help is available for module %module.', array('%module' => $module_name));
      }
      else {
        $build['top']['#markup'] = $temp;
      }

Patch attached adds tests for this and changes admin/help to work the same way as the module list page.

There is a point to displaying a module overview help page even if it does not provide one - there is code to display the module's admin links on this page - unfortunately this is broken - #2495657: Admin links are missing from a module's help page