Subheading Links

First of all, good on you for making the output of your module themeable.

I’ve done a manual read-through of the code and it looks pretty good. Just a few (smallish) issues:

• You will need to remove the version information from the .info file.

• In general, the preferred Drupal style is to use double-slashes (//) for in-code comments rather than block style (/* */). Line 35 of subheading_links.module (for example) would usually be written // load rather than /* load */ (this is a minor point though).

• On line 37 of subheading_links.module, you should be able to just return the value from drupal_get_form(), without needing to explicitly call drupal_render() (Drupal will do that for you).

• On line 55 of subheading_links.module, $headings might be better off defined as a constant at the top of the file.

• On line 111 of subheading_links.module, would not DRUPAL_CACHE_PER_PAGE be a more appropriate caching setting, or does that cause issues when users update pages?

• On line 152 of subheading_links.module, should the comment be changed to “search for subheadings”?

• On line 192 of subheading_links.module, the way you’re appending arrays to each other is a little bit confusing. Should it not be one of the following?

  $poistion_of_tags = array_merge($position_of_tags, _subheading_links_find_tags($updatebody, $tag));
  

  or

  $position_of_tags = _subheading_links_find_tags($updatebody, $tag, $position_of_tags);
  

  (To understand why I’m recommending you use array_merge() instead of the ‘+’ operator, see http://stackoverflow.com/questions/2140090/operator-for-array-in-php#214..., particularly the first comment)

Marking as 'needs work' because the version information does need to be removed.

Hi ruz,

what exactly is the difference in usage or added value to the Table of Contents module?

http://drupal.org/project/tableofcontents

Joost