Hi, I'm confused about how the hook_help() is structured in core modules. It's currently done like so:

function hook_help($section) {
  switch ($section) {
    case 'admin/help':
      $output = '<p>'. t('string') .'</p>';
      $output .= '<p>'. t('string') .'</p>';
      return $output;
    case 'admin/...':
      return '<p>'. t('string') .'</p>';
    case 'admin/.../...':
      return t('<p>long string</p><p>another long string</p>');
  }
}

Why the $output variable for the 'admin/help' pages while all others return it directly? When there are multiple paragraphs outside 'admin/help' they are embedded inside t().

Wouldn't it make more sense to just assign all cases to $output the return it outside of the switch to make it more consistent?

function hook_help($section) {
  $output = '';
  switch ($section) {
    case 'admin/help':
      $output .= '<p>'. t('string') .'</p>';
      $output .= '<p>'. t('string') .'</p>';
    case 'admin/...':
      $output .= '<p>'. t('string') .'</p>';
    case 'admin/.../...':
      $output .= '<p>'. t('long string') .'</p>';
      $output .= '<p>'. t('another long string') .'</p>';
  }
  return $output;
}

Or maybe I'm making the wrong connection here. Any thoughts?

CommentFileSizeAuthor
#8 help_flow2.patch110.63 KBdvessel
#7 help_flow1.patch110.89 KBdvessel
#5 help_flow.patch110.81 KBdvessel
#2 help_cleanup.patch122.32 KBdvessel

Comments

dvessel’s picture

dvessel commented

Note, I'm willing to clean it up but need some guidance. Which html tags are okay inside t()? Noticed a few UL tags in there too. Is this to make it easier for translators by grouping chunks of text?

dvessel’s picture

dvessel commented
Title: hook_help coding convention? » Clean up hook_help for core modules.
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new help_cleanup.patch122.32 KB

Okay, got some feedback from webchick. HTML inside t() is okay when elements are grouped together to give context. I cleaned it up a bit based on the information from http://api.drupal.org/api/HEAD/function/t and from the predominant coding pattern of what was already there. Namely, the spacing consistency.

General issues:

  1. Everything is collected into $output and is returned from a single point for all sections. Adds consistency.
  2. Breaks added as none where there beforehand.
  3. Changed single quotes to double & vice versa to minimize escaping of strings.
  4. Various spacing inconsistencies cleaned up.

Module specific issues:

book.module
-Changed if conditions to case.

drupal.module
-Moved <p> *inside* t() above <ul> for consistency.

filter.module
-Moved <p> tag out of t().
--originally on line 42

node.module
-Missing closing </p> tag.
--originally on line 35
-Added <p> tag.
--originally on line 39
-Added <p> tag.
--line containing "filter_xss_admin($type->help)"
--This is output from the user changeable submission guidelines. Not likely to have <p> on its' own.
-Changed if conditions to case.

search.module
-Moved <p> tag out of t().
--originally on line 104

Extra notes:

drupal.module's help leads seems to lead to nowhere. Might be a bug.

I've raised this to critical since there are string changes from moving a few tags. It's been double checked and checked against latest head but since there are a lot of changes, I guess it better be checked by someone else.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Priority: Critical » Normal
Status: Needs review » Needs work

I like this patch, but it's not critical. A few things:

  1. blogapi has too much indentation on + return $output;
  2. These:
    -  if (arg(0) == 'node' && is_numeric(arg(1)) && arg(2) == 'outline') {
-    return '<p>'. t('The outline feature allows you to include posts in the <a href="@book">book hierarchy</a>.', array('@book' => url('book'))) .'</p>';
+      $output .= '<p>'. t('Pages in a book are like a tree. As pages are edited, reorganized and removed, child pages might be left with no link to the rest of the book. Such pages are referred to as "orphan pages". On this page, administrators can review their books for orphans and reattach those pages as desired.'). '</p>';
+      break;
+    case 'node/'. is_numeric(arg(1)) .'/outline':
+      $output .= '<p>'. t('The outline feature allows you to include posts in the <a href="@book">book hierarchy</a>.', array('@book' => url('book'))) .'</p>';
+      break;

    are not equivalent. is_numeric(arg(1)) will return FALSE, and the URL will never be node/FALSE/outline. You probably want case 'node/'. arg(1) .'/outline'

  3. Why remove indentation on the <li>s? Like in drupal_help? LIs are nested inside ULs, so it makes sense to indent them, imo.
  4. Changes like:

    -Moved

    -Moved

    *inside* t() above

      for consistency.
      -etc.

      Should be broken out into separate patches. I know it's a pain to roll and maintain 5+ different patches and it's very easy to instead just catch these things all at once as you're going through; however:

      • it makes the patch harder to review because it's so large
      • There are many different types of changes, each of which needs to be weighed for pros/cons
      • It creates an "all or nothing" scenario; either all of these bugs will get fixed or none of them will

      Try and keep the guideline of one type of change per patch. -- better to split out the "no-brainer" fixes into separate patches so they can get committed quickly and aren't held back by people discussing whether a +XX character bloat to the source code is worth the trade-off in consistency (which I would argue it is, but others might have differing opinions). So I'd re-roll this patch so it's only the $output changes (and anything related to making the cases consistent), and make separate patch/patches for the other minor fixes.

dvessel’s picture

dvessel commented

Duly noted. Will re-roll into smaller patches.

The removal of ul indentation was because the majority of them didn't indent them either. :) I do agree with you though.

dvessel’s picture

dvessel commented
Title: Clean up hook_help for core modules. » rework flow for hook_help (core modules).
Status: Needs work » Needs review
StatusFileSize
new help_flow.patch110.81 KB

Updated patch.

1. Everything is collected into $output and is returned from a single point for all sections.
2. Case breaks added as none where there beforehand.
3. Changed if conditions to switch > case. Look at the previous comment to see which modules were affected.

It's still pretty large but that's all that was changed.

dvessel’s picture

dvessel commented
Status: Needs review » Postponed

The changes are too close together for the separate patches to work. I'll upload another after this one gets committed.
http://drupal.org/node/101562

dvessel’s picture

dvessel commented
Status: Postponed » Needs review
StatusFileSize
new help_flow1.patch110.89 KB

Updated for head. This patch does *not* affect the string freeze. I did add some code for the 'filter_xss_admin($type->help)' in node.module since the patch from http://drupal.org/node/101562 unconditionally added the paragraph tag. Now it will only output the html when there's content to go with it.

-  if (arg(0) == 'node' && arg(1) == 'add' && $type = arg(2)) {
-    $type = node_get_types('type', arg(2));
-    return '<p>'. filter_xss_admin($type->help) .'</p>';

+    case 'node/add/'. $type = arg(2):
+      $type = node_get_types('type', $type);
+      if (!empty($type->help)) {
+        $output .= '<p>'. filter_xss_admin($type->help) .'</p>';
+      }
+      break;

Besides that, it's the same as post #5 updated for latest head.

dvessel’s picture

dvessel commented
StatusFileSize
new help_flow2.patch110.63 KB

hook_help for taxonomy.module was updated since last patch.

Patch updated for that. Anyone care to set it RTBC?

robertdouglass’s picture

robertdouglass commented

Well, it makes the code look cleaner, I'll agree. I've not looked at every single help page that this affects, so I'm going to refrain from setting to RTBC. If I see the patch languishing too long I'll come back after Christmas and take a look =)

dries’s picture

dries commented
Version: 5.x-dev » 6.x-dev

Looks OK to me, but let's postpone this to D6.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: rework flow for hook_help (core modules). » Rework flow for hook_help (core modules).
Status: Needs review » Needs work

Thanks to this patch, hook_help() has changed. So this patch will need a reroll. Conversion docs are available: http://drupal.org/node/114774#hook-help.

dvessel’s picture

dvessel commented

Whoops, this fell under the radar for me. I'll get this in before the string freeze -if one exists.

Thanks for the links and reminder Wim Leers.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: 6.x-dev » 7.x-dev

Moving to D7.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Closed (fixed)

Cleaning up the issue queue. And I think the help system has already been revamped by now :)