Comment publish/unpublish hooks are not invoked correctly [#2294375]

Problem/Motivation

hook_comment_publish() and hook_comment_unpublish() are documented to exist. However, they are not really being invoked correctly. See Comment::postSave():

 if ($this->isPublished()) {
    \Drupal::moduleHandler()->invokeAll('comment_publish', array($this));
  }

So the publish hook is being invoked every time a comment is saved and it is currently published (whether or not it was previously published), and as far as I can tell, the unpublish hook is not being invoked at all.

And (see comment #2/Berdir) we probably do not need these hooks anyway, because you can use the generic update hooks and look at the original comment to see if its published status has changed.

Proposed resolution

Remove these two hooks from the comment.api.php file, and from being invoked in the postSave() method.

Alternative proposal: Invoke them as they are documented to be invoked (if the publishing status is being changed to published/unpublished).

Remaining tasks

a) Write a patch, get it reviewed.
b) Write a short change notice describing how to use other hooks to achieve the same purpose.

User interface changes

None.

API changes

No hook_comment_publish() or hook_comment_unpublish().

Original report by jhodgdon

While I was working on #2216535: Replace Node overview topic and Node API topic with Entity Hooks topic, I noticed several problems with the entity hooks:

a) When the entity CRUD hooks are invoked, the generic ones like hook_entity_create() and hook_entity_load() all have the entity type ID passed in. This was not documented in most of the hook documentation in entity.api.php (I'm fixing that).

However, this is not being tested in entity_crud_hook_test.module where it tests all the entity CRUD hooks. I think that should be fixed?

b) Inconsistency: In all cases but one, the EntityStorageBase::invokeHook() method is used to invoke the entity hooks from the storage controller. This makes the type-specific hook run before the generic hook (hook_ENTITY_TYPE_create() before hook_entity_create() for instance).

However, during load it goes in the opposite order. It seems like this should be reversed? See EntityStorageBase::postLoad().

Comment	File	Size	Author
#17	2294375-comment-hooks-17.patch	6.3 KB	jhodgdon
#17
#10	2294375-comment-hooks-9.patch	5.86 KB	jhodgdon
#10
#8	interdiff.txt	4.12 KB	jhodgdon
#8	2294375-comment-hooks-8.patch	5.84 KB	jhodgdon
#8
#5	2294375-comment-hooks.patch	1.72 KB	jhodgdon
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 28 June 2014 at 16:00

More to address:

c) I do not think that hook_comment_publish() and hook_comment_unpublish() are being invoked correctly. See Comment::postSave()
https://api.drupal.org/api/drupal/core!modules!comment!src!Entity!Commen...

It looks like the publish hook is being invoked on every save if the comment is now published, and the unpublish hook is not being invoked???

Comment #2

Berdir

German

Switzerland

CreditAttribution: Berdir commented 1 July 2014 at 22:40

So we fixed a) in a separate issue by removing that.

b) Yeah, possibly.

c) Not sure why we even have those hooks, it's really not that hard to check it yourself now with $comment->original.

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir commented 1 July 2014 at 22:41

Note that c), unlike a/b is a comment specific problem, should possibly be a separate issue to discuss what to do with them? Agree that they are broken, so maybe just remove them as a "fix"...

Comment #4

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 16:47

Title:	Inconsistencies with entity hooks	» Comment publish/unpublish hooks are not invoked correctly
Component:	entity system	» comment.module
Issue summary:	View changes

Well, it looks like the comment hook is the only real relevant issue left on this one (I'm giving up worrying about the fact that the load() hooks run in backwards order from all other hooks; if you think it's worth pursuing, we can pursue it elsewhere).

So, repurposing this issue.

Comment #5

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 17:00

Status:

Active

» Needs review

File	Size
2294375-comment-hooks.patch	1.72 KB

Here's the easy patch.

For the change notice, I think we could write this:
------
Title:
hook_comment_publish() and hook_comment_unpublish() have been removed

Body:
hook_comment_publish() and hook_comment_unpublish() have been removed in Drupal 8.x.

You can react to an update that changes the published status of a comment by using hook_entity_update(), or better yet hook_ENTITY_TYPE_update(). This will have $entity->original set to the original entity, before the changes.

Example in Drupal 7:

function mymodule_comment_publish($comment) {
  // The comment has just been published. React accordingly.
}

function mymodule_comment_unpublish($comment) {
  // The comment has just been unpublished. React accordingly.
}

Drupal 8:

function mymodule_comment_update($comment) {
  if ($comment->isPublished() && !$comment->original->isPublished()) {
     // The comment has just been published. React accordingly.
  }
  elseif (!$comment->isPublished() && $comment->original->isPublished()) {
     // The comment has just been unpublished. React accordingly.
  }
}

-------------

Thoughts? I can make a draft change notice, but since I am not sure the approach has been agreed to, I didn't do that yet.

Comment #6

2 July 2014 at 17:16

Status:

Needs review

» Needs work

The last submitted patch, 5: 2294375-comment-hooks.patch, failed testing.

Comment #7

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 17:27

It looks like Forum and Tracker modules were relying on the (flawed) hook_comment_publish() invoke. Their hook implementations will need to be updated.

Comment #8

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 17:36

Status:

Needs work

» Needs review

File	Size
2294375-comment-hooks-8.patch	5.84 KB

interdiff.txt	4.12 KB

1 file was hidden/shown/deleted

File	Size
2294375-comment-hooks.patch	1.72 KB

OK. Turns out node.module also implemented these two hooks, but since it also had a hook_comment_update(), it was pointless to do it again in those two hooks. The forum and tracker hook situation is I think a lot cleaner without these two hooks too, since (again) they were doing pretty much the same thing in the update hooks too.

This is seeming like a better and better idea...

Comment #9

2 July 2014 at 17:37

Status:

Needs review

» Needs work

The last submitted patch, 8: 2294375-comment-hooks-8.patch, failed testing.

Comment #10

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 17:39

Status:

Needs work

» Needs review

File	Size
2294375-comment-hooks-9.patch	5.86 KB

2 files were hidden/shown/deleted

File	Size
2294375-comment-hooks-8.patch	5.84 KB

interdiff.txt	4.12 KB

Oh, should fix a few docs things... and the non-working patch... interdiff failed...

Comment #11

2 July 2014 at 17:55

Status:

Needs review

» Needs work

The last submitted patch, 10: 2294375-comment-hooks-9.patch, failed testing.

Comment #12

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 18:42

OK, I give up. I don't get why using the update hook is failing. Maybe the comment storage class is doing something weird to bypass those hooks.

Anyway, the publish/unpublish hooks are not being invoked as they are documented and *something* needs to be done... I am just not sure what now.

Comment #13

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 2 July 2014 at 21:28

See #2289369: hook_comment_unpublish is never invoked

Comment #14

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 21:34

Status:

Needs work

» Closed (duplicate)

Yeah, this issue should be a duplicate of that other one.

Comment #15

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 21:35

Comment #16

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 21:38

Status:

Closed (duplicate)

» Needs work

I changed my mind. This issue is also about the publish hook, and the other one is only about unpublish.

Comment #17

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 21:44

Status:

Needs work

» Needs review

File	Size
2294375-comment-hooks-17.patch	6.3 KB

One more try on a patch. I think forum/tracker may need hook_comment_insert() done too.

Comment #18

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 2 July 2014 at 21:49

+++ b/core/modules/node/node.module
@@ -1673,23 +1673,3 @@ function node_comment_delete($comment) {
-function node_comment_publish($comment) {
...
-function node_comment_unpublish($comment) {

Should these be replaced with comment_{insert,update} hooks too?

Thanks for working on this!

Comment #19

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 21:52

If this works, here's a new proposed change notice:
=============
------
Title:
hook_comment_publish() and hook_comment_unpublish() have been removed

Body:
hook_comment_publish() and hook_comment_unpublish() have been removed in Drupal 8.x.

You can react to an update that changes the published status of a comment by using hook_entity_update(), hook_entity_insert(), or better yet hook_ENTITY_TYPE_update() and hook_ENTITY_TYPE_insert().

The update hooks will have $entity->original set to the original entity, before the changes.

Example in Drupal 7:

function mymodule_comment_publish($comment) {
  // The comment has just been published. React accordingly.
}

function mymodule_comment_unpublish($comment) {
  // The comment has just been unpublished. React accordingly.
}

Drupal 8:

function mymodule_comment_update($comment) {
  if ($comment->isPublished() && !$comment->original->isPublished()) {
     // The comment has just been published for the first time. React accordingly.
  }
  elseif (!$comment->isPublished() && $comment->original->isPublished()) {
     // The comment has just been unpublished for the first time. React accordingly.
  }
}

function mymodule_comment_insert($comment) {
  if ($comment->isPublished()) {
     // A new published comment has just been created. React accordingly.
  }
  else {
    // A new unpublished comment has just been created. React accordingly.
  }
}

===============

Comment #20

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 21:52

RE #18 - no, the node module already has insert/update comment hooks in it. No additional publish/unpublish hooks are necessary.

Comment #21

andypost

he/him

Russian

CreditAttribution: andypost commented 2 July 2014 at 22:01

+1 to rtbc and removal, confirm #20
we could update https://www.drupal.org/node/2112417 change record for that and others anyway

Comment #22

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 July 2014 at 22:15

I do not see this as being really related to that other change record. I also think having the hook names in the change record title is important. So I think we should make a separate change record.

The tests passed on the latest patch, so I will go ahead and make a draft change record.