hook_help should be moved and should explain how help is displayed

Apparently this is true in D7 as well. So this will need a backport.

I just filed a meta-issue where we need to discuss the top-of-page help:
#2263359: hook_help(): Top of page help sections can't link to help pages without a fatal error or checking for help module

Please do not put D7 patches on D8 issues. Read:
https://drupal.org/node/1319154#multiple-versions

The D8 failures... seem unrelated.

So I reviewed the patch. It is mostly OK but:

a) The first line -- that is not accurate. hook_help() is used for both page-level and module-level help.

b) In several places, there are two spaces between sentences. There should only be one.

c) I think in the first paragraph we should say "for specific routes" instead of "for specific paths".

d)

+ * The help information provided by this hook appears as a system help block
+ * on the page. Please note that the modules overview help page at admin/help
+ * is generated by the Help module.

That first sentence is not accurate -- only the page-specific help appears in the system help block. And the second sentence should not say "Please note that" -- it should just say that the Help module (if enabled) displays the module-level help... actually you will get a link on admin/help and also a link from the Extend page.

e)

+ *   For a specific page, use the route name as identified in the module's
+ *   routing.yml file. For the help overview page, the route name will be in the
+ *   form of "help.page.$modulename".

How about here saying "for page-specific help" and "for module overview help"? I think that would be clearer.

f)

+ * @param \Symfony\Component\HttpFoundation\Request $request
+ *   The current request.

Maybe it would be helpful to add that this can be used in conjunction with the route to further define what help should be displayed for page-specific help?

g) The example code is not all that great. The module-level help is not a good example (it doesn't follow our established standards), and the route-specific example does not include an example that uses the $request parameter. Could we:
1. Copy in a module-level help that follows our guidelines (possibly somewhat simplified)? There is one on the http://drupal.org/node/632280 help guidelines page.
2. Add an example that uses the $request parameter? Several core modules are doing this... node.module is one example. You can mix examples from different modules -- just add a comment explaining what the purpose of each example is and where it comes from.

Thanks for the new patch! Generally you should make an interdiff file when you make a new patch, especially if you made a lot of changes or the patch is large, or both. See https://drupal.org/documentation/git/interdiff

Anyway, I reviewed the new patch. It looks really good!

The only things I would change are fairly minor (mostly grammar/wording):

a) When referring to specific examples, I think instead of saying "see block.module" I would say "see block_help()", because this will link them to the actual function that provides the help. (same for content_translation.module ==> content_translation_help())

b) The first paragraph talks about help for specific routes, and the 2nd starts talking about page-specific help (which is the usage in the rest of the help). So I think in the first paragraph, we should change "or for specific routes" to say "or for specific pages".

c)

+ * For the detailed usage examples of:
+ * - The module overview help, see content_translation.module. The module
...
+ * - The page-specific help using only routes, see book.module.
+ * - The page-specific help using routes and $request, see block.module.

Remove "the" in all 4 lines.

d)

+ *   overview help should follow a certain standard structure.
+ *   @see https://drupal.org/node/632280

You cannot put @see in the middle of the help, and so this should be made into a link, something like:

... overview help should follow
@link https://drupal.org/node/632280 the standard help template. @endlink

e)

+ *   For the page-specific help, use the route name as identified in the
+ *   module's routing.yml file. For the module overview help, the route name
+ *   will be in the form of "help.page.$modulename".

Remove "the" in "For the" in both sentences.

f)

+ *   The current request. This can be used to generate a customized
+ *   page-specific help information.

information is not countable, so the grammar here is not correct ... I think the second sentence should be something like this:

This can be used to generate different help output for different pages that share the same route.

Looks good to me, thanks!

Oh... I think that after moving this hook, there is nothing left in help.api.php, so it should be removed?

Thanks! I think we're good to go now.

Thanks again all! Committed to 8.x.

We also need to backport to 7.x. Keep in mind that hook_help is a bit different in 7.x (uses paths and not routes, and the first argument to indicate the module-level help is not the same).

Could we fix this formatting problem (not introduced by this patch, but present in the existing hook_help() documentation for D7) please?

+ *   To provide a help page for a whole module with a listing on admin/help,
+ *   your hook implementation should match a path with a special descriptor
+ *   after a "#" sign:
+ *     'admin/help#modulename'
+ *       The main module help text, displayed on the admin/help/modulename
+ *       page and linked to from the admin/help page.

I think that the last two lines could just be removed here (not needed), and the line before that can be moved up to the end of the previous line.

Maybe this should just say, "For the help page for the module as a whole, $path will have the value 'admin/help#module_name', where 'module_name" is the machine name of your module.'

I think this would be sufficient and clearer, given the added documentation above that explains the dual purpose of hook_help?

Looks good, thanks!

Thanks, amit! You are right, and I missed those! But rather than going back now on this issue, we have another issue open to update the system help on #2091363: Update hook_help for System module, so we can just fix it there. The rest of the system help suffers from the same problems.

Meanwhile, committed the patch in #23 to Drupal 7.

Oh wait, in #25 you are talking about the hook_help() docs, not the system_help() function. Yes, let's go back to Drupal 8 and fix that problem. We shouldn't have an example in the hook_help() docs that sets a bad example!

RE #30, this is an example of the main help from the block module, so we do not need to check if the block module is enabled.

The patch in #29 is fine, though. I committed it to 8.x and I think this issue is (again :) ) done. Thanks!