Help text from hook_help() is displayed on 404/403 pages

Here's a first-pass at the patch.

#2: hook_help_404-2073121-2.patch queued for re-testing.

#2: hook_help_404-2073121-2.patch queued for re-testing.

Thanks for the feedback, I'll fix it.

@Cotter, I have removed the extra blank line and created a separate patch just for the test.

Oh, and ignore comment #9 I slipped and clicked on the save button. :(

I have testede it and it works as expected. But the test doesn't fail as expected due to the fact that the simpletest run on the admin page only get the content region and the rest of the region is empty. And therefor the test won't fail. I would mark this review "Reviewed and testede by the community" but not sure when the test doesn't work. What is the procedure?

I agree a failing test would be nice here.

Tentatively raising to major. While this doesn't expose anything too badly in Drupal core, it's possible custom modules add sensitive, admin-specific help text and showing that to anonymous users would be un-good.

Not sure how to make the test fail as well. I think this is because the tests uses the Stark theme and "System help" block isn't placed (at least when I tried it). Anyway, just re-rolling the patch.

Closed #2140315: The implementation of block_help does not access check the path 'admin/structure/block'; anon users see the help text as a duplicate.

Setting to "Needs review" so that testbot can run test for patch in #13.

I tried this with simplytest and it does not seem to fix the problem for admin/structure/block.

Also, node system uses hook_help() to display submission guidelines on node/add and node/edit. I tried this and could see the guidelines on access denied page.

#1969270: 403/404 pages: drupal_get_http_header('Status') returns no status code at all looks related.

In general using current_path() is also probably wrong, you should better get it from the request object directly.

I'v replaced curent_path() and found way how to obtain Status. Not sure my fix is good but it works.

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemHelpBlock.php
   @@ -78,6 +78,10 @@ public static function create(ContainerInterface $container, array $configuratio
   +    if ($exception && in_array($exception->getStatusCode(), array(403, 404))) {
   +      return FALSE;
   +    }
   

   Could it be that we want to show the help text just on 200? Note: \Symfony\Component\HttpFoundation\Response has some constants we could leverage

2. +++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
   @@ -259,4 +259,16 @@ function testInvalidTheme() {
   +  function testThemeSettingsNotFoundHelpText() {
   

   Let's make the test method public.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
   @@ -259,4 +259,16 @@ function testInvalidTheme() {
   +    // Ensure invalid theme settings form URLs return a proper 404.
   +    $this->drupalGet('admin/appearance/settings/' . $this->randomName());
   +    $this->assertResponse(404, 'The theme settings form URL for a non-existent theme could not be found.');
   ...
   +    $help_block = $this->xpath("//div[@id='block-help']");
   +    $this->assertFalse($help_block, 'The help block is not displayed.');
   ...
   +    // Ensure help block does not display.
   
   +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemHelpBlock.php
   --- a/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
   +++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
   
   index 554450a..8f3698e 100644
   --- a/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
   

   I think we should move this test to some help related test and maybe use a more generic path or even better a unit test.

What about something like this?

Added dawehner's #25 comment to #21 patch.

#19.3

... and maybe use a more generic path or even better a unit test.

I don't know how to make a unit test for this.

Back to needs review. Only the patch without the change failed.

[edit] typo

rerolling

rerolled patch to fix conflicts

error: patch failed: core/modules/system/lib/Drupal/system/Plugin/Block/SystemHelpBlock.php:90
error: core/modules/system/lib/Drupal/system/Plugin/Block/SystemHelpBlock.php: patch does not apply

Oh shoot. I'm really sorry but I already committed this in another issue. :( #2245783: Regression: Help blocks display on 403/404 page Somehow this issue got missed.