There's a perfectly good comment_save() function they could use.
Not using it breaks the following (off the top of my head, I'm sure there's more):

node_comment_statistics counts will be wrong.
tracker and forum modules will have inaccurate index tables.
entitycache will have stale entities.

I can't get motivated to write tests for actions, out of any of these, node_comment_statistics would make the most sense, since that is comment module breaking itself :(

CommentFileSizeAuthor
naughty_naughty_comment_module.patch1.89 KBcatch

Comments

damien tournoud’s picture

damien tournoud commented

We already have tests for those actions. They can easily be expanded, but I will be happy if they still pass.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Let's get this in.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

Hm. Are you sure this is right? node_publish_action() and node_unpublish_action() do not do node_load() and node_save().

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Looks like the entire thing is broken now I've seen those:

1. comment_actions_info() defines presave, insert and update as hooks. All of these get $comment as an object, but only pre_save can affect the comment before it gets saved to the database, so the else {. So I don't see how the insert or update hooks could ever possibly work, and the tests added in #974072: Comment publish / unpublish actions are broken only test the actual logic in the action, not in context of comment saving, so wouldn't catch this.

2. node_actions_info() defines presave, comment_insert and comment_update. But all the node actions only set $node->status and don't save the node, so this could never actually take effect.

catch’s picture

catch
he/him
Primary language English
commented
Title: Comment actions are naughty » Many actions are completely broken
Component: comment.module » system.module
Assigned: catch » Unassigned
Priority: Normal » Major
Issue tags: +Needs backport to D6

Reading through this mess is sapping my will to live, so I'm re-titling and unassigning myself. As far as I can see this is equally broken in Drupal 6.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Component: system.module » comment.module
Priority: Major » Normal
Issue tags: +Needs tests

Sigh.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Component: comment.module » system.module
Priority: Normal » Major

Oops.

catch’s picture

catch
he/him
Primary language English
commented

#244093: Node and comment actions are (still) completely broken and have broken tests too was duplicate.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented

...yes, but none of the issues is actually marked as such.

Taxoman’s picture

Taxoman commented

Subscribing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Closed (duplicate)

This is the duplicate, issue status fail :(

See you over at #244093: Node and comment actions are (still) completely broken and have broken tests too.