Triggers/actions need overhaul (part II) - Presave

FYI: This is the continuation of #525540: trigger.module and includes/actions.inc need overhaul

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 ;).

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.

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.

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

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: 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.

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.

Anyone?