Clean up API docs for forum module

Looks pretty good! A few things to fix:

a)

First line of @file docblock should be a one-line summary. Not your fault, but the wording is bad here too (indicated -> indicating). I think we have standards for .tpl.php file docs anyway? Yes:
http://drupal.org/node/1354#templates

Note: This applies to forums.tpl.php at the end of the patch as well as forum-submitted.tpl.php near the beginning, and may also need to be fixed in the other forum module tpls as well?

b)

Maybe "forum containers"?

c)

typo -- constutor -> constructor

d)

In other cleanup patches, we're either calling these "Pre-render callback:" or "Render API callback:" See Sven's patches at #1347890: Clean up API docs for file module and the discussion about standard or not standard on that issue).

Also, callback doc headers like this don't really need @return sections.

e)

Typo I think: form -> forum

f)

Why is this "nodes are new" sentence relevant to this function? Maybe instead describe what "active" means (is it the most "new" posts?) in the $sortby parameter?

g)

Needs . at end

h)

I don't think these should have $ in front of them? And they aren't really "arguments", maybe "elements"?
(same applies to next several functions)

i)

Maybe this should either explain what the code system is, or refer to another function that documents it?

a) It's in the "general" section at the top -- the 80-character limit applies to all summary lines. But I'll add it to the File section too.

h) I don't see why we need a new standard? We only use $ for parameters and variables in PHP. These are not function arguments/parameters. They are array elements. They cannot be referred to with $ in the PHP code, so they also shouldn't be referred to with $ in the docs?

Sorry, didn't notice this before. The .tpl.php files should follow:
http://drupal.org/node/1354#templates

This is looking pretty good. Here are a few things I noticed:

1) css files lack @file header.

2) The doc headers of most methods in forum.test do not follow standards (function standards apply).

3) This function doc does not follow standards:

4) Some preprocess functions are documented with "Processes ...", while other with "Preprocesses ..." --- best be consistent?

New patch with the requested changes.

Tagging.

Also @xenophyle: are you still planning to work on this? If so, please respond. If not, we'll unassign it so someone else can. Thanks!

Updated patch from #10 with new hook_menu() callback documentation standards.

There are two places where there is unnecessary white-space in lines 196 and 649, and '@ingroup themeable' is also being added to the tpl.php files under issue http://drupal.org/node/716496 , but other than that, it looks ok.

It looks pretty good, thanks! But it's not quite ready to be committed.

a) forum.admin.inc

Last line there needs to end in .

b) I took a look at forum.module, and this also needs to be fixed (around line 603):

See http://drupal.org/node/1354#hookimpl for standards (2nd example)

c) Also in forum.module:

There is no such hook. This is really an example of:
http://drupal.org/node/1354#hookimpl (example 3)

d) The *.css files do not have @file blocks, and should.

Dang. We are over thresholds for major/critical issues, and this patch will conflict with the fix for
#787652: Forum vocabulary alterations possibly obsolete -- possible to delete forum vocab
So I can't commit it now, and once that gets in (should be in a day or two), I expect this patch will need a reroll.

The patch looks pretty good though! The only things I think might need to be changed:

a)

- I think RTL should be written out as right-to-left.
- "the forum module" --> Forum should be capitalized when it's the name of the module. It would be good to check this in the other @file headers for the other files too. [I specifically noticed that forum.css has the same problem; probably others too.]

b) In forum.module:

There is no theme_block(). It should be block.tpl.php.

c) in forum.module:

- These two should use the same format -- I think the second should say "Preprocesses varialbes for whatever.tpl.php"
- The next several functions in the file have the same problem.

d) in forum.test:

We actually are not putting doc blocks on test methods setUp(), tearDown(), or getInfo().

#21: forum-clean-up-documentation-1357138-21.patch queued for re-testing.

It looks like that other patch was rolled back and this one still applies cleanly. Any chance we could slip this one in?

This is getting very close! Looking at the patch in #21, I found a few more things to fix:

a) forum-list.tpl.php [I wouldn't have even mentioned this except there were other things, so might as well get this right too]:

Normally we would word this:
"TRUE if ...; FALSE if ..."
rather than "Is TRUE if.... etc."

b) forum-list.tpl.php

True -> TRUE

c) forum-topic-list.tpl.php

Is "outputtable" really a word, and what does it mean here? Maybe this could be reworded to say "A string representing ... . Safe to output." like in the other list items?

d)

Needs comma before "and".

e) in forum.module

Normally we would leave out "returns" inside @return docs.

f) in forum.test

Why was this indented?

duh. Thanks xjm for reading the standards. Wonder when that went in... I don't remember it but it makes some sense. :)

Could we get an interdiff?

Thanks @jstoller. I confirmed that the interdiff in #31 covers all the points in #26.

I reviewed the latest pacth in #29 and found these things:

1. +++ b/core/modules/forum/forum-submitted.tpl.phpundefined
@@ -2,18 +2,20 @@
  * - $time: How long ago the post was created.
- * - $topic: An object with the raw data of the post. Unsafe, be sure
- *   to clean this data before printing.
+ * - $topic: An object with the raw data of the post. Unsafe: be sure to clean
+ *   this data before printing.

   I'd use a semicolon here, but I'm not sure that this isn't just a personal preference on my part.

2. +++ b/core/modules/forum/forum.admin.incundefined
@@ -2,7 +2,22 @@
+ * @param $edit
+ *   Associative array containing a forum term to be edited. Defaults to an
+ *   empty array.

@@ -20,11 +35,13 @@ function forum_form_main($type, $edit = array()) {
+ * @param $edit
+ *   Associative array containing a forum term to be added or edited.

@@ -120,11 +137,13 @@ function theme_forum_form($variables) {
+ * @param $edit
+ *   Associative array containing a container term to be added or edited.

   I think this parameter is optional, so we should document it as such.

3. +++ b/core/modules/forum/forum.moduleundefined
@@ -491,8 +492,8 @@ function forum_comment_publish($comment) {
+ * The Comment module doesn't call hook_comment_unpublish() when saving
+ * individual comments so we need to check for those here.

   I think there should be a comma after "individual comments".

4. +++ b/core/modules/forum/forum.moduleundefined
@@ -855,6 +866,23 @@ function _forum_topics_unread($term, $uid) {
+ *   - 1: Date - newest first.
+ *   - 2: Date - oldest first.
+ *   - 3: Posts with the most comments first.
+ *   - 4: Posts with the least comments first.

   This is out of the scope of the patch, but, wow, magic numbers. Can we open a followup issue to replace these with constants?

5. +++ b/core/modules/forum/forum.moduleundefined
@@ -971,15 +999,16 @@ function forum_preprocess_block(&$variables) {
+ * @param $variables
+ *   An array containing the following elements:
+ *   - forums
+ *   - topics
+ *   - parents
+ *   - tid
+ *   - sortby
+ *   - forum_per_page

@@ -1050,12 +1079,13 @@ function template_preprocess_forums(&$variables) {
+ * @param $variables
+ *   An array containing the following elements:
+ *   - forums
+ *   - parents
+ *   - tid

@@ -1096,13 +1126,14 @@ function template_preprocess_forum_list(&$variables) {
+ * @param $variables
+ *   An array containing the following elements:
+ *   - tid
+ *   - topics
+ *   - sortby
+ *   - forum_per_page

@@ -1163,14 +1194,15 @@ function template_preprocess_forum_topic_list(&$variables) {
+ * @param $variables
+ *   An array containing the following elements:
+ *   - new_posts
+ *   - num_posts = 0
+ *   - comment_mode = 0
+ *   - sticky = 0
+ *   - first_new

   We should change this to use standard list formatting and add descriptions for the parameters, correct?

6. +++ b/core/modules/forum/forum.moduleundefined
@@ -1198,9 +1230,14 @@ function template_preprocess_forum_icon(&$variables) {
+ * @param $variables
+ *   An array containing the following elements:
+ *   - topic

   This seems to be missing the parameter description? Also, are there other keys?

7. +++ b/core/modules/forum/forum.pages.incundefined
@@ -2,11 +2,20 @@
+ * @return
+ *   Themed forum listing.

   Can we specify here whether it is HTML or a render array?

8. +++ b/core/modules/forum/forum.testundefined
@@ -5,14 +5,49 @@
+/**
+ * Represents a test case for menu administration.
+ */

   This docblock seems to be entirely wrong to me. Menu administration?

   How about simply "Provides automated tests for the Forum module."?

9. +++ b/core/modules/forum/forum.testundefined
@@ -190,9 +225,10 @@ class ForumTestCase extends DrupalWebTestCase {
+   * @param object $user
+   *   The logged in user.

   "Logged-in" should be hyphenated.

10. +++ b/core/modules/forum/forum.testundefined
@@ -287,15 +323,16 @@ class ForumTestCase extends DrupalWebTestCase {
-   *   Forum parent (default = 0 = a root forum; >0 = a forum container or
+   *   The forum parent (default = 0 = a root forum; >0 = a forum container or
    *   another forum).

    This pseudocode is confusing. While we're updating this line, can we write out the condition in English?

11. +++ b/core/modules/forum/forum.testundefined
@@ -368,15 +405,15 @@ class ForumTestCase extends DrupalWebTestCase {
-   *   True if $forum is a container.
+   *   TRUE if $forum is a container, FALSE otherwise.

    I believe this should either have a semicolon instead of a comma, or use the word "or".

Once again, an interdiff would be helpful. Thanks!

#33: forum-clean-up-documentation-1357138-33.patch queued for re-testing.

This patch looks really good to me! The only thing I would change (assuming it still applies -- sorry for the delay in reviewing it) is:

forum.module - function template_preprocess_forum_icon():

if -> whether in the first two bullet points.

Can someone do a very quick reroll with that (very minor) change? Thanks!

Sorry it took me so long to get back to this. Alas, my last patch no longer applies—since the test file was moved, renamed and modified—but I was able to make it work.

This patch includes the requested changes from #36. I'm heading off the grid and into the woods for the next week, so fingers crossed this is the last of it. :-)

Rerolled.

Thanks! I just committed this patch to 7.x. Another module cleaned up!