help_menu() creates unnecessary router items

How's this?

Reroll.

This needs a reroll first.

Rerolled. I removed the unnecessary title argument, since we are not using a title callback at all. The page title is set via drupal_set_title() in help_page().

Some notes from my review:

• The NotFoundHttpException() change is within scope here, otherwise a blank page is shown for /admin/help/foobar, which is now a valid router item but the module just doesn't exist.
• I don't think this needs any additional tests, the existing tests for help.module validate this change. I was thinking we might need a test to ensure the correct title gets set, but there's already test coverage for that. There are no user-facing or functional changes here, this change only cleans up menu_router items.

Could use a review from one more set of eyes.

Here's an interdiff.

#6: 1618458-6.patch queued for re-testing.

This definitely needs a reroll, will get to it over the next few days.

This should do it.

#11: 1618458-11.patch queued for re-testing.

well, this doesn't cause any changes to functionality, but changes the code to remove unnecessary router items. I don't often use help so I can't comment too much on how it is different but good work Cottser.

My first attempt at the D7 backport. The D8 patch doesn't contain page arguments but I wasn't sure if the same would be the case for D7 so I left it in. I did remove the title like in the D8 patch.

The title is different as it is not longer handled in hook_menu(). In order to support the title properly you probably need a title callback.

Thanks for the feedback dawehner - I had a look at the page callback help_page($name) in help.admin.inc and it does set the title there using the module name so I thought that was enough:

drupal_set_title($info[$name]['name']);

But there could well be another use of the title callback that I'm missing, maybe to do with i18 or breadcrumbs?

Well, you might need this for menu links.

Thanks for your help dawehner. I've added a title callback. It does not translate using t() because this wildcard should be the machine name of a module. I also added some text to help.admin.inc which is displayed if the wildcard from the URL does not correspond to any module name or no hook_help() is found for that module name. This may not be the most appropriate message, or even best way to go - perhaps I should use drupal_not_found()? In the D8 patch a NotFoundHttpException() is thrown and I'm not sure what the equivalent would be for D7.

This patch uses drupal_not_found() instead of the custom message when the help page doesn't exist. Also removed unnecessary whitespace. It's my first interdiff so I may not have done it right but we'll see. Was looking to replace the custom title callback with another that already existed but couldn't find anything appropriate, so left the custom one.

ok, something weird going on there with some code from field.api. Will figure it out and fix.

I had reverted a commit I think. Trying again.

Updating issue status...

Tested and realised that using drupal_not_found() caused a double page not found message as well as a 'headers already sent' error. Changed to return MENU_NOT_FOUND instead (as suggested at https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...) and this seems to work. New patch attached.

Minor but the @param and @return on the helper function could indicate the data type as on https://drupal.org/node/1354#types.

Good work on this @foxtrotcharlie :)

Thanks @Cottser. Here's an updated patch that includes the data types for the @param and @return values.

Shouldn't we sanitize the value? I am just curious.

Thanks for checking @dawehner. I think I read another issue where it was escaped twice so was trying to avoid that (see https://drupal.org/node/1137744). And maybe in this issue too: https://drupal.org/node/1555294 ?

But, now that you mention I'm not sure if that applies in this case. @Cottser was suggesting that it would be ideal if we can get rid of the title callback completely, but we weren't sure if that was possible. I will try dig into the menu system to see if I can understand better how the title callback will be used in this case - if it's passed to l() then we would want an unsanitized value, no? I am also actually not sure what MENU_VISIBLE_IN_BREADCRUMB means in practice, but I will look into it.

At least it seems clear that the title callback is not being used for displaying the page title as that's handled by drupal_set_title() in the help_page() callback in help.admin.inc.

router time = router item

I think we don't need to sanitize the value because when drupal calls for the title in drupal_get_title(), if the title hasn't yet been set using drupal_set_title (which itself runs it through check_plain()), drupal_get_title() gets the title using menu_get_active_title() which it then runs through check_plain(). Perhaps someone who knows the menu system well could confirm this.

Although the menu title (that we set using the new title callback) is not used as the page title because it's set by the page callback, and it is not used in the breadcrumb (because the current page is never displayed in the breadcrumb), the menu title is used by the shortcut module when you add a shortcut to the toolbar, and in our case it will just display the machine name of the module, not the actual name. I don't know if this is a shortcoming of the shortcut module ;-) but I haven't had time to investigate it.

A way to fix this in the help module would be to move the logic which sets the page title in help.admin.inc:

  if (module_hook($name, 'help')) {
    $info = system_get_info('module');
    drupal_set_title($info[$name]['name']);
  }

into our custom title callback so that it looks something like:

function help_page_title($name) {
  if (module_hook($name, 'help')) {
    $info = system_get_info('module');
    $name = $info[$name]['name'];
  }  
  return $name;
}

In this way, if the module exists, we set the title so that it is not just the machine name of the module, but the actual display name.

The updated help_page callback would then look like this:

function help_page($name) {
  $output = '';
  if (module_hook($name, 'help')) {

    $temp = module_invoke($name, 'help', "admin/help#$name", drupal_help_arg());
    if (empty($temp)) {
      $output .= t("No help is available for module %module.", array('%module' => $info[$name]['name']));
    }
    else {
      $output .= $temp;
    }

    // Only print list of administration pages if the module in question has
    // any such pages associated to it.
    $admin_tasks = system_get_module_admin_tasks($name, $info[$name]);
    if (!empty($admin_tasks)) {
      $links = array();
      foreach ($admin_tasks as $task) {
        $links[] = l($task['title'], $task['link_path'], $task['localized_options']);
      }
      $output .= theme('item_list', array('items' => $links, 'title' => t('@module administration pages', array('@module' => $info[$name]['name']))));
    }
  }
  else {
    return MENU_NOT_FOUND;
  }
  return $output;
}

Then I am not sure why we are using type "MENU_VISIBLE_IN_BREADCRUMB" which is a flag and not a menu type (types are listed https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...). Should we not be using "MENU_CALLBACK" as the type? There was an issue early on in D7 where MENU_CALLBACK was buggy and did not correctly display the page title (https://drupal.org/node/965272) but this was fixed - perhaps this is why the flag MENU_VISIBLE_IN_BREADCRUMB was initially used? Can I use MENU_CALLBACK instead?

Discussed this in IRC with @Cottser today and he said that it was probably beyond the scope of this issue to change the menu title to be the human readable module name. So we'll keep the patch at #29, and now just needs review - would be great if someone can confirm that we don't need to sanitise the title callback value :-)