Add type-hinting and parameter type docmentation for comment objects

Now that the philosophical debate has been decided as you point out, I think this is a Novice issue:
To-do list:
1. Go into core/modules/comment
2. Check all of the containing files for functions which receive $comment as a parameter
3. Add the type-hint to the parameter, i.e.:

// Before
function comment_confirm_delete($form, &$form_state, $comment) {

// After
function comment_confirm_delete($form, &$form_state, Comment $comment) {

I tried to do a patch for this without forgot any $comment ;)

I try new patch ;)

Here's another shot at this patch

Better when its not empty ;)

needs review

+++ b/core/modules/comment/comment.test
@@ -104,7 +104,7 @@ class CommentHelperCase extends DrupalWebTestCase {
+  function commentExists(Comment $comment, $reply = FALSE) {
     if ($comment && is_object($comment)) {

This should be:

function commentExists(Comment $comment = NULL, $reply = FALSE) {
  if ($comment) {
...

I.e. the argument should be optional and the is_object() can be removed.

Otherwise looks good. :)

EDIT: Added the type-hint in my example code. *slapsforehead*

Made the above changes and rerolled the patch, thanks for the help and review @tstoeckler

Shouldn't type hinting also be done for the variables in the @param and @return directives in the docblocks? That way the documentation will indicate the expected type of $comment variable.

Added to the docblocks

+++ b/core/modules/comment/comment.api.php
@@ -49,10 +49,10 @@ function hook_comment_update($comment) {
- * @param $comments
+ * @param Comment $comments
  *  An array of comment objects indexed by cid.
  */
-function hook_comment_load($comments) {
+function hook_comment_load(Comment $comments) {

This does not make sense. We have an array for $comments here.

+++ b/core/modules/comment/comment.module
@@ -1467,10 +1467,10 @@ function comment_access($op, $comment) {
- * @param $comment
+ * @param Comment $comment
  *   A comment object.

should be "A comment entity.". Please check all occurences of "comment object".

There is no need to write "A comment object" nor a "A comment entity" here since the type hinting is self documenting. Just put the "@param Comment $comment" with no additional information is enough.

Re #16. That is incorrect. We have a standard that each parameter needs a description. If you disagree with that standard please open a new issue.

The docblocks in comment.module seem to vary from documentation standards. For instance, comment_submit() is missing a @param directive and description. That needs to be added here. Additionally, when adding type hinting to one variable in a docblock, please do so for all of the @param variables.

Rerolled the patch with #19 @fago

needs review

+++ b/core/modules/comment/comment.api.phpundefined
@@ -49,10 +49,10 @@ function hook_comment_update($comment) {
+ * @param Comment $comments
  *  An array of comment objects indexed by cid.

This does not make sense. It should be @param array $comments.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1889,7 +1889,7 @@ function comment_form_build_preview($form, &$form_state) {
  *
  * @see comment_form_build_preview()
  */
-function comment_preview($comment) {

No @param directive here?? Appears @param Comment $comment is missing.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1987,7 +1987,7 @@ function comment_form_validate($form, &$form_state) {
 /**
  * Prepare a comment for submission.
  */
-function comment_submit($comment) {

Ditto.

+++ b/core/modules/comment/comment.moduleundefined
@@ -2435,7 +2435,7 @@ function comment_unpublish_by_keyword_action_submit($form, $form_state) {
  *
  * @ingroup actions
  */
-function comment_save_action($comment) {

Ditto.

Changes from #22

Thanks for working on this!

+++ b/core/modules/comment/comment.api.php
@@ -49,10 +49,10 @@ function hook_comment_update($comment) {
- * @param $comments
+ * @param Comment $comments
  *  An array of comment objects indexed by cid.
  */
-function hook_comment_load($comments) {
+function hook_comment_load(Comment $comments) {

this is still wrong.

Whoops, attached wrong patch. Corrected

+++ b/core/modules/comment/comment.api.phpundefined
@@ -49,10 +49,10 @@ function hook_comment_update($comment) {
- * @param $comments
+ * @param array $comments
  *  An array of comment objects indexed by cid.
  */
-function hook_comment_load($comments) {
+function hook_comment_load(Comment $comments) {
   $result = db_query('SELECT cid, foo FROM {mytable} WHERE cid IN (:cids)', array(':cids' => array_keys($comments)));

This is impossible. First it's an array than a Comment object.
This should brake the testbot somehow... So we aren't testing this?

+++ b/core/modules/comment/comment.moduleundefined
@@ -1887,9 +1887,11 @@ function comment_form_build_preview($form, &$form_state) {
+ * @param Comment $comment
+ * ¶
  * @see comment_form_build_preview()

Needs a line of explanation and has a trailing whitespace

+++ b/core/modules/comment/comment.moduleundefined
@@ -2432,10 +2437,12 @@ function comment_unpublish_by_keyword_action_submit($form, $form_state) {
+ * ¶
+ * @param Comment $comment
  *

Same

Why would it need an explaination, would you write "A comment object." ? Is "Comment $comment" not self-descripting enough?

That said, you're right about first points, it's an array not a Comment instance.

Because it's our standard...

This particular piece of standard is about documentation, and this particular piece of documentation isn't needed anymore. I think in this case the standard doesn't fit the need anymore and must be revisited.

Regarding #29: Each @param does need a description. Reference: http://drupal.org/node/1354#param-return-data-type

I'd agree that this:

@param Comment $comment
  A Comment object.

is redundant and not so useful. However, instead of removing the parameter description, we should make it more useful and provide detail about the parameter. And, having a human-readable line explaining each parameter makes it easier to understand in the API docs.

That said, it's off-topic for this issue. We should follow the current standard as long as it is the standard. I suggested to pounard in IRC to open a separate coding standards issue to consider whether the standard should be changed if he thinks it should be.

Yes sorry for that, opened #1512338: Revisit Coding Standard about parameters documentation for discussion.

I removed the extra whitespace from #26, I'd be happy to re-roll with additional changes not sure what needs to happen though.

See #26, Comment should be array

Re-rolled the patch with the things mentioned in #26 fixed.

By the way, Netbeans 7.2 has support for @param Comment[] $comments, which is way nicer than @param array $comments but that's something for another issue and day to look at ;)

@aspilicious: This can not be covered by tests as the hook example implementations are dead code, they are never executed and just serve as an example.

Darn it, found another one:

+++ b/core/modules/comment/comment.api.php
@@ -49,10 +49,10 @@ function hook_comment_update($comment) {
+function hook_comment_load(Comment $comments) {

That should also be "array" instead of "Comment". (Or nothing, I've seen us type-hinting arrays elsewhere but I'm not sure it is a standard. Can't hurt though, I guess.)

Well, it would help if I would not re-upload the old patch but my own.

Head -> Keyboard.

This patch adds additional type hinting to:
comment.module

• comment_uri
• comment_view
• comment_build_content
• comment_links
• comment_access
• comment_save
• comment_publish_action
• comment_unpublish_action
• comment_unpublish_by_keyword_action

Also, there seems to be some function in comment.test that define a Comment object as a generic object. Is that right?

Looks like there are some cases in comment.module where we're not loading comments as entities, for example:

 $comment = db_query('SELECT c.*, u.uid, u.name AS registered_name, u.signature, u.signature_format, u.picture, u.data FROM {comment} c INNER JOIN {users} u ON c.uid = u.uid WHERE c.cid = :cid AND c.status = :status', array(
          ':cid' => $pid,
          ':status' => COMMENT_PUBLISHED,
        ))->fetchObject();
        if ($comment) 

That should be changed I guess but probably not in this issue.

Edit: Given that we have entity caching, I guess it's actually better and much simpler to do a simple comment_load($pid) and *then* check the status in the condition.

fago isn't there a way to NOT write two lines for a single load? Do we have to go through comment_load_multiple?

As I said, I'd suggest this:

$comment = comment_load($pid);
if ($comment->status == COMMENT_PUBLISHED) {

}

Using $conditions is deprecated and removes the possibility of getting the entity from cache and the status check is only a sanity check to not unpublish unpublished comments, so...

I'll try to finish this one off now.

Well, that wasn't too hard. The code mentioned above in comment.pages.inc was responsible for most of those failures.
All previously failing tests pass locally now.

+++ b/core/modules/comment/comment.pages.incundefined
@@ -47,12 +47,8 @@ function comment_reply($node, $pid = NULL) {
+        $comment = comment_load($pid);
+        if ($comment->status = COMMENT_PUBLISHED) {

Perhaps an inline comment here, e.g. "Load the parent comment."?

Edit: I take back what I said about scope; I previously failed to understand that this change is needed for tests to pass.

That is necessary to make the type hinting for comment_unpublish_action() work. Adding the type hint showed that we are in fact not passing an actual comment entity to that function.

That said, I initially suggested to do that in a follow-up issue as well. Just trying to explain why it was done.

Added a comment per #47.

Alright, assuming #49 passes the bot, I think this is ready. Thanks @Berdir and @tstoeckler.

Thanks! Committed/pushed to 8.x.

Pretty sure this is a bug:

+        if ($comment->status = COMMENT_PUBLISHED) {

Patch attached.

D'oh.

I guess that means we don't have tests for this :)

Oops, dammit, that was me. Sorry.
Will write a test for this tomorrow, if no one beats me to it.

#52: drupal_1480866_52_comment_type_hint_follow_up.patch queued for re-testing.

Still need the test coverage here, no?

Yeah, I had totally forgotten about that, sorry.
Here we go.

Looks good to me...

+++ b/core/modules/comment/comment.testundefined
@@ -382,6 +382,12 @@ class CommentInterfaceTest extends CommentHelperCase {
+    $this->assertText('The comment you are replying to does not exist.', t('Replying to an unpublished comment'));

The use of t() here is inverted. The first string is translated in comment.module, so we need to use t() when we check for it. And the t() on the assertion message is not needed, though with assertText() I'd recommend just omitting that parameter. (The default message is useful.)

Reference: http://drupal.org/simpletest-tutorial-drupal7#t

This is a minor cleanup, so another good novice task. When you create a new patch, be sure to upload both a test-only version and the complete patch, as in #57.

I adjusted the message so it is inline with the surrounding messages. I would suggest doing the clean-up mentioned in #59 in a separate issue. If we were to change this patch the resulting code would look pretty awkward, IMO.

I went through that before as well, changed lines need to have a proper coding standard, doesn't matter if surrounding lines are wrong.

Also, the first problem (no t() on the text we are testing for) is a straight-up bug. I would have just rerolled it myself otherwise.

no t() on the text we are testing for) is a straight-up bug

How is that? We know the language of the site we are testing. So we might as well test for the exact string as it appears on the site.

Here are updated patches from #57 with the changes from #59.

Actually this is rtbc (first in #64)

Thanks. Committed/pushed to 8.x.

Followup : #1870790: Needless loading in comment_reply()