Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xenophyle’s picture

Status: Active » Needs review
FileSize
20.04 KB
jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good! A few things to fix:

a)

 /**
  * @file
- * Default theme implementation to format a simple string indicated when and
- * by whom a topic was submitted.
+ * Default theme implementation to format a simple string indicated when and by
+ * whom a topic was submitted.

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)

 /**
- * Returns a form for adding a container to the forum vocabulary
+ * Form constructor for adding and editing containers.

Maybe "forum containers"?

c)

+ * Form construtor for confirming deletion of a forum taxonomy term.

typo -- constutor -> constructor

d)

+ * #pre_render callback: Lists nodes based on the element's #query property.

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)

+ * @return
+ *   The number of new posts in the form that have not been read by the user.

Typo I think: form -> forum

f)

+/**
+ * Gets all the topics in a forum.
+ *
+ * Nodes are new if they are newer than NODE_NEW_LIMIT.
+ *

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)

 /**
- * Process variables for forums.tpl.php
+ * Processes variables for forums.tpl.php

Needs . at end

h)

+ * @param $variables
+ *   An array containing the following arguments:
+ *   - $forums
+ *   - $topics
+ *   - $parents
+ *   - $tid
+ *   - $sortby
+ *   - $forum_per_page

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)

 * @param $sortby
+ *   A code specifying how the topics should be sorted.

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

xenophyle’s picture

a) I wasn't sure if file headers had to be limited to 80 characters. I would like to update to documentation to say so, but I am not able to edit that page.

h) I also thought this was weird, but it seems that all the other modules do it this way. Shall we make an official standard?

jhodgdon’s picture

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?

xenophyle’s picture

Status: Needs work » Needs review
FileSize
20.26 KB
jhodgdon’s picture

Status: Needs review » Needs work

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

xenophyle’s picture

Status: Needs work » Needs review
FileSize
23.39 KB

For the .tpl files, I modified the first line under @file and moved the "subvariable" documentation up.

sven.lauer’s picture

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:

/**
 * Entity URI callback used in forum_entity_info_alter().
 */
function forum_uri($forum) {

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

sven.lauer’s picture

Status: Needs review » Needs work
xenophyle’s picture

Status: Needs work » Needs review
FileSize
29.8 KB

New patch with the requested changes.

jhodgdon’s picture

Status: Needs review » Needs work

We had a discussion on #1315992: No standard for documenting menu callbacks and we have (unfortunately) changed the standard for hook_menu() callbacks:
http://drupal.org/node/1354#menu-callback

So this patch will need an update (no paths and they need @return sections).

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

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!

Anonymous’s picture

Assigned: xenophyle » Unassigned

Since there hasn't been activity for 4+ weeks, I'm going to move this back to unassigned.

jstoller’s picture

Assigned: Unassigned » jstoller
Status: Needs work » Needs review
FileSize
30.2 KB

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

jstoller’s picture

Missed a couple white space characters.

hosef’s picture

Status: Needs review » Needs work

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.

hosef’s picture

Status: Needs work » Reviewed & tested by the community

Ignore my previous comment this looks like it is ready to go.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

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

a) forum.admin.inc

@@ -176,9 +195,13 @@ function forum_form_container($form, &$form_state, $edit = array()) {
 }
 
 /**
- * Returns a confirmation page for deleting a forum taxonomy term.
+ * Form constructor for confirming deletion of a forum taxonomy term.
+ *
+ * @param $tid
+ *   ID of the term to be deleted

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):

/**
 * Implements hook_form_BASE_FORM_ID_alter().
 */
function forum_form_node_form_alter(&$form, &$form_state, $form_id) {

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

c) Also in forum.module:

/**
 * Implements hook_preprocess_block().
 */
function forum_preprocess_block(&$variables) {

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.

jstoller’s picture

Status: Needs work » Needs review
FileSize
31.5 KB

New patch with requested changes.

jhodgdon’s picture

Status: Needs review » Needs work

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)

-- a/core/modules/forum/forum-rtl.css
+++ b/core/modules/forum/forum-rtl.css
@@ -1,3 +1,7 @@
+/**
+ * @file
+ * RTL styling for the forum module.
+ */

- 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:

@@ -962,7 +990,7 @@ function forum_get_topics($tid, $sortby, $forum_per_page) {
 }
 
 /**
- * Implements hook_preprocess_block().
+ * Implements hook_preprocess_HOOK() for theme_block().

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

c) in forum.module:

@@ -971,15 +999,16 @@ function forum_preprocess_block(&$variables) {
 }
 
 /**
- * Process variables for forums.tpl.php
+ * Preprocesses variables for forums.tpl.php.
...
@@ -1050,12 +1079,13 @@ function template_preprocess_forums(&$variables) {
 }
 
 /**
- * Process variables to format a forum listing.
+ * Preprocesses variables to format a forum listing.

- 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:

@@ -24,7 +62,7 @@ class ForumTestCase extends DrupalWebTestCase {
   }
 
   /**
-   * Enable modules and create users with specific permissions.
+   * Implements DrupalWebTestCase::setUp().
    */
   function setUp() {

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

jstoller’s picture

Status: Needs work » Needs review
FileSize
32.5 KB

New patch with changes requested in #20.

jhodgdon’s picture

jhodgdon’s picture

That other patch got committed so I requested a retest -- probably this needs a reroll.

jstoller’s picture

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

jhodgdon’s picture

No, we can't slip this one in. That one is on the critical/major list. This one isn't. We are over thresholds for criticals/majors. Sorry.

jhodgdon’s picture

Status: Needs review » Needs work

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]:

+ *   - $forum->is_container: Is TRUE if the forum can contain other forums. Is
+ *     FALSE if the forum can contain only topics.

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

b) forum-list.tpl.php

+ *   - $forum->new_topics: True if the forum contains unread posts.

True -> TRUE

c) forum-topic-list.tpl.php

+ *   - $topic->created: An outputtable string representing when the topic was
+ *     posted.
+ *   - $topic->last_reply: An outputtable string representing when the topic was
+ *     last replied to.

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)

+++ b/core/modules/forum/forum.install
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Install, update and uninstall functions for the forum module.
+ * Install, update and uninstall functions for the Forum module.
  */

Needs comma before "and".

e) in forum.module

+ * @return
+ *   If the user has previously viewed the node, returns the timestamp of the
+ *   view; otherwise returns NODE_NEW_LIMIT.
+ */
 function _forum_user_last_visit($nid) {

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

f) in forum.test

@@ -534,10 +574,10 @@ class ForumTestCase extends DrupalWebTestCase {
   }

... 
-   * @todo The logic here is completely incorrect, since the active
-   * forum topics block is determined by comments on the node, not by views.
+   * @todo The logic here is completely incorrect, since the active forum topics
+   *   block is determined by comments on the node, not by views.

Why was this indented?

xjm’s picture

Regarding f, the standard says @todo are supposed to be indented: http://drupal.org/node/1354#todo

jhodgdon’s picture

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

jstoller’s picture

Status: Needs work » Needs review
FileSize
32.51 KB

New patch with changes requested in #26.

jhodgdon’s picture

Could we get an interdiff?

jstoller’s picture

FileSize
3.11 KB

Here's an interdiff between #21 and #29.

xjm’s picture

Status: Needs review » Needs work

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!

jstoller’s picture

Status: Needs work » Needs review
FileSize
9.46 KB
34.37 KB

There have been several changes to the Forum module preventing the patch from #29 from applying cleanly. This patch addresses those conflicts, as well as all the changes requested in #32 and a few additional typos I found.

Someone more familiar than I with the workings of this module should look closely at my changes to the preprocess function comments, as requested in #32 points 5 & 6. I did my best, but I'm not 100% confident this is what you want.

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7, +docs-cleanup-2011

The last submitted patch, forum-clean-up-documentation-1357138-33.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Needs reroll

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():

+ *   - comment_mode: An integer indicating if comments are open, closed, or
+ *     hidden.
+ *   - sticky: Indicates if the topic is sticky.
+ *   - first_new: Indicates whether this is the first topic with new posts.

if -> whether in the first two bullet points.

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

jhodgdon’s picture

Status: Needs review » Needs work
jstoller’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
35.03 KB

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. :-)

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

Looks good! Committed to 8.x. Ready for backport to 7.x.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
33.79 KB

Rerolled.

eärendil’s picture

Assigned: jstoller » eärendil
Status: Needs review » Reviewed & tested by the community

Documentation looks good.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

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

Automatically closed -- issue fixed for 2 weeks with no activity.