Clean up hook_help() by providing real Drupal and router paths

dimitrig01: maybe *you* did not want to add help text to those places, but maybe there are core patches (I know at least of one) blocked because it is not possible to add help text granularly. How many times should I describe that if we have the $section parameter, it implies that the help hook is used for more then just asking for the page level help one time on a page view? And it is not merely implied, it really is used at least two other places additionaly to asking for page level help texts (building the help menu items and printing "more help..." links when required). You advocate hacking with $_GET['q'] in core modules in the help hook? Is this the ultimate solution?

pwolanin: let's see that patch. If it gets too big or unrelated, then it needs to be broken up. If the issues are really closely related, then they will be broken up anyway. (I don't see what you are up to yet with local tasks, the block configure pages are not local tasks for example as far as I know).

Well, this looks like a *very good* development! Looks very good.

A few remarks:

1. You did include hook_help($path, $router_path = NULL) in most modifications, but the help, php, ping, poll, search, syslog, throttle, tracker and upload modules does not include the $router_path argument at all. Other help hook implementations include the $router_path even if they don't need it. I think it should be there on all functions for consistency.
2. This looks like this patch makes help hook writes lives an order of magnitude easier :)

-  if (arg(0) == 'node' && is_numeric(arg(1)) && arg(2) == 'outline') {
+    case 'node/%/outline':

But! Chx noted that we should really see the menu key here, ie. "node/%node/outline", so we can use the same keys in the help hook and in the menu hook. I understand it will not be a router path then, but a menu key. That would be the extra mile to go here IMHO to make this even more sexy for developers.

3. Staying at the book module example (but this happens with others too), it looks like you divided the help hooks into a part dealing with $path and one with $router_path... The second part deals with paths with variable parts, but it also deals with constant parts. In cases like 'admin/content/book' and 'admin/content/book/orphan', does it make any difference to switch() against the $router_path and not the $path? As far as I understand there is no difference. What about separating by "has dynamic parts" == switch against $router_path and "static path" == switch against $path? Does that make sense, or there is some deeper reason to separate as the current version in the patch?

4. Slightly related. Although you killed of most (nearly all?) arg() calls in help hooks, which is a good cleanup, you did move the "admin/build/themes/settings" to the $path switch. This might be because the menu system has no router definition for this with the ending path part. Would it make sense instead to add this key to the menu with an ending % part, so we can simplify the handling of it? Just as it is with "admin/settings/filters/%"?!

All-in-all I think this is a good direction, simplifies help hooks a lot and even makes some new things possible, so thanks!

Well, well... I don't think it is easy to distinguish between what should I put into switch($path) and switch($router_path)... Seems like most of the modules would have both switches, and the first switch mostly contains one item, the one for the module level help page. So this seems like a pattern which we enforce on people, but which could be simplified.

There are really three types of paths we display help for:

- the admin/help#$modulename pseudo-path
- router path based shortcuts, like common help for default local tasks and parent tabs, or node/%/edit page help, etc...
- help based on full path, like node/1/edit help set by some contrib module, or admin/build/block/configure/user/0 help, as it is not available as a menu key

I looked into how Drupal 5 handles the help hook exactly, and it simply passes on $_GET['q'] as the help $section parameter. So what is new in Drupal 6 is that we can display help for the router path, not only the pseudo-path or $_GET['q']. But this also makes it more complex, so we *need to choose* between what we need to return help for.

Seems like we are pushing to use the pseudo-path and the router path if possible, and only resort to full path based trickery if the two simpler cases are not adequate. So why not make *that* way of thinking simple?

I'd say the first parameter of hook_help() should be the pseudo-path and the router path folded into one value (these complement each other, there is no overlap). How to name this is a good question. $section was very odd before, $path might be usable here, and it is already in the patch :) Then the second (optional) argument could be the $_GET['q'] value for the request, if given (that is, if the first argument was a router path). This second argument could be named $full_path or something similar, I would even say $q, but I bet Dries would not like that one :)

So most help hooks would be a switch on the first parameter only, and I would only need to think about the special cases, if $full_path was specified. And then never use arg(), but use $full_path instead in a preg, as that is what specifies the path to look up help for. I think that would simplify the help hook to a Drupal 5 level of understandability, while gaining the good of building on router paths. (I would default $full_path to an empty string to easily use it in preg_match(), without needing to check whether it exists or not).

BTW I discussed with chx that we cannot do better here then using % for all placeholders, without object names.

PS. forgive me that I started off this into a complicated direction, and just trying to help you move the patch to a simpler solution.

PS. search module also creatively makes up a pseudo-path, using the help hook as a callable message store (another sign that it does not return help text only for the current page)... this kind of creative thinking might be present in contrib modules... I am not entirely sure, whether I like this approach or not, a custom callback function to retrieve the reusable help text might be overkill for this task, so reusing the help hook was probably easiest for whoever added that code.

Well, previously (D5) the full path and the pseudo path was in the same variable. Now we have the router path to simplify our lifes a bit, but we still need the fill path for more sophisticated help. So it seems to be logical to merge the new primary values.

Yes, I advocate that. The core use cases might better do with the array structure as with arg(), but they might be rewritten to use preg() which would use a regex instead of multiple array element tests. This is probably a matter of taste. I am not sure what other core maintainers would say, so it might be a safe bet to keep the arg() like format and pass on an array, so the philosophy behind existing code is not shifted.

This patch is looking really good, but:

1. The filling of $empty is repeated too many places. It is also a computable value, so why not have a helper function which automates it, and use it when needed?

/**
 * Generates empty elements for the $arg array in the help hook,
 * so we can use them without checking the existence of values.
 */
function drupal_help_args() {
  return array_fill(0, MENU_MAX_PARTS + 1, '');
}

2. Some modules are missing from the patch, like openid.module and translation.module.

The patch seems to be RTBC if these two issues are fixed.

Well, I lamented, lamented and lamented whether $args would be better then $arg... But at the end, I turned to the "familiarity with arg()" argument, so renamed drupal_help_args() to drupal_help_arg() to be consequent. Also fixed a remaining $router_path hook help param in the patch, renamed $empty to $empty_arg at all places, $empty looked very odd. Also removed two now unneeded line from locale_help() and removed an extra line from comment_help().

All-in all, I committed the patch attached. Thanks for your work!

This change should be documented in the module update guide!

Also fixed a capitalization issue in the beginning of the $path docs. IMHO feel free to commit. Thanks.