help from hook_help displayed on 403 pages

After exploring a fix in drupal_access_denied:

@@ -378,7 +378,7 @@ function drupal_access_denied() {
   }
 
   $path = drupal_get_normal_path(variable_get('site_403', ''));
-  if ($path && $path != $_GET['q']) {
+  if ($path != $_GET['q']) {
     // Set the active item in case there are tabs to display or other
     // dependencies on the path.
     menu_set_active_item($path);

(which chx was uncomfortable with), I found the solution in the attached patch. This should save us some cycles, so it doubles as a performance patch.

Patch is against D6, but should be identical for D7.

Ok, let me try to explain again.

Currently hook_help() is invoked on every page load. This includes 403 and 404 pages, where clearly there should not be any help to display. In some cases currently, there is even help displayed in this case, since the help text is returned based on the args, rather than the path.

The latter problem could be worked around, but it would be better to never waste the cycles invoking this hook if we are on 403 or 404.

In the 403/404 we have this sort of code currently:

  $path = drupal_get_normal_path(variable_get('site_404', ''));
  if ($path && $path != $_GET['q']) {
    // Set the active item in case there are tabs to display, or other
    // dependencies on the path.
    menu_set_active_item($path);
    $return = menu_execute_active_handler($path);
  }

So, $_GET['q'] is not changed unless the 'site_404' variable is defined. Even in this case, we will have an unneeded hook invocation.

The proposed solution in the patch relies on the fact that the router_path used to invoke hook_help comes from menu_tab_root_path(). The reason we use that function is that it provides "the router path, or the path of the parent tab of a default local task." In that way we get the same help if you are on the default local task's path instead of the parent.

The router path is actually generated by menu_local_tasks() using the $return_root flag. The relevant code there is:

    $router_item = menu_get_item();
    if (!$router_item || !$router_item['access']) {
      return '';
    }

so on either 403 or 404 we get back an empty string instead of a router path. So, the proposed patch just checks for this condition - an empty router path - before invoking hook_help.

To re-iterate: hook_help is getting fooled because $_GET['q'] doesn't change unless we have defined a custom 403 or 404 page. In addition, even if $_GET['q'] is changed for a custom 403/404 we are invoking hook_help when we don't need to.

We should consider also applying the change shown in #1 (or something similar), but that's really a separate issue and not needed for this bug fix.

patch still applies (with offset) to both 6.x and 7.x

With this patch applied to 7.x all test pass - except known failures:

Cache expire test: 21 passes, 1 fail, 0 exceptions
Site-wide contact form: 120 passes, 4 fails, 0 exceptions
Core filters: 48 passes, 12 fails, 0 exceptions
Poll create: 27 passes, 7 fails, 0 exceptions
XML-RPC validator 0 passes, 0 fails, 3 exceptions

patch still applies cleanly to 7.x (with offset) rechecked again and it does fix the bug.

patch still applies