Hook implementation headers out of compliance with standards

I think that for seasoned Drupal programmers, it is important to have hook_foo() (which will turn into a link) somewhere in the first few words of the doc for a hook implementation function.

I also think that all doxygen function headers' one-line summaries should describe what the *particular* function does. I don't think repeating the generic description of the hook's purpose satisfies that; and given that hook_foo() will link to the generic hook's doc page, I don't see a lot of use in making that the standard. The hook implementations and hook doc will also tend to get out of sync, so I think this would become a maintenance issue.

So let's take a look at http://api.drupal.org/api/function/poll_update/7 as an example. Right now, the one-liner is

Implement hook_update().

As If would propose changing this to:

Responds to node updates.

But how about this instead:

Implements hook_update() - responds to node updates by updating the poll settings and votes appropriately.

This:
a) Includes a link to the generic hook doc
b) Tells us what the function is actually doing to implement the hook

(Note: This is the opinion of webchick the Drupal hacker and not webchick the core maintainer. Feel free to disagree.)

The initial line of PHPDoc must wrap at 80 characters, so option #3 is out.

Additionally, I feel that "Responds to node updates." is:

a) Non-descriptive. Responds to whose node updates? Does what with the response?
b) Incorrect. This is an implementation of hook_update() which is what's actually called when the node is saved. hook_nodeapi_update() would be the one that responds to node updates. That's only in modules that don't define their own node type.

I personally would like to see this structured in such a way that API module can easily provide "backreferences" to hook implementations. So @see seems like a really nice way to handle this.

So maybe:

/**
 * Handles the update and removal of poll settings and choices.
 *
 * @see hook_update()
 */
function poll_update($node) {
  ...
}

I would also actually recommend not postponing the "Make everything 3rd person" on this patch since if we do this right (or my definition of right ;)), all of these "Implementation of" summaries will be different, since they'll be descriptive of what the function is actually doing (like any proper PHPDoc function summary). It's going to take awhile to figure out what these all should be, if we go this route. And in the meantime, adding an "s" or an "es" is a very easy change that can go in quickly.

Oh. One additional reason to nix the suggestion of:

"So the description for hook_update should say: Responds to node updates.

The description for hook_menu should say: Responds to URL requests."

I think this would create a DX nightmare to have to stop every 4 seconds as you're writing your module to look up the proper API documentation string for whichever of the hundreds of hooks you're implementing. I'm a coding standards zealot, and even I can't picture myself doing that. :) Life is too short.

On the other hand, people who document their functions know how to document their functions. The only rules they need to know, which they can apply consistently, is:

1. Must be a 3rd-person description (this applies to all PHPDoc blocks, so it's an easy rule to remember)
2. Must include @see hook_XXX(), for hook implementations. We do a similar thing with @ingroup themeable for theme functions already.

Those are two small rules to learn as opposed to hundreds.

Oh no! :) Let's file a follow-up patch for "Make the docs for hook_update() not suck!" ;)

Are you sure about the 80-character thing? In JavaDoc and other autodoc systems I have used in the past, the "first line" extracted for the function summary went to the first period ("." or "full stop" for non-US English speakers), rather than being cut off by a carriage return/line break. But I am not sure about doxygen... Anyway, whether it is exactly 80 characters or not, the point remains that it needs to be short, so that it will display successfully on pages like
http://api.drupal.org/api/file/modules/search/search.module/6

Actually, looking at that page, some of the descriptions are almost certainly more than 80 characters, and they don't seem to stop on "." either. So my guess is that Doxygen is stopping at the first blank line in extracting the "short and snappy" description.

Anyway, I am fine with the hook_foo() link not being at the top of the doc, as long as it is somewhere, and I think having descriptions of what functions do would be infinitely better than "implements hook_xyz()" or any of its grammatical permutations. :) I vote yes for the example of #3 and the rules of #4.

The 80 character thing comes from the bot in #drupal. I'll ask Morbus to chime in here for more info on that.

The bot follows our own Doxygen formatting guidelines, which state (in "Documenting functions"):

The first line of the block should contain a brief description of what the function does, beginning with a verb in the form "Does such and such" (third person, as in "This function does such and such", rather than second person imperative "Do such and such"). A longer description with usage notes should follow after a blank line.

There's no mention of line length there. I interpret the above as this being correct:

/**
 * Provides an example Doxygen formatting block.
 *
 * This is an anonymous function that serves only to ... [more lines]
 ...
 */
function ...

and this is OK too:

/**
 * Provides an example Doxygen formatting block.
 */
function ...

However, this is not:

/**
 * Provides an example Doxygen formatting block. It can go on and
 * on and eventually wrap to the next line. Morbus doesn't think so.
 */
function ...

Note, also, that the Doxygen standard itself asserts the same mentality (emphasis mine)

As the name suggest, a brief description is a short one-liner, whereas the detailed description provides longer, more detailed documentation. An "in body" description can also act as a detailed description or can describe a collection of implementation details. For the HTML output brief descriptions are also use to provide tooltips at places where an item is referenced.

My Coder Tough Love project has a rule for this, called "Function summaries should be one line only." And, yes, Druplicon has code that checks for this as well, based on api.module's handling of summary in the database:

if (preg_match("/\n/", $function_result->summary) && $function_result->branch_name != 'php') {   
  $function_result->summary = '[Documented summary is not one line. This is a bug. File a patch!]';
}

Well, it doesn't seem to be a *technical* limitation that the brief description be limited to one 80-character line. But I agree that it is a good idea. I've added this to the regular function doc section in the Doxygen standards page.

So, the idea would be to edit the "Documenting hook implementations" section to say something like this... let's see:

Documenting hook implementations

When documenting hook implementations, follow the same guidelines as for all other functions: The first line (limited to 80 characters) should contain a brief description of what this particular hook implementation does, beginning with a verb in the form "Does such and such" (third person). A longer description with usage notes may follow after a blank line, if more explanation is needed.

Generally, you should omit the @param and @return sections documenting the parameters and return value of your hook implementation, since this would just repeat information available in the hook documentation. Instead, include a line like @see hook_foo() at the bottom of your documentation header, which will generate a link to the hook documentation in the API module (and on api.drupal.org). Example:

/**
* Defines and generates blocks for the Forum module.
*
* Defines two blocks: Active forum topics (forum topics with the most recent comments), and New 
* forum topics (most recently created topic nodes). Each block can be configured to choose the 
* maximum number of forums to display.
*
* @see hook_block()
*/
function forum_block($op = 'list', $delta = 0, $edit = array()) {
  ...
}

subscribing. i'd rather that we define the summary as the first sentence rather than the first line.

On one hand, it is very good to force the summary to be on one line, because people do not make the mistake to write explanations then. (Explanations should follow after the summary.)

On the other hand, there are edge cases, in which it is impossible to squeeze a summary that makes sense into 80 chars. This happens quite rarely, but when it happens, a line exceeding the 80 char limit looks and feels strange.

On yet another hand, the current standard unveils many wrong PHPDoc summaries, where developers put entire novels into the summary.

OK, how about this for the doc standard, to be placed on the Doxygen page:

- Documenting functions section... was (in part):

The first line of the block should contain a brief description of what the function does, limited to 80 characters, and beginning with a verb in the form "Does such and such" (third person, as in "This function does such and such", rather than second person imperative "Do such and such"). A longer description with usage notes should follow after a blank line, if more explanation is needed.

(then it goes on to talk about params and returns).

How about this as a new version of the above:

The first section of the block should contain a brief description of what the function does, and beginning with a verb in the form "Does such and such" (third person, as in "This function does such and such", rather than second person imperative "Do such and such"). If possible, this description should be limited to one 80-character line, and in any case, it should end with a period (".") and be followed by a blank line. A longer description with usage notes should follow after the blank line, if more explanation is needed.

- Documenting hook implementations section (which is immediately following the Functions section) -- proposed new text:

When documenting hook implementations, follow the same guidelines as for all other functions. Start with a brief description of what this particular hook implementation does, beginning with a verb in the form "Does such and such" (third person), if possible limited to a single 80-character line, and ending with a period (".") and a blank line. A longer description with usage notes may follow after a blank line, if more explanation is needed.

Generally, you should omit the @param and @return sections documenting the parameters and return value of your hook implementation, since this would just repeat information available in the hook documentation. Instead, include a line like @see hook_foo() at the bottom of your documentation header, which will generate a link to the hook documentation in the API module (and on api.drupal.org). Example:

/**
* Defines and generates blocks for the Forum module.
*
* Defines two blocks: Active forum topics (forum topics with the most recent comments), and New 
* forum topics (most recently created topic nodes). Each block can be configured to choose the 
* maximum number of forums to display.
*
* @see hook_block()
*/
function forum_block($op = 'list', $delta = 0, $edit = array()) {
  ...
}

-1 from me, personally. As tha_sun states, too-long summaries are edge case rarities. The revised summary approach would make us incompatible with the Doxygen guidelines, UNLESS the particular configuration value JAVADOC_AUTOBRIEF is set. I would prefer that our internal documentation stay as close to "default" Doxygen as possible.

Wasn't aware of that. So we should probably remove the "if possible" and "in any case" from the latest proposal.

People will always violate the guidelines anyway. So maybe even if we think it might, in extreme circumstances only, be OK to violate the 80-character rule, we should have it in there as the guideline?

I'm +1 for no ambiguity in the guidelines. Just remove the "If.." altogether. "This description should be..."

Other than that, are we agreed on the standard above? If so, I will edit the doxygen page and we can close this issue finally, and then file separate issues and patches for all of the many function docs that don't follow the standard.

jhodgdon, as well as issues against coder and api.modules so that they're all enforcing the new, unambiguous standard.

#14 sounds great. I really didn't like "Implementation of" & either "Implement" or "Implements" was too terse.

@drewish#20 and @stella: my Coder Tough Love already has rules for one-liner summaries and longer-than-80-character documentation lines, so we can just steal those from there. API.module would still need to be fixed.

yeah, we can add it to the coder module. Morbus has already done most of the work :)

I've updated the instructions for documenting hooks on http://drupal.org/node/1354, since it seems we are all in agreement. I took out the "if possible", and made no changes to the standards for non-hook functions. If someone's unhappy, feel free to edit or suggest changes. Until then, this Documentation issue is hereby declared "fixed". :)

Rather than changing this to a Coder issue, I filed a separate issue: #521452: Update to check for new function header doc standards

I'm not sure what changes, if any, would be needed for the API module, as drewish suggested in #20. It seems like the API module would parse the doc correctly with the new standards? Or maybe #514404: One-line function descriptions adding additional text inappropriately takes care of this somehow? Not sure... Someone who knows what changes would be needed for API could please file an issue on that. Thanks!

Re-opening and tagging.

http://drupal.org/node/1354 still contains

/**
 * Implement hook_help().
 */
function blog_help($section) {
  // ...
}

which is entirely inconsistent now, because we use the third-person form everywhere else. And since users start to complain about PHPDoc summaries not following exactly what's noted there, while what is noted there does not make sense, I'm marking this critical.

I guess the fix is simple (just make "Implement[s]" plural), but won't alter that page without confirmation.

This issue was about creating the standards for documenting hook implementations, which is done.

It is a separate issue that much of Drupal 7 is out of compliance with the current doc standards. Or many separate issues -- the procedure is that one issue => one patch, and there is no way we should/could file one big patch to fix the entire Drupal 7 code base to bring it all into compliance. I think it's going to take a very long time until it is all in compliance.

So I suggest submitting other issues like #525596: Standardize function doc for actions, bootstrap, and batch include files when you have the time/energy to fix a module or include file or two, or when you notice that a given module is out of compliance (most are). And we should make sure that any new doc that is committed complies with the current standards.

I must be speaking Spanish or Greek. I did not refer to any particular code in HEAD, but instead, I referred to the coding standards handbook page for documentation/PHPDoc. Specifically, I referred to that exact page, as known as http://drupal.org/node/1354. The code snippet was copied 1:1 from there, so I'm not sure how you come to your conclusion.

Anyway, good luck. The new standards make zero sense to me, so I won't adhere to those new changes anyway. Reverting to previous status, because it seems you wanted to revert the status.

I am reopening this for discussion, because Drewish apparently decided to revert the changes that had been made to the coding standards doc on July 27, with no discussion here, even though my previous edit referred to this issue node.

See http://drupal.org/node/1354/revisions

What do you want to do?

And sun: regarding your recent comments here -- I hadn't been aware of the changes to the standards doc, so I apologize for not understanding what you were bringing up.

Drewish noted in IRC that his revert was partially aghast at "a full paragraph about what every hook implementation does", and his revert matches that, primarily deleting a large portion of documentation regarding hook_block().

I think, though, @drewish, that it might have been too hasty. I will heartily agree that describing what "hook_block()" does is pointless (and any other common hook). But the documentation that was deleted didn't really describe what hook_block does(), but described what blocks were being created in this particular implementation of hook_block(). Instead of describing hook_block(), the implementation, it described what blocks, specific to this particular module, were being created. I agree with that (though, I probably won't ever follow it either cos, in hook_block, as in hook_menu or hook_theme, it tends to be self-documenting based on function names, etc.).

The other major portion of documentation removed, regarding absentee @params and @returns for known hooks, removed was simply added back, in a different form, so there's no real argument there.

As for the rationale that "core doesn't do this", it's bunk. The code and doc standards have always been far ahead of what core actually does, and core fails in Coder and Coder Tough Love in dozens of places. It's a "do what I say, not what I do" sorta thing. Yes, they should be fixed (and my Druplicon's been saying that for years in its function lookups ;)), but reverting based solely on "what core does" shouldn't be weighed.

The whole point of having documentation headers on functions is that someone should be able to figure out from the doc header what a function does, and not have to read the code. Right? Otherwise, let's just scrap doc headers completely and force everyone to read the code for all functions.

The point of the previous version of that standard, which was debated on this issue fairly extensively above, was that hook implementations should follow the same rules as other function headers. As we had already agreed, don't say what hook_block() is (we can read that on hook_block()'s documentation header), and don't tell us what the standard params are for hook_block() (ditto), say what this particular implementation does that's special. And if there isn't anything special, which may occasionally be the case, maybe we should add to the standard that was reverted that a short form is:
Implements hook_menu().
(note the S on implements, in accordance with current doc standards)

And just as a note, don't we have a policy about debating issues in the issue queue and not on IRC? It nearly made me blow my top yesterday when I realized that doc had been edited without opening this issue back up for discussion. The revision log pointed to this issue, and it should have been discussed here rather than silently edited.

jhodgdon, in hind sight i should have left a comment here but you changes seemed far beyond what had been discussed. as for IRC morbus had simply asked me about my revision.

So can we make the standard for hook implementations be:

/**
 * Implements hook_foo().
 */

If we can agree on that, I don't mind whipping up another patch to fix all of the existing comments in core.

Well, I think we can definitely agree on that. I just think we were trying to take things a step further, and create consistency throughout all documentation, and also potentially make it easier for api.drupal.org to generate lists of hook implementations, since the "references" link doesn't do anything right now on hook_xxx.

But fixing the annoying "Implement" -> "Implements" issue before code freeze would be dramatically awesome, and I guess that one won't be as contentious.

@drewish: to be fair, what you reverted is nearly exactly what was posted in comment #14, to which no one naysayed against. But, equally fairly, mentally I skipped over the ramifications of #14 too (I was originally referred to this issue because of the evil evil Druplicon bot telling everyone lies, purportedly, and that's all I was "seeing" and comprehending in this issue.)

@jhodgson, for your benefit, reposted without permission:

[19:15]  <Morbus> drewish: ping?
[19:15]  <drewish> Morbus: pong
[19:15]  <Morbus> drewish: the doc standards have always been ahead of core.
[19:15]  <Morbus> drewish: i'm not a fan of your revert.
[19:16]  <drewish> Morbus: well those were way off of what i'd understood the consensus to be
[19:16]  <drewish> Morbus: a full paragraph about what every hook implementation does?
[19:16]  <drewish> that's totally silly
[19:16]  <Morbus> drewish: hrm. ok. i'm looking at the diff now.
[19:17]  <Morbus> i'm not sure what the hell the other guy is talking about now.
[19:17]  <drewish> but i'm heading out the door right now so i can't really get into it now
[19:17]  <Morbus> no worries.
[19:17]  <drewish> Morbus: fee free to hack away on it
[19:17]  <drewish> Morbus: i might have been a bit too hasty but it seemed like a pretty extreme revision
[19:17]  <drewish> later all
[19:17]  <-- drewish has quit ()

And then I wrote my comment, above. There was no discussion *prior* to the revert.

The whole point of having documentation headers on functions is that someone should be able to figure out from the doc header what a function does, and not have to read the code. Right? Otherwise, let's just scrap doc headers completely and force everyone to read the code for all functions.

Yes and no. It's a slippery slope. If I have 15 menu items, I'm not going to write a doc for my hook_menu that describes each one, because each one is self-documenting, with its #title and #description and #page_callback. Enforcing _too much documentation_ means one thing: that the documentation is going to go outta date, and I'd much rather have *no* documentation than *incorrect* documentation. As above, I feel the same about hook_block: hook_block is _usually_ a router function, like hook_menu and hook_theme. They primarily serve only to give back a list of *other places to go* (i.e., block functions for 'content', menu functions for 'page callback', template files or function for hook_theme). Documenting them, in the way described in #14 (which I should have read and comprehended and argued against long ago, definitely) would be bad, in my opinion.

The same can be said about the difference between hook_form_alter (a general function that could modify more than one form), and hook_form_FORMID_alter(). In hook_form_alter()'s case, the documentation should be as *close to the code as possible* - I would hate to have a massive ten-form modifying function, hundreds of lines long, and have to go back and forth between the header docs and the actual place in the code they document (or, worse, have duplicate documentation in both places). Compare that to hook_form_FORMID_alter, which affects only one thing, and I'd agree: the documentation should go into the header.

Hm. Morbus makes a good point in #38 which supports the simple "Implement[s] hook_foo()." standard we have now.

There's nothing that stops people from documenting particularly tweaky instances of hooks by expanding the PHPDoc:

/**
 * Implements hook_form_alter().
 *
 * On all node forms, add the flerg setting. This will smardy flardy the smoogen googens. 
 */

But in probably half of the hooks, the code is indeed fairly self-explanatory with a quick scan since they're merely registering permissions|menu items|theme functions|etc.

here's a patch that changes core to use "Implements hook_foo()."

well, that was exactly my point in #472642-17: Remove "implementation of" nominalizations from Docblocks. Comments on that issue are closed for whatever reason (which is another indicator for another, monster FAIL).

Marking needs review so test bot picks it up.

Aw shoot. Needs to be in the Drupal project as well.

patch re-roll

OK.

I at least added an "s" in http://drupal.org/node/1354 for the standard header.

Found one instance where patch is modifying a header's full description, not simply the 1 line description:

 /**
- * Implement  hook_comment_unpublish().
+ * Implements hook_comment_unpublish().
  */
 function search_comment_unpublish($comment) {
   // Reindex the node when comments are unpublished.
@@ -1030,9 +1030,9 @@ function search_get_keys() {
  *   by hook_view() and hook_node_view(). This is usually sufficient. You should
  *   only use this mechanism if you want additional, non-visible data to be
  *   indexed.
- * - Implement  hook_search(). This will create a search tab for your module on
+ * - Implements hook_search(). This will create a search tab for your module on
  *   the /search page with a simple keyword search form.
- * - Implement  hook_update_index(). This allows your module to use Drupal's
+ * - Implements hook_update_index(). This allows your module to use Drupal's
  *   HTML indexing mechanism for searching full text efficiently.
  *
  * If your module needs to provide a more complicated search form, then you need

This is the text being changed:

<?php
/**
 * @defgroup search Search interface
 * @{
 * The Drupal search interface manages a global search mechanism.
 *
 * Modules may plug into this system to provide searches of different types of
 * data. Most of the system is handled by search.module, so this must be enabled
 * for all of the search features to work.
 *
 * There are three ways to interact with the search system:
 * - Specifically for searching nodes, you can implement hook_node_update_index()
 *   and hook_node_search_result(). However, note that the search system already
 *   indexes all visible output of a node, i.e. everything displayed normally
 *   by hook_view() and hook_node_view(). This is usually sufficient. You should
 *   only use this mechanism if you want additional, non-visible data to be
 *   indexed.
 * - Implement hook_search(). This will create a search tab for your module on
 *   the /search page with a simple keyword search form.
 * - Implement hook_update_index(). This allows your module to use Drupal's
 *   HTML indexing mechanism for searching full text efficiently.
 *
 * If your module needs to provide a more complicated search form, then you need
 * to implement it yourself without hook_search(). In that case, you should
 * define it as a local task (tab) under the /search page (e.g. /search/mymodule)
 * so that users can easily find it.
 */
?>

This not only reads badly, as I read the discussion it's not the intent of this patch.

All the other changes are fine.

Fixed.

Patch re-roll

Diff code that I think creates new files slipped in at line 4560. File names:
- modules/simpletest/tests/simpletest_test.info, and
- modules/simpletest/tests/simpletest_test.module

Not sure if this is a problem, but saw six instances of: \ No newline at end of file
The patch removes then re-adds the last line. e.g.:
- */
\ No newline at end of file
+ */

Btw, I can't really see myself committing this until after code freeze since it'll cause massive patch breakage. So you can probably hold off mass re-rolls until Paris. But let's make it one of the first patches we commit after that, OK?

We need a new patch for this, basically to change

* Implement hook_foo().

to

* Implements hook_foo().

I'll create one. Since we're really in code freeze now, it should (hopefully) be able to be applied without too much consequence?

Yep, I will happily commit this as soon as it's ready.

Here's a patch that fixes these (thanks to a little Perl script, and then a few manual changes).

Perl? Who needs perl for that?

# find \( -name "*.module" -or -name "*.inc" \) -exec sed --in-place -e "s/^ \* Implement / * Implements /" '{}' ';'

;)

If all that you did is that simple script, your patch didn't find everything mine did. For one thing, you missed the .install and .profile files. For another thing, my patch includes some manual fixes that the script missed (I did a full search of the source code afterwards, combed it, etc.

So I think my patch is more complete...

Right, plus .profile and .install.

# find \( -name "*.module" -or -name "*.inc" -or -name "*.profile" -or -name "*.install" \) -exec sed --in-place -e "s/^ \* Implement / * Implements /" '{}' ';'

Based on the filesize, I'd say that some instances were missing earlier.

Your patch still misses a couple of manual fixes I did in my patch. e.g. line 273 in file.inc. Haven't done a comprehensive survey to find other spots.

Do you have something against my patch in #56? I believe that it applies cleanly and fix the problem comprehensively...

Sorry, that was line 273 in modules/simpletest/tests/file_test.module that was missed by your patch.

oh, thanks! Then we even have more :)

find \( -name "*.module" -or -name "*.inc" -or -name "*.profile" -or -name "*.install" \) -exec sed --in-place -e "s/^ \*\s*Implement / * Implements /" '{}' ';'
find \( -name "*.module" -or -name "*.inc" -or -name "*.profile" -or -name "*.install" \) -exec sed --in-place -e "s/^ \*\s*Implementation of / * Implements /" '{}' ';'

Could be done in one pass, but splitted for clarity.

Ideally though, I'd prefer to defer this until D7 string freeze, because there are still quite some patches in the queue that would break due to this change.

For later reference:

find \( -name "*.module" -or -name "*.inc" -or -name "*.profile" -or -name "*.install" \) -exec sed --in-place -e "s/^ \*\s*Implement hook/ * Implements hook/; s/^ \*\s*Implementation of / * Implements /" '{}' ';'

FYI - I brought this up now because webchick asked me to, in conjunction with another issue.

That aside, I gave your latest patch (#63) a thorough read-through.

Line 273 in modules/simpletest/tests/file_test.module is still not caught (I think that one has 2 spaces between * and Implement).

There are also several places where your approach falls down or manual fixes are needed to bring doc headers into compliance:

From modules/contact/contact.module:

 /**
- * Implement of hook_form_FORM_ID_alter().
+ * Implements of hook_form_FORM_ID_alter().
  *
  * Add the default personal contact setting on the user settings page.
  */

From modules/field/modules/list/list.module:

 /**
- * Implement hook_field_settings_form().
+ * Implements hook_field_settings_form().
  *
  * @todo: If $has_data, add a form validate function to verify that the
  * new allowed values do not exclude any keys for which data already
  * exists in the databae (use field_attach_query()) to find out.
- * Implement the validate function via hook_field_update_forbid() so
+ * Implements the validate function via hook_field_update_forbid() so
  * list.module does not depend on form submission.
  */

From modules/menu/menu.module:

 /**
- * Implement hook_form_FORM_ID_alter() for the node type form.
+ * Implements hook_form_FORM_ID_alter() for the node type form.
  * Adds menu options to the node type form.
  */

From modules/node/node.module:

 /**
- * Implement a Drupal action.
+ * Implements a Drupal action.
  * Sets the status of a node to 1, meaning published.
  */
 function node_publish_action($node, $context = array()) {
@@ -3023,7 +3023,7 @@ function node_publish_action($node, $con
 }
 
 /**
- * Implement a Drupal action.
+ * Implements a Drupal action.
  * Sets the status of a node to 0, meaning unpublished.
  */
 function node_unpublish_action($node, $context = array()) {
@@ -3032,7 +3032,7 @@ function node_unpublish_action($node, $c
 }
 
 /**
- * Implement a Drupal action.
+ * Implements a Drupal action.
  * Sets the sticky-at-top-of-list property of a node to 1.
  */
 function node_make_sticky_action($node, $context = array()) {
@@ -3041,7 +3041,7 @@ function node_make_sticky_action($node, 
 }
 
 /**
- * Implement a Drupal action.
+ * Implements a Drupal action.
  * Sets the sticky-at-top-of-list property of a node to 0.
  */
 function node_make_unsticky_action($node, $context = array()) {
@@ -3050,7 +3050,7 @@ function node_make_unsticky_action($node
 }
 
 /**
- * Implement a Drupal action.
+ * Implements a Drupal action.
  * Sets the promote property of a node to 1.
  */
 function node_promote_action($node, $context = array()) {
@@ -3059,7 +3059,7 @@ function node_promote_action($node, $con
 }
 
 /**
- * Implement a Drupal action.
+ * Implements a Drupal action.
  * Sets the promote property of a node to 0.
  */
 function node_unpromote_action($node, $context = array()) {
@@ -3068,7 +3068,7 @@ function node_unpromote_action($node, $c
 }
 
 /**
- * Implement a Drupal action.
+ * Implements a Drupal action.
  * Saves a node.
  */
 function node_save_action($node) {
@@ -3077,7 +3077,7 @@ function node_save_action($node) {
 }
 
 /**
- * Implement a configurable Drupal action.
+ * Implements a configurable Drupal action.
  * Assigns ownership of a node to a user.
  */
 function node_assign_owner_action($node, $context) {
@@ -3151,7 +3151,7 @@ function node_unpublish_by_keyword_actio
 }
 
 /**
- * Implement a configurable Drupal action.
+ * Implements a configurable Drupal action.
  * Unpublish a node if it contains a certain string.
  *
  * @param $node
 }

From modules/simpletest/tests/database_test.module:

 /**
- * Implement hook_query_TAG_alter(). Called by DatabaseTestCase::testAlterRemoveRange.
+ * Implements hook_query_TAG_alter(). Called by DatabaseTestCase::testAlterRemoveRange.
  */
 function database_test_query_database_test_alter_remove_range_alter(QueryAlterableInterface $query) {

From modules/system/system.module:

 /**
- * Implement a configurable Drupal action. Sends an email.
+ * Implements a configurable Drupal action. Sends an email.
  */
 function system_send_email_action($object, $context) {


...

 /**
- * Implement a configurable Drupal action. Redirect user to a URL.
+ * Implements a configurable Drupal action. Redirect user to a URL.
  */
 function system_goto_action_form($context) {

From modules/toolbar/toolbar.install:

/**
- * Implementation of hook_install().
+ * Implements hook_install().
  *
  * @todo
- *   Implement role based shortcut bars. 
+ * Implements role based shortcut bars. 
  */
 function toolbar_install() {

From modules/user/user.module:

 /**
- * Implement a Drupal action.
+ * Implements a Drupal action.
  * Blocks the current user.
  */
 function user_block_user_action(&$object, $context = array()) {

Yes, that's why I posted the more fail-safe command in #64.

OK, then please propose another patch and I will review it.

I don't have sed available on the box where I have Perl available, and that is why I used Perl to generate my patch. But you can't probably just use Perl or sed to generate a patch. You need to take a look at the results, make sure they make sense, and do some hand-editing. Your revised script is not going to fix those errors where a blank line is needed between

* Implements hook_foo().

and the rest of the comment.

sun -- you seem to want to take this over. Please do so!

We should not squeeze further changes/fixes into this patch. If there is no new line, then there is none. Such fixes need to go into separate issues.

As mentioned before, I don't think it's a good idea to commit a patch like this now. We have merely ~2 weeks left (was it 11/15 or 12/01 again?) for Performance and UX patches, so I'm actually strongly against committing this now.

IMO, it's "API-changeish" since it affects people porting their modules. I'd prefer it get committed sooner than later, since I'm sure we already have a mess out there of Implementation/Implement in D7 contrib. Another 3-4 weeks won't improve things.

OK, sun : If webdries agrees, and if you can run your revised script and make a new patch that doesn't break existing doc, I won't stand in the way of it being applied, just because it doesn't fix the other issues. But normally, patches don't get applied for doc fixes unless they leave the doc in an "up to standards" state for the function headers they affect.

But the latest patch (#63) breaks some existing doc and needs a fix.

New patch sun?

Alternatively, we can go back to reviewing my proposed (and hand-fixed) patch in #56. Re-attached here for convenience, in case Sun doesn't want to generate a patch that doesn't break existing doc.

Here's a re-roll of the patch in #72, with an additional few fixes.

Looks like the patch needs a re-roll again. If that isn't too redundant. I'll take care of it tomorrow AM.

Sorry, unintended status change.

Here is yet another re-roll of this patch.

Yet another reroll of this huge patch, with more manual fixes.

Alright, committed the monster! Thanks all.