1) The special-cased "changes_node_property" action behavior is required for other entities, too - for example, comments.

2) The current "changes_node_property" behavior is a dependency controller in reality. An action needs to run before another, resp. requires another action to run afterwards.

3) Every action should always be passed a proper object of the target object type. The current conditional munging in comment actions (f.e. http://api.drupal.org/api/function/comment_unpublish_action/7) makes no sense at all. The module that invokes an action should ensure that the first passed argument is the target object the action can process and/or optionally alter by reference. Additional information must be contained in the passed $context, not the other way around.

Also note these issues:
- #585838: Consider a generic $entity->user property (also contained here, because comment unpublish action isn't manually testable otherwise)
- #360023: Update node_comment_statistics during "Publish comment" action

CommentFileSizeAuthor
#17 drupal.trigger-actions.17.patch18.16 KBsun
Passed: 14663 passes, 0 fails, 0 exceptions View
#13 drupal.trigger-actions.13.patch17.06 KBsun
Passed: 14644 passes, 0 fails, 0 exceptions View
#10 drupal.trigger-actions.10.patch16.62 KBsun
Passed: 14662 passes, 0 fails, 0 exceptions View
drupal.trigger-actions.patch17.68 KBsun
Failed: Failed to apply patch. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

sun’s picture

mattyoung’s picture

.

fago’s picture

ad 3.: Agreed!

@comment_unpublish_action
comment_unpublish_action() seems to not clear the cache when dealing with a 'cid' only. Ideally this 'cid' hack should be removed, though I don't know whether it's needed somewhere. At least I'd suggest changing it to do a proper comment_load(), as usually that would be served from static cache anyway.

ad 1,2: In general, I agree. However the code in trigger module doesn't really make sense that way. You made it a generic dependency controller, but it still talks about changing properties. As we don't need those generic dependency controller, I'd suggest to invent a behaviour 'changes_entity_property' or so and lookup the to be run action from somewhere... Maybe just try to use $entity_type . '_save_action' ?
For rules I solved that with a separate data abstraction layer that also deals with saving.

amc’s picture

Issue tags: +actions, +triggers

Just for reference, here is the UX side of this: #491236: D7UX Microproject - Trigger UX

sun’s picture

Thanks for reviewing!

1) and 2):

ok, I can see where you're coming from -- it's now supposed to be generic, but it still contains a hard-coded check for "presave"... right. :-/ I'm pretty sure that we cannot introduce a separate data abstraction layer like you have in Rules though. ;) I agree that we can change that key to something more specific, but I don't really like the "changes_foo_property", because when I looked at that and still had little idea of trigger/actions, I had no idea what this could mean. I'd prefer a more specific term like "needs_saving" or maybe just "add" -- and we absolutely have to define the function name to invoke as value, because there are many developers who already complain about the magic function naming for configurable actions (auto-appended "_form", "_validate", and "_submit" suffixes, without any way to specify one form_id for multiple actions).

I now figure that "add" would have the same generalization problem as "before". It's unfortunate that the check for the "presave" trigger is actually a negated condition, i.e. the specified save action needs to be invoked whenever we are NOT in the 'presave' hook/trigger. Otherwise, we could simply do:

    'comment_unpublish_by_keyword_action' => array(
      'type' => 'comment',
      'label' => t('Unpublish comment containing keyword(s)'),
      'configurable' => TRUE,
      'behavior' => array(
        'comment_presave' => 'comment_save_action',
      ),
      'triggers' => array('comment_presave', 'comment_insert', 'comment_update'),
    ),

Well. Please forgive me, but I'm thinking aloud. :)

Actually - all we have to keep is the current behavior of how the system works. It doesn't matter whether the structure we're introducing would technically allow for more features, because we only can and want to support the current features. I.e.: I'm still not sold that we cannot properly specify dependencies in a way, developers new to actions would understand it.

I don't know whether Rules implements further 'behavior' properties for actions, but as of now, it seems like the 'changes_node_property' behavior is the only one that exists. Additionally, the specified 'behavior' information is only processed once when an action is assigned, so ease of access to that data doesn't really matter. The entire 'behavior' property looks too abstracted to me, especially since all actions can be altered (now).

Furthermore, this current behavior for checking a "presave" condition only works for data handled by APIs that provide a "presave" condition. Hence, many modules not supporting a "presave" stage cannot even implement actions.

    'comment_unpublish_by_keyword_action' => array(
      'type' => 'comment',
      'label' => t('Unpublish comment containing keyword(s)'),
      'configurable' => TRUE,
      'dependencies' => array(
        'comment_save_action' => array(
          'exclude' => array('comment_presave'),
        ),
      ),
      'triggers' => array('comment_presave', 'comment_insert', 'comment_update'),
    ),

At least, something like that I would understand at first sight.

However, if that doesn't work out for you, then I'd also be fine with going back to "changes_entity_property" or "needs_saving" as key, specifying the function to invoke as value. That would mean:

    'comment_unpublish_by_keyword_action' => array(
      'type' => 'comment',
      'label' => t('Unpublish comment containing keyword(s)'),
      'configurable' => TRUE,
      'behavior' => array(
        'changes_entity_property' => 'comment_save_action',
      ),
      'triggers' => array('comment_presave', 'comment_insert', 'comment_update'),
    ),

We should definitely find a more abstracted solution in D8, but for D7, we absolutely have to find a key name that works with the current system.

3):

As of now, the list of actions that contain these wonky object/context munging are only:

- Comment actions
- http://api.drupal.org/api/function/system_message_action/7 (should use $context[$context['type']] = $object;)
- http://api.drupal.org/api/function/system_send_email_action/7 (unsure)
- http://api.drupal.org/api/function/user_block_user_action/7 (while this looks about right)

I absolutely agree that all actions should use proper API functions to alter any data in the system (except for the 'presave' case ;).

fago’s picture

ad 1+2)
Rules doesn't introduce any other behaviours, so I'm not aware of any others too. Also I can't think of cases where this dependency system might be helpful, so I'd suggest going "back" and use "needs_saving". I agree that "needs_saving" is much more intuitive. Adding the function as value sounds to be fine.

We could add an function action_do_save() that handles that saving, which is invoked by the trigger module when trigger thinks it needs to save. That way we don't have to automatically move/add the saving action and tell the user about it. It's just saved automatically.

Triggers could also have a behaviour like 'is_saved' - so we wouldn't hardcode it to 'presave' triggers either.

>We should definitely find a more abstracted solution in D8, but for D7, we absolutely have to find a key name that works with the current system.

For d8 I'd like to work on integration the core of rules - I hope we can avoid those incompatible action/trigger systems in future (there is also conditional actions of ubercart..). In rules each action/condition/event may deal with multiple parameters. Thus those "behaviours" are per-parameter.

ad 3) object/context munging:
Well as you said actions shouldn't have to deal how to get the parameters, the system has to deal with that beforehand. Triggers provides variables, actions may use them. What the system here misses is the abstraction of *how to get those* variables, which should be specified when implementing a trigger - because only the triggering module knows. Fiddling around afterwards can work in some cases, but generally it won't.
However probably it's too late to solve that properly for d7, as that way triggers would have to provide already multiple variables - as they are working with actions of different types. Then you have to deal with lazy-loading and populate something like $context once needed by token - probably we'd end up with a variable system similar to that of rules.. ;)

sun’s picture

Issue tags: +D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Status: Needs review » Needs work

The last submitted patch failed testing.

andypost’s picture

wait til #601398: Simpletest does not allow assigning actions to triggers
Suppose all tests should be re-viewed because of ^

sun’s picture

Status: Needs work » Needs review
FileSize
16.62 KB
Passed: 14662 passes, 0 fails, 0 exceptions View

Re-rolled. This patch is horribly critical to make any sense of actions in D7. So let's try to get at least the contained "presave" handling in.

andypost’s picture

Title: Triggers/actions need overhaul (part II) » Triggers/actions need overhaul (part II) - Presave
Status: Needs review » Reviewed & tested by the community

I've changed a title little to indicate that API change.

Presave should be handled anyway! tested the unpublish_by_keyword - it works.

I'd like 'before' behavior because it more intuitive.

After commiting this there's should be follow-up patches to add tests.
Because right now we cant write them (my comment #9)

andypost’s picture

So little about 'before'

Mostly all RDBMS have a trigger staff and if we talk about trggers most of DB-wise developers understand what it means "before' and 'after".

For example http://dev.mysql.com/doc/refman/5.0/en/create-trigger.html

http://download-uk.oracle.com/docs/cd/B19306_01/appdev.102/b14251/adfns_...

PS: not need to reinvent the wheel

sun’s picture

FileSize
17.06 KB
Passed: 14644 passes, 0 fails, 0 exceptions View

Clarified that PHPDoc about "before" a bit more, especially to clarify the meaning of "before".

webchick’s picture

Status: Reviewed & tested by the community » Needs review

I can see why this patch is needed. However, I really don't like this mechanism of this weird 'before' index to do this, as it's totally non-standard with anything we do elsewhere in core. Everywhere else, we use weights to dictate "thing X needs to come come before/after thing Y."

Why can't we just weight comment_save_action and node_save_action at 10 or something, so they always run last?

sun’s picture

There can be other actions assigned, which may shall run after the actions that are changing entity properties and the action that saves the entity.

The purpose of this patch is to remove the hard-coded support for node and comment entities only. It's only a small step to a more streamlined system, because there was not much time between the original trigger/actions overhaul patch and API freeze.

For D8, yes, we should definitely revamp the entire system, and perhaps introduce a better weighing, as you suggest.

webchick’s picture

Hmmm. I'm having a hard time with this patch. We're introducing 'before' and not also 'after', and we do not re-use concepts that people are familiar with elsewhere in Drupal (weights, dependencies...). We have more than enough WTF in this subsystem, and I really hesitate to add more. I'm leaning heavily towards figuring this out properly in D8.

Can someone explain to me why this is "critical"? It seems to me that despite limitations in the actions/trigger system, lots of D6 modules are making good use of it, Views Bulk Operations being the most prominent example.

sun’s picture

FileSize
18.16 KB
Passed: 14663 passes, 0 fails, 0 exceptions View

@webchick: ok, as discussed in IRC - can we agree on on this compromise?

Does not introduce the "before" thingy, renames the "changes_node_property" to "changes_property", and automatically figures out the corresponding save action for such an action.

sun’s picture

Status: Needs review » Reviewed & tested by the community

This revised and stripped down patch is the minimally required change and compromise I can think of. This issue is critical for two reasons:

1) You cannot change multiple properties in any entity other than a node without horribly breaking performance of your site.

2) You cannot use Trigger module or actions in a sane way other than for nodes or comments.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Let's get a review of this from someone.

sun’s picture

Anyone?

jvandyk’s picture

I read through the issue and the patch. It looks good to me. We need the generalization of the changes_property flag since now more than just node has a presave, and otherwise people will be confused as to why their property changes are not persisting.

webchick’s picture

Status: Needs review » Needs work
Issue tags: -D7 API clean-up +Needs Documentation

That sounds like an RTBC to me. ;)

Committed to HEAD!

Please document this change in the 6.x => 7.x upgrade guide.

catch’s picture

Priority: Critical » Normal
andypost’s picture

andypost’s picture

#24 means that *_unpublish_by_keyword_action does not work for hooks other then _presave

Also I filed #874624: Optimization of *_unpublish_by_keyword_action

jhodgdon’s picture

New tagging scheme for update issues

jhodgdon’s picture

Status: Needs work » Fixed

Trying to understand this... It looks like this issue changed the 'behaviors' section of hook_action_info(), and made it actually do something regarding presave/save. I have attempted to document this in
http://drupal.org/update/modules/6/7#trigger_overhaul
so I think this is now fixed. Reviews welcome (if you find a problem, please set issue back to needs work, or edit the page).

Status: Fixed » Closed (fixed)
Issue tags: -actions, -triggers, -API clean-up, -Needs Update Documentation

Automatically closed -- issue fixed for 2 weeks with no activity.