Custom Help Text

Hi,

Just a couple of notes:
- Use 2 spaces instead of 4 spaces per "tab" as per the drupal coding standards.
- Considering changing the tab name in the admin area to something short e.g. List. This also follows similar conventions that other drupal modules use. Same with add.
- Line 14 in hook_help.module looks unfinished
- Rather than defining your own submit button you can use "return system_settings_form($form);" instead.

Nice inclusion of a test file, my test setup is currently broken but will get it up and running soon. If you drop by #drupal-uk there are a few people who are willing to review as well.

Setting to needs work as per comments above. Please move back to needs review when you're ready for a reviewer to look at this.

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/hook_help.module:
   +9: [minor] Comment should be read "Implements hook_foo()."
   +30: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +76: [minor] Comment should be read "Implements hook_foo()."
   +86: [minor] Comment should be read "Implements hook_foo()."
   +90: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +91: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +98: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +99: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +106: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +113: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +114: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +121: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +122: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +133: [minor] Comment should be read "Implements hook_foo()."
   +267: [minor] Comment should be read "Implements hook_foo()."
   +292: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
   +358: [minor] Comment should be read "Implements hook_foo()."
   +382: [normal] use stdClass caseCapitalization, it's the one exception to the mixed case style standard
   +391: [normal] else statements should begin on a new line
   +454: [normal] use stdClass caseCapitalization, it's the one exception to the mixed case style standard
   +488: [normal] use stdClass caseCapitalization, it's the one exception to the mixed case style standard
   +494: [normal] else statements should begin on a new line
   +534: [minor] Comment should be read "Implements hook_foo()."
  
  sites/all/modules/pareview_temp/test_candidate/hook_help.install:
   +3: [minor] Commits to the Git repository do not require the CVS $Id$ keyword in each file.
   +11: [minor] Comment should be read "Implements hook_foo()."
   +18: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 3 files, 16 normal warnings, 10 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./help.html:                                              ASCII English text, with very long lines, with CRLF line terminators
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  hook_help.install:3:// $Id$
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

wrong branch name, 6.x-1.x-dev should be 6.x-1.x. And you should remove all contents from the master branch, step 5 in http://drupal.org/node/1127732

Review of the 6.x-1.x branch:

• ./hook_help.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

      // Function is called twice, return false here on second call to prevent multi printing.
  

• ./hook_help.test: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  24-   *  The array of options
  

• ./hook_help.install: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  25- */
  

• ./hook_help.module: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  477- *  The path data element.
  479- *  The contents of the help item.
  482- *  The new hid of the inserted record.
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• remove the 6.x-1.x-dev branch to avoid confusion (master cannot be removed)
• hook_help_schema(): hook doc block missing
• hook_help_schema(): do not run any DB descriptions through t(), this will only create overhead for translators
• "$path_results = db_query("SELECT hh.hid, hh.path FROM {hook_help} hh INNER JOIN {hook_help_roles} hhr ON hh.hid = hhr.hid WHERE hhr.rid IN (0, " . implode(', ', array_keys($user->roles)) . ")");": do not use the SQL IN statement like this, see http://drupal.org/writing-secure-code or db_placeholders()
• hook_help_admin_page(): you don't need that page callback if you just want to display a form. Use drupal_get_form() as callback in hook_menu() directly.
• "filter_xss(l(t('edit'), 'admin/build/hook_help/edit/' ....": why do you need filter_xss() here? There is no user data involved and the l() functions sanitizes with check_plain() already.
• "drupal_set_title(t('Edit help message for path "!path"', array('!path' => check_plain($hook_help['path']))));": you can use t() with the @ placeholder here, which will do a check_plain() automatically. See http://api.drupal.org/api/drupal/includes--common.inc/function/t/6

Regression since the above changes reveals new (or remaining) coding-style flaws:

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  
  hook_help.install
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Besides that, may I suggest a different name for the module?

Because hook_help() is the name of a core hook, I believe that it would be very easy for there to be confusion. That name also does not really communicate what the module is for. Something like (just an idea) "Custom Help Text" is descriptive.

No, thats not necessary. This sandbox's title can be changed, so can this issue's title. You woukd need to change the module's filenames and function names. If you're not familiar, the right way to rename files in git is to run "git mv oldfile newfile". If you just use mv, git is not aware and treats oldfile as deleted and newfile as without history.

@philipnorton42 has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

@philipnorton42 writes back that the application is not abandoned.

I have downloaded and installed the 6.x-1.x version on my localhost D6 site. The module works well and is very useful. I was able to easily add help messages to the top of various forms and nodes.

One strange thing that I noticed here for example (admin/build/custom_help_text/edit/2), I am unable to get the 'user role' checkboxes to stay checked. I tried turning on/off the 'view custom_help_text' perm to see if that would get the checkboxes to stick, but that didn't work either.

Basically, I was unable to get the 'user roles' checkboxes to do anything. This would be a nice feature, so you could limit the visibility of the help text to certain roles.

Also, the text 'Leave empty to add to all user roles ('view custom_help_text' permission will prevent certain users from seeing it).' near the checkboxes mentioned above is confusing to me. It seems vague/contradictory... this could use some clarification.

Other than that, nice job on this module. The only thing I would recommend is adding another field to allow the user to specify a drupal message box class (status, warning, error) along with each custom help text. That way when the custom help text is displayed it can 'stand out' a bit more, and user's won't have to write any css to spice up the custom help box. But perhaps this could be added later as a new feature once the project is up and running officially.

Hi Philip,

I gave your module another try with a fresh Drupal 6.24 install. Again I used it to add some custom help text messages to places like the contact page, the user login and registration pages, as well as the node add story page. The custom text appeared for all cases. I then tried flipping on/off the new user role mechanism you setup (much better by the way) and visiting each page as an anonymous/authenticated user to make sure it worked as expected, it did.

In my opinion this module is good to go, except there are some coding standard issues that should be cleaned up:

http://ventral.org/pareview/httpgitdrupalorgsandboxphilipnorton421193750git

Nicely done. RTBC for me. I'll use a module like this regularly - currently I just write a module on a per-project basis with explicit, specific custom helptext messages in it as aids for my clients and their content-authors and site-admins. A re-useable generic solution is brilliant.

This is great in its current state. I will share one detail which has recently been pointed out to me. That's that any strings in the module which go through t() would be best wrapped in double quotes rather than in single quotes with escape-slashes behind any single quotes in the string. That's because the code and string are easier to read but more importantly the presence of escaping-slashes can introduce reliability problems if those strings ever are translated.

Suggestions for improving this module:
Obviously, porting to D7.
Making help-texts and mapped paths exportable.
Providing token support in the messages or the paths.
Providing for drupal_set_messages as well as help text (you can do this with things like Rules and Context, but, what overkill just to set status message and mode).
Providing for contextual links on the messages, for admin tasks.

Just suggestions! It just goes to show that your module has lots of potential because it's such a good idea.

Would you like to take part in the review bonus program? I personally only review/approve applications with a review bonus, but of course you can also wait for other git administrators to take a look at your code.

Congratulations philipnorton42 on a long review process and learning a lot about Git and d.o idiosyncracies too! You are now a vetted Git user and can promote this and other projects you create to full project.