GHOP #39: Update PHP documentation blocks in core modules

Subscribing.

It was much more work than I had anticipated, but here it is at last. It's a patch
against "drupal-6.0-beta4" root directory. I hope it will be of any use :)

This is so nice! :-) ... and it's ready!

A quick read through of the text makes most of it look good. However, I did notice a few problems.

- The comment blocks do not conform to Drupal standard. They should read like this:

/**
 * Parse a feed and store its items.
 *
 * @param $data 
 *   The feed data.
 * @param $feed 
 *   An associative array describing the feed to be parsed.
 * @return 0 on error, 1 otherwise.
 */

Note that description text is on the following line, indented 2 spaces, rather than being on the same lin.e

- Hook implementations should be marked "Implementation of hook_menu()", which it looks like you did, but currently do not use @see hook_menu(). There's currently no actual function called hook_menu(), so we shouldn't @see it. At some point in the future we may move toward doing that, but currently it's not the standard. (Ibid for all other hooks, of course.)

- The function parameters to hook implementations are also not needed, as those are standardized. Just the "Implementation of hook_foo()" string is sufficient. Similarly, form definition functions should be @ingroup forms, but do not need their parameters or return defined (unless there's a non-standard parameter being passed in, like to the form builder function itself). Those are standard for all forms.

- Don't over-use @see. There's no hard-and-fast rule about using @see yet, but in general it should only be used if the function being referenced would only ever be used as part of the function referencing it. For example, form validate and submit handlers have no actual use except as part of a form, so the form builder function @see's them so that going to the form builder's API page will link to the validate and submit handlers. Where else it should be used is not well defined, but I'd say err on the conservative side. No sense adding tags that we're just going to take out later down the line when we do establish a standard.

- Functions should always be separated from the next/previous function (and/or its DocBlock) by a single carriage return. I think a few multi-line breaks crept in there.

- The first line of each DocBlock should be a short and sweet single line, no more than a sentence. Additional descriptive text, which is good where appropriate, should be separated from the first line by one blank line in the DocBlock. That way there's a short description that is good for lists, and a longer description (which could be one line or several paragraphs, as needed) that can be pulled up for more information.

The @return docs should also be on a separate line, just as with @param.

I apologize for the mistakes. Here is a revised version, I really hope it's going to be OK this time.

I tried applying the patch but it failed on the comment.module, system.admin.inc and system.module.

You're getting very close but I think some of Crell's comments in #7 still apply:
- It's minor but the formatting of the @params is still incorrect. The variable name should be on the first line with @param, and the description by itself on the second. Perhaps it would be helpful to look at the Doxygen formatting conventions.
- There are quite a few @see's that don't seem very helpful, e.g. the @see _aggregator_page_list() on aggregator_page_source()'s comment is probably actually a bad idea. Documenting the implementation leads people to believe that's the only way it can be done and ties you to a single implementation.
- aggregator_init() is an implementation of hook_init() and should be documented as such.

And, you added a few @ingroup forms, but it looks like you missed aggregator's forms.

No need to apologize, vitezslav. You're not really a Drupal developer until you've totally and inexcusably screwed up on something at least three times. :-) At first glance, this looks a lot better. I'll try to go over the text later tonight and get back to you.

Oops, cross-posted.

Another try. According to http://drupal.org/node/1354 submit and validation handlers shouldn't be @ingroup forms, so I removed these comments. Insteads, the handlers are referenced by @see from their form builders (the formatting conventions again).

Applying the patch Works For Me^TM.

vitezslav.smid, i haven't tried re-applying it yet but make sure you do a "cvs up" before your roll the patch.

No need for cvs - I always succeeded patching against the latest dev tarball. But it seems that vitezslav.smid is patching against the latest beta (beta4), which is a few days old now. Please download the 6.x-dev version (and refresh before each new attempt, it's contents changes daily) - that's the newest code all developers are working on. Beta releases are only for testing by wider audience, not refreshed after release anymore. http://drupal.org/node/97368

A patch against 6.x-dev. There were, of course, many changes to the code since beta4. I have edited the patch manually to keep just my changes to the documentation. I have little experience with diffs and patching so I don't know if this is the right way. Applying works for me, however.

Patch now applies cleanly, fine in this aspect :) (BTW - if needed to refresh later, just apply to new 6.x-dev, check/edit any failed bits only (offset doesn't hurt, fuzz is mostly just a reminder to double check that bit), and then re-create using diff - that's how I do it usually).

Now to the patch itself: It's big, and I read through it only just briefly. The texts in there looked good as far as I see, but again I only reviewed carefully less than 30%. Needs another set of eyes.

I found a problem though - there seems to be still some junk from the manual re-roll just done (i.e. some things unrelated to the docs update, most probably generated from the difference between beta4 and -dev and unnoticed):
- The patch removes system_update_6041() function inside system.install - obviously shouldn't do that.
- At the top of system.module, there's the version number defined - the patch have it from beta4 still. Shouldn't touch that.
- To be checked: there are some changed lines, which are seemingly identical (not changed at all) - not sure whether we're undoing some of the previously done whitespace-cleanups here? I spotted such a line inside system_block(), the top $Id line on system.install, and maybe missed some more. Would be nice to check, why we change these.

Otherwise this is a great piece of work, I have to say. Leaving as CNR in hope that someone take a closer look at the texts.

I've fixed the errors mentioned by JirkaRybka but the patch is a failure. Three hunks keep failing whatever I do. I've been working on it for hours and I'm getting desperate now. No idea what to do.

Seems like a patch edited by hand, some indentation on unchanged lines didn't match, $Id line skipped in system.module hunk - but still 3 hunks failed whatever I do. Patches should not be edited manually (perhaps I'm wrong, but it seems like it here). Re-rolling is easily done as follows (my personal practice, note that nearly everyone have own beliefs, and also CVS is really popular here):
- Download latest 6.x-dev, unpack and rename the folder
- put the old patch somewhere else
- inside the unpacked and renamed folder apply the patch patch -p0 < path/filename.patch
- see which files/hunks failed
- correct the failures manually (go to these files, see the rejected bits and original state now saved in extra files, and re-create the missing changes in files)
- unpack a fresh copy of 6.x-dev from the downloaded archive aside
- roll a brand new patch file by doing a diff - I generally use inside the renamed/edited folder diff -urp --strip-trailing-cr ../drupal-6.x-dev ./ > ../filename.patch which grabs the fresh 6.x-dev from parent folder and puts the new patch there, and ignores any Windows-style line endings (which are often cause of invisible problems).
- check whether the new patch is OK (contains everything, but nothing unwanted).

Now I attempted to re-roll it for you, but I leave the last point (check if it's OK) to you. It's getting really late night here.

Also there was still the '6.0-beta4' version in the patch - I fixed that.

Further review needed.

Thank you, you have saved my life here. The patch needed just a few tiny changes. It applies cleanly for me. You are right, I did edit my previous patch manually - and I said I didn't know if it is the right way. Well, now I know better :). Thanks again!

The latest version which needs review is attached. By the way, somebody else has been working on the documentation lately. Several functions have comments which weren't there when I started working on the issue. Mostly they have the same meaning as mine, only rephrased.

I tried applying the patch to D6 head and it did apply (though with some fuzz and offset). The Doxygen looks fine for the most part, but I didn't take the time to read through each docblock to check the validity of the code comments.

Hm, I looked into the patch, and found some stuff, which I did not like:

+ * @return
+ *   $comments
+ *   An array of comment objects each containing a nid,

Hm, we don't name what we return ($comments).

  * @param $node
+ *
  *   The node which comment(s) needs rendering.
  * @param $cid
+ *
  *   Optional, if given, only one comment is rendered.

These newlines look very odd.

 /**
  * Generate the basic commenting form, for appending to a node or display on a separate page.
  *
+ * @param $title
+ *   Not used.

Ugh? It is really not used?

- * @ingroup themeable
+ * @param $form
+ *   The form structure.
+ * @themeable

Replaced an @ingroup themeable with a @themeable??

This looks like in need of a little bit of a cleanup still.

Attached is a new version of the patch. Made against the newest -dev version.

Hm, we don't name what we return ($comments).

I wouldn't do somthing like this intentionally. Mistakes of this kind appeared here and there and I was actually fixing them. This one escaped my attention. Fixed.

These newlines look very odd.

mēa culpa. Fixed.

Ugh? It is really not used?

If we are talking about comment_form() then yes, the variable is not referenced inside the function body. Or so it seems to me, please correct me if I am wrong.

Replaced an @ingroup themeable with a @themeable??

No idea how this could have happened, I apologize. Fixed.

Good, I quickly looked over the patch, and did not identify any obvious problems introduced by this patch (I did not check validity of comments one by one, as this was done above), so committed, thanks.