Node and comment actions are (still) completely broken and have broken tests too

Seems like a worthy bug fix based on the description. Still a problem?

triggers now have 'changes property'

Is this still reproduced?

Confirm this bug! Its caused by that node already saved when property going to change.

Patch should be re-roller, I think we need separate functions that return function name and AID

PS: Referenced issue #585868: Triggers/actions need overhaul (part II) - Presave

This actions (for node and comment) works only in _presave hook because actual update was removed from them!

So I think it's better to limit them to presave hooks

Marked #1008198: Many actions are completely broken as duplicate.

.

...coming from #1008198: Many actions are completely broken

#1094236: comment_publish_action + comment_unpublish_action still broken was another duplicate.

...and the issue title had a 'duplicate' space ;)

#5: 244093-unpublish-presave-d7.patch queued for re-testing.

Just in case this still applies.

Limiting these to presave hooks looks great.

The Drupal 7 patch is going to need an upgrade path for trigger module - presumably triggers could be left in an invalid state.

It is also probably an API change for contrib modules like rules if they use these actions, but right now they don't work anyway.

I don't see a way to add tests for this since we're removing functionality that didn't work (and never did). However the D7 upgrade path could have tests added for that so leaving the tag.

Once this is committed to 8.x, it is going to be CNW for 7.x to add the updates, I don't think we need simultaneous patches/commits here but if we do please slap it back down.

#5: 244093-unpublish-presave-d7.patch queued for re-testing.

I'm not sure why we can't write some tests along the lines of the OP. I'm definitely not comfortable making arbitrary, potentially BC-breaking changes to this stuff without test coverage.

Webchick, you can't write a test for the op because this removes the association between the unpublish comment action and the 'after saving an updated post' trigger altogether. If one doesn't already exist you could write a test for the same action and a different trigger but that is not testing for this bug. And yes it's bc breaking but these options are dead code, have been for 3-4 years and the patch just makes that official.

I somewhat agree with @catch. The actions are altering the passed in $thing properties. When being invoked past save, they have no impact. No matter how hard you try (or test). Or in other words: There's no way to test an action that cannot work in the first place.

Inventing a testing mechanism for configurable actions sounds like a nice challenge for the future though. It's somewhat related to ->generatePermutations() that we introduced in D7, but if you thought that this was a multi-dimensional problem, then testing configurable actions is a multi-multi-dimensional problem. Yuck. ;)

There is more background in #1008198: Many actions are completely broken.

But reading through that, and more importantly #974072: Comment publish / unpublish actions are broken I see that while the patch here is fine, we still have tests for different comment actions that are completely wrong in core and other broken comment actions, and the other issue was marked as duplicate (by me). fwiw that's exactly the kind of test that could be written to prove this bug was fixed, but would be completely meaningless.

So to fix the other bug too, we need to do similar for the other actions, and remove those completely misleading tests. I won't work on that, I regret ever looking at this rats nest.

Clearly no-one uses this otherwise there'd be more activity here, so downgrading this.

Although this issue mostly concerns actions, anyone stumbling over this issue should be aware of #764558: Remove Trigger module from core, which is RTBC at this point.

I'm still interested in a solution for D7. I'm trying to "Unpublish content containing keyword(s)", but the content is published regardless of it containing or not the banned keywords.
So, is there any patch/solution for D7?

See also #1412964: Add additional test coverage for actions.

There's no trigger module in core 8+