Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sven.lauer’s picture

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

Status: Needs review » Needs work

This is mostly good, thanks! A few things to fix:

a)

+/**
+ * @file
+ * Attach behaviors to the node form.
+ */
 (function ($) {
 

Needs a blank line after the @file docblock.

b)

+ * @see node_menu()
  */
 function comment_admin($type = 'new') {

I think this should be @see comment_menu() ?

c)

+ * Form submit handler for comment_admin_overview().

submit -> submission

d)

+ * Form constructor for confirmation form for bulk comment deletion.

--> for the conformation form... (needs "the"). Also:

+ * Form constructor for confirmation form for comment deletion.

and possibly other similar lines.

e)

+ * Form constructor for confirmation form for bulk comment deletion.
  *
- * @param $form_state
- *   An associative array containing the current state of the form.
  * @return
  *   TRUE if the comments should be deleted, FALSE otherwise.
+ *
  * @ingroup forms
  * @see comment_multiple_delete_confirm_submit()
  */

If this is a form constructor, it can't have a true/false return value?

f)

 /**
  * @file
- * Hooks provided by the Comment module.
+ * Hooks provided by the comment module.
  */

Original was better here. Refer to modules as "the Comment module" or "comment.module" (without "the").

g)

 /**
- * The comment is being updated.
+ * Respond updates to a comment.

--> Respond to updates...

h)

 /**
  * @} End of "addtogroup hooks".
- */
+ */
\ No newline at end of file

I wasn't sure here if you removed the newline by mistake, or if the original had no newline?

i)

/**
- * Menu loader callback for Field UI paths.
+ * Menu loader callback for Field UI paths of the comment module.

Probably should follow http://drupal.org/node/1354#menu-callback format? Such as:
Menu loader callback: Loads the ...

j)

+ * @return
+ *  The comment bundle name corresponding to the node type.

Second line needs one more space of indentation.

k)

  * @return The result or FALSE on error.

Needs reformatting (newline after @return). There's at least one farther down too:

    * @return boolean Comment found.

l)

+ * Page callback: Display the comment editing form.
+ *
+ * Path: comment/%comment/edit
+ *
+ * @params $comment

- First line: Display -> Displays
- Last line: @params -> @param

m)

+ * Form constructor for blacklisted keywords form.

Needs "the". Same for the next few docblock starting lines. Also this group:

+   * Sets comment subject setting.

(and following docblocks). Actually... "Sets comment subject setting." ??!? That makes no sense to me. How about just "Sets the comment subject."?

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
38.95 KB

Attached patch addresses these issues. Thank you for your patience ;)

With respect to m), you suggestion "Sets the comment subject." would not work, because the function does not actually set a subject, but rather sets a value for the "subject setting" (i.e. the setting specifying whether a subject field should be displayed on comment forms).

I tried to make this clearer and settled on the format "Sets setting: ", in order to avoid the awkward "Sets the 'xyz' setting" formulation. I could re-roll if you have a better suggestion for those.

jhodgdon’s picture

Status: Needs review » Needs work

Hm.

+   * Sets setting: Should the comment form display a subject field?

I am not fond of this formatting, especially the question mark at the end, and the "sets setting" part...

How about something like this:

Sets a value telling whether the comment form should have a subject field.

or

Sets the value governing whether comment forms have subject fields.

or something like that?

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
39.94 KB

I did change all these to description along the lines of your suggestion.

I also fixed a number of @file headers which referred to "comment module" etc. instead of "the Comment module".

jhodgdon’s picture

Status: Needs review » Needs work

This is looking pretty good! A couple of things to fix still:

a) Extra blank line here:

+ *   The comment the action is being performed on.
+ *
  */
 function hook_comment_unpublish($comment) {

b) I don't think we want @ingroup themeable for theme preprocess functions?

 /**
- * Process variables for comment.tpl.php.
+ * Processes variables for comment.tpl.php.
  *
  * @see comment.tpl.php
+ * @ingroup themeable
  */
 function template_preprocess_comment(&$variables) {

http://drupal.org/node/1354#themeable

c)

 /**
  * @file
- * Tests for comment.module.
+ * Tests for the Comment.module.

Either comment.module or the Comment module (no dot in the middle).

d) should fix formatting for the @params here as well as the @return that you fixed:

    * @param object $comment Comment object.
    * @param boolean $reply The comment is a reply to another comment.
-   * @return boolean Comment found.
+   *
+   * @return boolean
+   *   Comment found.

e) Needs to end in .:

    * @param int $mode
-   *   Preview value.
+   *   The preview mode: DRUPAL_DISABLED, DRUPAL_OPTIONAL or DRUPAL_REQUIRED

This also needs . at end:

+   *   - 2: Contact information required
sven.lauer’s picture

Status: Needs work » Needs review
FileSize
40.06 KB

Fixing the issues mentioned in #6.

jhodgdon’s picture

Status: Needs review » Needs work

OK. There are still a few issues here (sorry if I missed them the last review):

a) (header/handler):
+ * Form validation header for comment_admin_overview().

b) This line needs some sentence rearranging:

+ * Form constructor the for confirmation form for comment deletion.

c) And if you have to re-roll anyway, you could fix this:
+ * An object representing info on the node type. The only property that is
Should say "An object representing the content type."

d) This should also be fixed. Use "ID" not "cid" or "id" in text, to refer to an identifier:

+ * Loads the entire comment by cid.
  *
  * @param $cid
  *   The identifying comment id.

Here's another one:
* Node-id to count comments for.

e) This needs a "the":
+ * Page callback: Publishes specified comment.

f) Lists generally need a header before them:

    * @param integer $level
-   *   Anonymous level.
+   *   - 0: No contact information allowed.
+   *   - 1: Contact information allowed but not required.
+   *   - 2: Contact information required.

So before 0: there should be a line saying something like "The level of contact information allowed:"

g) There are still a few places that say "comment module" instead of "Comment module" or "comment.module", such as:
+ * Tests that comment module works when enabled after a content module.
+ * Tests that comment module works correctly with plain text format.

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
42.61 KB

Fixing those.

With respect to (d) and (g): I grepped through the module directory to make sure I have not missed any of those. Found some (d) and a lot of (g) additional instances, mostly in lines I had not touched so far.

xjm’s picture

Status: Needs review » Needs work

Alright, I read through the patch but did not apply it. Some of what follows are suggestions to change wordings that existed before the patch, so we can fix them here or submit followups, whichever is better.

  1. +++ b/core/modules/comment/comment-node-form.jsundefined
    @@ -1,3 +1,7 @@
    +/**
    + * @file
    + * Attach behaviors to the node form.
    + */
    

    My first thought on reading this is "Attaches what behaviors?" (for comments, or the Comment module, etc.)

  2. +++ b/core/modules/comment/comment.admin.incundefined
    @@ -182,12 +194,8 @@ function comment_admin_overview_submit($form, &$form_state) {
    + * Form constructor for the confirmation form for bulk comment deletion.
    

    Presumably this confirmation form is the result of clicking a button on another form. Can we add an @see to the parent form constructor?

  3. +++ b/core/modules/comment/comment.admin.incundefined
    @@ -248,7 +263,10 @@ function comment_confirm_delete_page($cid) {
    + * Form constructor for the confirmation form for comment deletion.
    

    Looks like this form doesn't have its own menu path but is instead used in comment_confirm_delete_page(), so let's add @see linking these two functions.

  4. +++ b/core/modules/comment/comment.moduleundefined
    @@ -4,7 +4,7 @@
    - * When enabled, the Drupal comment module creates a discussion
    + * When enabled, the Drupal Comment module creates a discussion
    

    This is sort of a trivial point, but do we need to specify Drupal here?

  5. +++ b/core/modules/comment/comment.moduleundefined
    @@ -140,9 +140,17 @@ function comment_entity_info() {
    - * Menu loader callback for Field UI paths.
    + * Menu loader callback: Loads the comment bundle name for Field UI paths.
    ...
     function comment_node_type_load($name) {
    

    The string "menu loader callback" doesn't seem to be used anywhere else in core that I could find, nor anything close to it. This is the bit of the hook_menu() doc that explains wildcard autoloaders:

    Registered paths may also contain special "auto-loader" wildcard components in the form of '%mymodule_abc', where the '%' part means that this path component is a wildcard, and the 'mymodule_abc' part defines the prefix for a load function, which here would be named mymodule_abc_load(). When a matching path is requested, your load function will receive as its first argument the path component in the position of the wildcard; load functions may also be passed additional arguments (see "load arguments" in the return value section below).

    Can we check and see how other callbacks like this are documented? (Edit: If they're documented at all. Maybe we need to be trend-setters.)

  6. +++ b/core/modules/comment/comment.moduleundefined
    @@ -764,6 +783,9 @@ function comment_node_page_additions($node) {
    + * @return
    + *   An array of comment IDs to be displayed.
    + *
    

    Semantic nitpick: the comments are being displayed, not their IDs.

  7. +++ b/core/modules/comment/comment.moduleundefined
    @@ -858,12 +880,10 @@ function comment_get_thread($node, $mode, $comments_per_page) {
    - * Loop over comment thread, noting indentation level.
    + * Loops over a comment thread, noting indentation level.
    

    I know this text exists before the patch, but what does "noting indentation level" mean here? Maybe we could make the summary a little clearer.

  8. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1015,14 +1035,13 @@ function comment_build_content($comment, $node, $view_mode = 'full', $langcode =
    - * Adds reply, edit, delete etc. depending on the current user permissions.
    + * Adds reply, edit, delete etc. links, depending on user permissions.
    

    Should there be a comma after "delete" here?

  9. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1077,7 +1096,7 @@ function comment_links($comment, $node) {
    - * Construct a drupal_render() style array from an array of loaded comments.
    + * Constructs a drupal_render() style array from an array of loaded comments.
    

    Maybe just "Constructs a render array," possibly with an @see to drupal_render()? (That's the form used elsewhere in the patch for render arrays.)

  10. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1703,14 +1723,16 @@ class CommentController extends DrupalDefaultEntityController {
    - * Get number of new comments for current user and specified node.
    + * Gets number of new comments for current user and specified node.
    

    We can probably add some articles here.

  11. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1703,14 +1723,16 @@ class CommentController extends DrupalDefaultEntityController {
    + *
    + * @return
    + *   The result or FALSE on error.
    

    Should we also improve this text while we're reformatting it? Presumably "the result" is the number of comments on the node. And what might cause an error?

  12. +++ b/core/modules/comment/comment.moduleundefined
    @@ -2032,7 +2063,7 @@ function comment_form($form, &$form_state, $comment) {
     /**
    - * Build a preview from submitted form values.
    + * Builds a preview from submitted form values.
      */
     function comment_form_build_preview($form, &$form_state) {
    

    This is the form submission handler for the Preview button for some form, correct? If so, let's use the Form API-style docblock.

  13. +++ b/core/modules/comment/comment.moduleundefined
    @@ -2208,7 +2243,10 @@ function comment_form_submit_build_comment($form, &$form_state) {
     /**
    - * Process comment form submissions; prepare the comment, store it, and set a redirection target.
    + * Form submission handler for comment_form().
    + *
    + * @see comment_form_validate()
    + * @see comment_form_submit_build_comment()
      */
    

    Maybe we could add the deleted information from the old summary in a second paragraph? Not sure if this is necessary or not.

  14. +++ b/core/modules/comment/comment.moduleundefined
    @@ -2256,7 +2294,7 @@ function comment_form_submit($form, &$form_state) {
    - * Process variables for comment.tpl.php.
    + * Processes variables for comment.tpl.php.
    
    @@ -2364,7 +2402,7 @@ function theme_comment_post_forbidden($variables) {
    - * Process variables for comment-wrapper.tpl.php.
    + * Processes variables for comment-wrapper.tpl.php.
    

    Should these say "preprocesses" rather than "processes"?

  15. +++ b/core/modules/comment/comment.moduleundefined
    @@ -2454,7 +2496,7 @@ function _comment_update_node_statistics($nid) {
    - * Generate sorting code.
    + * Generates sorting code.
    
    @@ -2474,7 +2516,7 @@ function comment_int_to_alphadecimal($i = 0) {
    - * Decode sorting code back to an integer.
    + * Decodes sorting code back to an integer.
    

    These should probably be "a sorting code." (I apologize because I think I wrote these docblocks...)

  16. +++ b/core/modules/comment/comment.pages.incundefined
    @@ -18,10 +19,9 @@
      * @param $pid
    ...
    + *   Some comments are replies to other comments. In those cases, $pid is the
    + *   parent comment's comment ID.
    

    This parameter is optional, correct? Should it begin "(optional)" and end with "Defaults to NULL"?

  17. +++ b/core/modules/comment/comment.testundefined
    @@ -204,16 +212,17 @@ class CommentHelperCase extends DrupalWebTestCase {
    -   * Check for contact info.
    +   * Checks for contact info.
    

    Checks what for contact info? (Again, not sure whether we need to fix this in this patch or not.)

  18. +++ b/core/modules/comment/comment.testundefined
    @@ -1505,7 +1519,7 @@ class CommentApprovalTest extends CommentHelperCase {
    -   * Test comment approval functionality through node interface.
    +   * Tests comment approval functionality through node interface.
    

    Through the node interface?

  19. +++ b/core/modules/comment/comment.testundefined
    @@ -1876,7 +1889,7 @@ class CommentActionsTestCase extends CommentHelperCase {
    -   * Helper function: clear the watchdog.
    +   * Clears the watchdog.
    

    I think it would just be "clears watchdog"? ("Clears the watchdog" makes me think we're referring to an actual dog. The furry kind.)

Note that there are two issues open that involve significant changes to the comment Comment module, so some patches may need to be rerolled if others go in first:

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
42.67 KB

Fixing all of these, except:

13) I chose to not add the information back in ... what the submission handler does is pretty much the usual thing, so I don't think there is any reason to note this especially.

jhodgdon’s picture

Status: Needs review » Needs work

Much better! There are still a couple of minor things that I probably missed last time (sorry!)... If you make a new patch, posting an "interdiff" would be helpful to aid in reviewing, if you have the tools to make one. Ask in IRC for help if not. :)

a)
This needs an extra space of indentation:

  * @param $node
- *  A node object.
+ *  The node object for which to build the comment-related elements.

b)

 /**
  * Generate a comment preview.
+ *
+ * @see comment_form_build_preview()
  */
 function comment_preview($comment) {

Wrong verb tense.

c) List formatting:

    * @param integer $level
-   *   Anonymous level.
+   *   The level of the contact information allowed for anonymous comments
+   *   - 0: No contact information allowed.
+   *   - 1: Contact information allowed but not required.

The line before the list should end in :

d) Needs an apostrophe (commenter's):

  /**
-   * Check for contact info.
+   * Checks whether the commenters contact information is displayed.

e)

   /**
-   * Set comment setting for article content type.
+   * Sets a comment setting for article content type.

Huh? Sets a comment setting -- what does that mean? And maybe needs "a" or "the" before article content type?

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
42.72 KB

Here is another patch with an interdiff ... I actually tried to do one for the last patch, as well, but that proved difficult, as I had pulled in upstream changes in the meantime that created conflicts.

Re: e) This is what the function does---it set a setting for use in a test case. I changed this to "Sets a comment setting variable ...", but I am not sure this is much better ...

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good -- thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs work

Sorry folks, no longer applies so will need a re-roll.

sven.lauer’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
42.36 KB

Re-roll attached. Setting back to RTBC because all I did was fixing the merge conflicts.

Status: Reviewed & tested by the community » Needs work
Issue tags: -docs-cleanup-2011

The last submitted patch, doc-cleanup-comment-module-1332598-9.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +docs-cleanup-2011
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

backport time!

oriol_e9g’s picture

Assigned: sven.lauer » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
41.07 KB

Docs improvements backported except the sorting functions because we still use vancodes in Drupal 7.

xjm’s picture

Status: Needs review » Needs work

Thanks @oriol_e9g! Few things:

+++ b/modules/comment/comment.admin.incundefined
@@ -2,11 +2,20 @@
+ * Page callback: Presents an administrative comment listing.
+ *
+ * Path: admin/content/comment
+ *
+ * @param $type
+ *   The type of the overview form ('approval' or 'new'). See
+ *   comment_admin_overview() for details.
+ *
+ * @see comment_menu()
+ * @see comment_multiple_delete_confirm()
  */

The menu callback changes aren't getting backported to D7, so we should remove or reduce this and similar hunks to fixes for the standards that existed previously.

+++ b/modules/comment/comment.api.phpundefined
@@ -105,24 +106,20 @@ function hook_comment_view_alter(&$build) {
+ * @param $comment ¶

Trailing whitespace here. Hopefully that wasn't in the original patch. :P

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
40.95 KB

@xjm Wow! You are in all issues queue!!!

jhodgdon’s picture

Status: Needs review » Needs work

I don't think we're applying the new menu callback standards to Drupal 7 patches?

xjm’s picture

Right, like I said in #23. :) The Path: blah blah and @see foo_menu() need to be removed from #24.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
40.61 KB

Which is the coding standard for @see in Drupal 7? :/

xjm’s picture

Status: Needs review » Needs work
+++ b/modules/comment/comment.admin.incundefined
@@ -2,11 +2,18 @@
+ *
+ * @see comment_menu()

Remove this line too. It's part of the menu callback standard added in #1315992: No standard for documenting menu callbacks, and so it is not backportable to Drupal 7. Thanks!

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
22.69 KB
40.92 KB

Updated patch with interdiff from #27.

jhodgdon’s picture

Status: Needs review » Needs work

Sorry for the delay! Finally getting around to reviewing some of these issues... There are a couple of problems I can see in this patch:

a) comment.admin.inc near the top:

/**
- * Menu callback; present an administrative comment listing.
+ * Menu callback: present an administrative comment listing.

present -> presents

b) hook_comment_view_alter() doc in comment.api.php:

+ * If the module wishes to act on the rendered HTML of the comment rather than
+ * the structured content array, it may use this hook to add a #post_render
+ * callback. Alternatively, it could also implement hook_preprocess_comment().
+ * See drupal_render() and theme() documentation respectively for details.

There is no hook_preprocess_comment(). This should say "implement hook_preprocess_HOOK() for comment.tpl.php".

c)

+++ b/modules/comment/comment.test
@@ -1,8 +1,8 @@
 <?php
 
 /**
- * @file 
- * Tests for comment.module.
+ * @file
+ * Tests for the Comment module.
  */
 
 class CommentHelperCase extends DrupalWebTestCase {

The class here needs a doc block.

d) also in comment.test

@@ -94,9 +94,13 @@ class CommentHelperCase extends DrupalWebTestCase {
   /**
    * Checks current page for specified comment.
    *
-   * @param object $comment Comment object.
-   * @param boolean $reply The comment is a reply to another comment.
-   * @return boolean Comment found.
+   * @param object $comment
+   *   The comment object.
+   * @param Boolean $reply
+   *   Boolean indicating whether the comment is a reply to another comment.
+   *
+   * @return Boolean
+   *   Boolean indicating whether the comment was found.
    */
   function commentExists($comment, $reply = FALSE) {

The proper type should be "bool" not "Boolean" or "boolean" in @param/@return. See http://drupal.org/node/1354#param-return-data-type -- and this applies elsewhere in this file too.

e)

-   * @param boolean $aproval
+   * @param Boolean $aproval
    *   Operation is found on approval page.
    */
   function performCommentOperation($comment, $operation, $approval = FALSE) {

$approval is misspelled in the @param.

f)

@@ -2040,7 +2055,7 @@ class CommentFieldsTest extends CommentHelperCase {
   }
 
   /**
-   * Test that comment module works when enabled after a content module.
+   * Tests that comment module works when enabled after a content module.
    */

Refer to the module as "the Comment module". Several other spots in comment.test too...

Lars Toomre’s picture

There is a code problem in comment.module where 'use Drupal\comment\Comment;' occurs after the declaration of various constants.

Lars Toomre’s picture

Here is a work in progress patch after doing a full review of the files in the Comment module. This patch still includes numerous ??? indicators for missing text substitutions.

Albert Volkman’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
20.79 KB

Re-roll of #32. Filled out ???'s, removed extraneous changes. I'm sure this will still need some work. Setting back to 8.x.

Status: Needs review » Needs work

The last submitted patch, comment_docs-1332598-33.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
20.2 KB

Some code from another patch sneaked in. Also fixed a whitespace error.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch, and sorry for the review delay...

So, the changes in this patch for comment.api.php are all wrong (they incorrectly changed the verb tense for hook definition documentation). Just revert those.

The other changes mostly look good. A few minor corrections:

a) Punctuation of this change in comment.module -- : vs. ;

 /**
- * Entity URI callback.
+ * URI Callback; Returns the URI elements of a comment.

b) later in comment.module -- this @throws should be at the end of the documentation, not right after the @param:

  * @param $cid
  *   A comment identifier.
+ * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException

c) Also in comment.module:

 * @param $langcode
- *   A string indicating the language field values are to be shown in. If no
- *   language is provided the current content language is used.
+ *   (optional) A string indicating the language field values are to be shown
+ *   in. If no language is provided, the current content language is used.
+ *   Defaults to NULL.

That "Defaults to NULL" is confusing and unnecessary, given that the previous sentence explains exactly what happens if no language is provided.

d) Actually, take out all those "Defaults to ..." statements that just say the value that is already in the function signature. Our standards do not recommend doing that:
http://drupal.org/node/1354#param

e) We also need to be careful about things like this (from comment.module again):

  * @param array $context
- *   Array with components:
+ *   (optional) Array with components:
  *   - 'cid': Comment ID. Required if $comment is not given.

This "Required if $comment is not given" goes with $context presumably, not with 'cid' (which shouldn't be in quotes anyway). So it shouldn't be part of the list item.

f) in CommentTestBase.php

+   * @param bool $aproval
+   *   (optional) A Boolean indeicating whether the operation is found on
+   *   approval page. Defaults to FALSE.
    */
   function performCommentOperation($comment, $operation, $approval = FALSE) {

$approval is misspelled in the docs.

g) And a bit lower in CommentTestBase.php:

-   *   Comment id.
+   *   The Comment ID.
    */
   function getUnapprovedComment($subject) {

Comment should not be capitalized here.

Albert Volkman’s picture

FileSize
10.19 KB
16.35 KB

No reason to apologize at all. You're a busy gal :) I appreciate your hard work.

Albert Volkman’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 37: comment_docs-1332598-37.patch, failed testing.

no_angel’s picture

Assigned: Unassigned » no_angel
Issue summary: View changes
no_angel’s picture

Issue tags: +Needs reroll

Manually test comment_docs-1332598-37.patch - failed.
Needs reroll.

jhodgdon’s picture

Status: Needs work » Closed (won't fix)

This patch is 3 years old. I think at this point we can close the issue as "won't fix" and start over with anything left. Besides which, for coding standards fixes we are generally not trying to fix them by module, but by type of fix.