Help block should not be displayed when hook_help() implementations return an empty string

This entire test and patch looks great, except ...

+++ b/core/modules/help/tests/modules/help_page_test/help_page_test.module
@@ -17,6 +17,15 @@ function help_page_test_help($route_name, RouteMatchInterface $route_match) {
+function template_preprocess_block_help(&$variables) {
+  $a = 1;
 }

What is this function? Maybe it was some debugging code you left in by mistake?

+++ b/core/modules/help/src/Plugin/Block/HelpBlock.php
@@ -97,10 +97,12 @@ protected function getActiveHelp(Request $request) {
+        if (!is_array($item)) {
+          $item = ['#markup' => $item];
+        }
+        $build[] = $item;

Minor but not needed to reassign $item? This also feels like it reads cleaner:

$build[] = is_array($item) ? $item : ['#markup' => $item];

I completely agree this is a non-disruptive change -- it is changing the API behavior to be what everyone would expect it to be, not to mention what it was in D7.

Fixed the title, which had gotten reversed from what it should have been.

The patch looks great, except for a number of comment nitpicks:

1. +++ b/core/modules/help/src/Tests/HelpBlockTest.php
   @@ -0,0 +1,56 @@
   + * Tests display of help block
   

   nitpick: needs to end in .

2. +++ b/core/modules/help/src/Tests/HelpBlockTest.php
   @@ -0,0 +1,56 @@
   +    // Login the root user to ensure as many admin links appear as possible on
   

   nitpick: Log in is a verb; login is a noun/adjective.

3. +++ b/core/modules/help/src/Tests/HelpBlockTest.php
   @@ -0,0 +1,56 @@
   +    // Login the root user to ensure as many admin links appear as possible on
   +    // the module overview pages.
   +    $this->drupalLogin($this->adminUser);
   

   This comment is also not accurate. We're not using the root user (which would be user 1), but an admin user created in the setUp() method.

4. +++ b/core/modules/help/tests/modules/help_page_test/help_page_test.module
   @@ -17,6 +17,11 @@ function help_page_test_help($route_name, RouteMatchInterface $route_match) {
   +  // to display
   

   nitpick: this should end in .

Yeah, the test module gives TRUE as access to those pages. Much cleaner.

So I think this is ready to go, assuming the test bot agrees on the latest patch. Would it make sense to upload a test-only patch just to make sure it still fails in its current lean-and-mean state? I think it would... so here is one (I just edited the patch file from #13 and took out the HelpBlock portion).

Assuming this one fails, and the one in #13 passes, we're good to go. Hopefully having this patch here will not cause the test bot to test it over and over and set the issue to Needs Work.

So this is not really a contributed project blocker because it can be worked around by returning NULL. I'd call it major though because of the unexpected visual regressions.

Makes sense to do this in RC - it's user-facing and at least with the current patch can't be fixed in a patch release.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!