hook_comment_publish() docs are completely wrong

Is the issue really that the documentation needs to change, that the hook should be dropped, or that it should be changed to be something useful?

If I understand your comment correctly, the real issue is that we need a hook that only targets a CHANGE in status, that doesn't fire every time a node is saved with a specified status. For the sake of clarity, I might suggest that we implement a new hook, for example hook_comment_status_change.

Please clarify what action you want taken, and I would be happy to put some sprint equity against this issue.

Here's a patch to the documentation, which admittedly does not accurately describe the actual behaviour for hook_comment_publish() or hook_comment_unpublish(). Perhaps if there are questions about whether or not the hooks are actually useful or should change those could be posted as new issues.

Forgot to update the status.

Nice job on the patch, mandclu. The code and formatting looks good. However I think the entire issue may need to be looked at. See below.

@@ -149,7 +149,7 @@ function hook_comment_publish(Drupal\comment\Comment $comment) {
 }
 
 /**
- * Respond to a comment being unpublished by a moderator.
+ * Act on a comment that is being saved in an unpublished state.
  *
  * @param Drupal\comment\Comment $comment

I just looked through the current code, and this still exists, but I don't think hook_comment_unpublish() is in core anymore? It may have been removed more recently than when the patch was uploaded. It is definitely not called in the postSave() method.

@@ -139,7 +139,7 @@ function hook_comment_view_alter(&$build, \Drupal\comment\Plugin\Core\Entity\Com
 }
 
 /**
- * Respond to a comment being published by a moderator.
+ * Act on a comment that is being saved in a published state.
  *
  * @param Drupal\comment\Comment $comment

I think this is accurate although I question the name of the hook. Why should it be called hook_comment_publish()?

This seems like a duplicate of #900396: Remove separate comment publishing hooks or fix them, which questions the need for these hooks as well.

If we already have a hook for general comment save via Entity, then maybe it would be better to add in logic so it only fires on state transition.

I've created a documentation patch with a little bit more details and attached.

I checked the core and found both hook_comment_publish() and hook_comment_unpublish() is actually used .

I changed the status ^^

Sorry there was an error with white spaces.

It's a RTBC for me!

This documentation on the @param is not right. @param documentation should explain what the hook parameter is. Saying

+ *   This hook is invoked every time a comment is saved and
+ *   changes the status of the comment to published,
+ *   whether it was previously in a published status or not.

is not explaining what the parameter is, it's explaining what the hook does... or at least attempting to. The way it is worded, it sounds like the *hook* is changing the status of the comment to published, and the wording also says "changes the status to published" then "whether it was previously published or not", which negates the word "changed"..

The whole thing needs some work! Make it clear (don't say the status is changed to published if it isn't changed) and don't put it in the @param docs.

#2554965: hook_comment_unpublish is never invoked

This issue seems to be outdated. The hooks hook_comment_publish()/hook_comment_unpublish() are not longer present here.

Well we probably need to fix it in Drupal 7 and 6 then.

This is still an issue in D7. #2554965: hook_comment_unpublish is never invoked is committed, so would need to fix doc for both hook_comment_publish and hook_comment_unpublish.

Uploading a D7 patch based on #3.

RTBC #17,

Thanks @poker10

might as well jam this in.

https://www.drupal.org/node/3324532 could be updated with this change.

One of the tests is being a bit stubborn but the failures look unrelated, especially as we're only changing comments here.

Thanks everyone!