The trigger module defines hook_hook_info(). I think the name of this hook is confusing. It looks like it should provide info on Drupal hooks in general, from the name, but it is actually just providing information on triggers for actions to the trigger module. I just think we should not have things called 'hooks' in Drupal other than actual Drupal hooks.

So I would propose changing the hook's name to hook_trigger_info(), or something similar.

If we do this, we would also want to update some other documentation in the trigger module that mentions 'hooks', and make sure it only says 'hooks' when it is talking about a Drupal hook or its implementation, rather than for a trigger name. For example, hook_action_info() returns an array with component 'hooks', which should be called 'triggers', I think. The documentation of that component is very confusing to me as it is.

If there is agreement on the principle, I could probably attempt a patch...

Refs:
http://api.drupal.org/api/function/hook_hook_info/7
http://api.drupal.org/api/function/hook_action_info/7

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brianV’s picture

Agreed. hook_hook_info() is a bad and confusing name.

I like hook_trigger_info() *much* better.

jhodgdon’s picture

I was working on a patch for this, and I still think that hook_trigger_info() would be better than hook_hook_info() as a name for the base hook of the trigger module, and that we should avoid using the word "hook" in Drupal documentation and functions, unless it refers to a hook in the normal Drupal sense of the word.

However, there's still some terminology that needs reworking, because a "trigger" is a combination of a "hook" (bad current term) and an "operation". For instance, there is a node_update trigger, which is hook=node, operation=update. So we need a replacement for the word "hook" in that context. It's generally a module, but not necessarily the same as the name of the module, as the module can define these in hook_hook_info (or the renamed hook_trigger_info). It's an operand, but I don't think that's a good name for it.

Thoughts?

webchick’s picture

Not sure about this one. That data is there for more than just Trigger module. Trigger module is just an example in core of how to put action and hook data into practice, but other contributed modules also invoke these to determine what system events they can attach actions to.

Also, the hook_info hook *does* describe information about hooks exposed by a module. http://api.drupal.org/api/function/node_hook_info/6 contains hook_nodeapi and all of its various ops.

What about this hook's name/contents did you find confusing? Could it be addressed with better documentation?

jhodgdon’s picture

That logic was OK in Drupal 6, but it doesn't work any more in Drupal 7.

Only one of the many "hooks" given in Drupal 7 core hook_hook_info implementations is actually a Drupal hook (in the normal Drupal sense of the word "hook") -- that is hook_cron() named in system_hook_info(). All of the rest of the hook_hook_info() return values for core implementations are not keyed off the actual Drupal hook names any more. For instance, your example of node_hook_info(): in Drupal 7, the inner array has key "node", but there is no hook_node() in Drupal 7 -- the actual hook names are node_insert, node_delete, etc., not hook_node( $op = 'insert'). Which is to say, the Drupal hook name is a combination of the trigger/action "hook" name ("node" in this case) and the trigger/action "operation" ("insert", "delete", etc.).

I don't see any way to clean up the documentation of hook_hook_info(), assuming we keep the word "hook" all over the Trigger and Action module documentation, to avoid confusion between the trigger/action meaning of "hook" and the standard Drupal "hook" concept. The example mentioned over and over in http://api.drupal.org/api/function/hook_hook_info/7 is hook_node(), which doesn't exist at all.

Besides which, really in any module that defines triggers/actions, you could use anything for the name of the trigger/action "hook". You can name your trigger/action "hook" whatever you want, and cause it to be fired whenever you want by calling actions_do() in your module in any function of your module. Actually -- and this is bad -- the only way for your module to find out what actions the user has assigned to your trigger/action "hook" is to call _trigger_get_hook_aids(), which is not even an official API function (starting with an underscore as it does).

Anyway, it may not make sense to tie this to a Drupal hook at all -- you just want some actions (that the user chooses) to be fired off when a particular "trigger" happens in your module, and that may not be related to a Drupal hook that your module is defining -- why would it? The Trigger module and the Actions API don't care whether these triggers/actions are related to a Drupal hook, and Trigger is only going to be putting up the UI screen for you, not causing those actions to be fired (you have to do that yourself in your module). The hook_hook_info() hook documentation should mention this.

So I stand by my original premise: Especially in Drupal 7, calling the array keys returned by hook_hook_info() "hooks" is innacurate and confusing, since they need not be actual Drupal hooks (most of the core ones aren't). And calling the hook hook_hook_info() is also confusing, since implementations of this don't begin to cover all Drupal hooks, and they need not even be related to core Drupal hooks.

Additional issues with the Trigger module and Trigger API:
- There is no official Trigger API function for finding out which actions the Trigger module's UI has assigned to your trigger/action "hook" -- the closest thing is function _trigger_get_hook_aids() which is internal-use only.
- Taxonomy action triggering in trigger.module is broken (actions would never fire, as they are in an implementation of hook_taxonomy(), which no longer exists in Drupal 7). I'll make sure this is filed as a separate issue.
- The actual action firing-off for core modules in Trigger isn't really tied directly to what is in the core modules' hook_hook_info() implementations. It is hard-wired into the Trigger module (which has to implement those core modules' hooks in order to fire the actions). So in my opinion, the current hook_hook_info() implementations really should be stored in the Trigger module, not the individual core modules, since the Trigger module is what is actually making the actions fire off.
- hook_hook_info() documentation should make clear that your module needs to call actions_do() in order to cause anything to happen, because the Trigger module isn't going to do it for you.

Thoughts?

jhodgdon’s picture

I filed #538058: Trigger module is not triggering taxonomy correctly on the taxonomy triggering issue.

I think I like the name "event" for a combination of a trigger/action "hook" and an operation, and the name "group" for what is now called a trigger/action "hook".

jhodgdon’s picture

I also filed #538064: No official Trigger API function to discover assigned actions on the issue of not having an API function for discovering assigned actions.

webchick’s picture

Wow, nice retort! :)

I'd love to get some opinions on this from someone like jvandyk or eaton, since they are more familiar with actions/trigger than I am. But what it sounds like to me at a glance is simply that hook_hook_info() was never updated when we renamed the node, taxonomy, comment, etc. hooks. This is not particularly shocking given that I can count the number of people who really understand this stuff on about 4 fingers. :P

So while we might have a problem with the name of the hook as well, what it more sounds like to me atm is you've uncovered an actual oversight/bug (not to mention a gaping hole in our test coverage...)

jhodgdon’s picture

Yes, definitely a gaping hole in testing. I mentioned that in the first comment on #538058: Trigger module is not triggering taxonomy correctly.

jhodgdon’s picture

I solicited opinions from jvandyk and eaton. Haven't heard from eaton, but jvandyk wrote back and said that he didn't have time to look into it, but "...if ops have truly gone away than a revisit to how triggers happen is clearly needed."

jhodgdon’s picture

FileSize
75.73 KB

I have a big patch to fix this and the following other issues:
#538064: No official Trigger API function to discover assigned actions
#539716: actions_get_all_actions - wrong documentation
#538058: Trigger module is not triggering taxonomy correctly

Sorry for having to roll them all together, as they are all intertwined.

This patch makes the following changes, some of which will need to be documented on the module update 6/7 guide, if this patch is committed:
- Hook names, function names, and parameters now make (hopefully) more sense, in that they don't refer to "hooks" unless they are really talking about Drupal hooks (see extensive discussion above in this issue).
- In particular, hook_hook_info() is now hook_trigger_info(), and the 'hooks' component of the hook_action_info() return value is now 'triggers'.
- All of the hook_trigger_info() implementations that were previously in node, system, comment, taxonomy, and user modules have been moved into trigger.module, because trigger.module is the place they are actually triggered.
- In general, the API doc and help text has been cleaned up.
- There's a new API function in Trigger that tells you what the user did in the UI - trigger_get_assigned_actions()
- Descriptions of actions have been updated to UI text guidelines (mostly that they should refer to "content" not "post" as in "publish content" vs. "publish post")
- Paths for trigger pages are now consistently admin/structure/trigger/module -- so the path admin/structure/trigger/cron is now admin/structure/trigger/system
- Fixed taxonomy triggering so it would actually work with current taxonomy hook API
- Added the ability to attach system actions to user logout
- Added tests for previously untested sections of the trigger module: user, taxonomy, and comment

jhodgdon’s picture

Status: Active » Needs review
jhodgdon’s picture

Priority: Normal » Critical

I am setting this to critical because it fixes another critical issue #538058: Trigger module is not triggering taxonomy correctly.

jhodgdon’s picture

Title: Name of hook_hook_info() is confusing » trigger.module needs overhaul

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

Oh, testing bot...

Status: Needs review » Needs work

The last submitted patch failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +ui-text, +API change
FileSize
74.72 KB

Some changes in system.module made the previous patch not apply. Here's a re-roll.

Also tagging this issue...

jhodgdon’s picture

Just another note that if this patch is committed, there will be some documentation needed (module update guide), and also the trigger example module will need a rework (which it could use anyway -- it hardly makes any sense to me).

jhodgdon’s picture

Title: trigger.module needs overhaul » trigger.module and includes/actions.inc need overhaul
Status: Needs review » Needs work

Bojhan just wrote to me and suggested that maybe includes/actions.inc should be made into a module (currently its menu router item and page-generating function are in the system module). I'm going to look into whether that makes sense.

I also am thinking that the actions API should not have hook_action_info() returning trigger information -- the actions API should be independent of the trigger module.

Also, I realized that the actions setup screen is displaying non-translatable text (the Type column is not translatable as it stands). Another problem that should be fixed in this overhaul.

And I should probably look through the issue queue and see if anything else relevant has been reported against actions and trigger.

This may get pushed off to Drupal 8... I'm not sure I can get the whole thing fixed up in the next week, not to mention getting it reviewed and committed, but we'll see. I should have considerable time to work on it next Monday and Tuesday.

Anyway, for the moment, I'm marking this as "needs work", and I don't recommend committing this patch.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
webchick’s picture

Note that I just committed #296322: Tests for abort of actions firing in a loop + trigger module API is completely broken + fix which might have some overlap with this; not sure.

sun.core’s picture

Status: Needs work » Needs review
Dave Reid’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

jhodgdon’s picture

Thanks for the links. Sigh.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Needs work » Active

I took a look at other actions/trigger related issues that have been reported and/or fixed/committed recently...

Given that code freeze is in a week, and that core is a moving target due to these other issues, and the difficulty of getting patches reviewed at all, I don't think I can get a comprehensive overhaul patch written, approved, and committed before then.

Some of the problems discussed above are on other issues, too. Anyway, I think I'll just unassign myself and see what happens with the other issues.

jvandyk’s picture

Since hooks and ops are no longer separate in Drupal 7, it doesn't make sense to artificially support ops in actions/triggers anymore. This simplifies things. Hooks could just be supported. The hook and op columns in the trigger_assignments table get combined. We just need a way to still group things together in a nice UI. jhodgdon suggests 'groups' as a term.

The patch in #17 is quite massive. I wonder if we can break out some of the changes being made.

jhodgdon’s picture

Yeah, I was going for a comprehensive "let's make this module actually make sense" approach in #17's patch, because it didn't make sense to me to just tweak the API a little if the whole approach was outmoded. And I didn't want to patch those 3 issues I was working on in 3 separate patches that wouldn't play well together.

So probably the terminology and hook changes could be pulled out and discussed/patched on #344772: Rename hook_hook_info() to hook_trigger_info(). That would need to go in before code freeze.

The part that fixes the taxonomy issues could be done on #538058: Trigger module is not triggering taxonomy correctly after the code freeze, as it's a bug fix and doesn't involve API changes. I plan to do that, but will wait for after code freeze.

Ditto with the doc fixes -- those can wait until after code freeze, and I will do what I can after code freeze with whatever the state of the API is. Doc patches I've made in the past have had good success at getting reviewed and committed.

But I am not going to try to get in any API changes before code freeze, having had little luck with my other API patches that are just as important (to me anyway) and have been sitting in "needs review" for weeks on end. If someone else wants to have a go at it, the patch is there. If someone wants to commit to working this through review in the next 3 days, I might reconsider, but it's pointless and discouraging to put in hours on making a patch and then have it sit there for weeks with no review.

jvandyk’s picture

Assigned: Unassigned » jvandyk

I am actively working on this to get rid of ops. New opless hook_action_info():

/**
 * Implement hook_action_info().
 */
function comment_action_info() {
  return array(
    'comment_unpublish_action' => array(
      'description' => t('Unpublish comment'),
      'type' => 'comment',
      'configurable' => FALSE,
      'triggers' => array('comment_insert', 'comment_update'),
    ),
    'comment_unpublish_by_keyword_action' => array(
      'description' => t('Unpublish comment containing keyword(s)'),
      'type' => 'comment',
      'configurable' => TRUE,
      'triggers' => array('comment_insert', 'comment_update'),
    )
  );
}

New opless hook_trigger_info():

 function trigger_trigger_info() {
   return array(
     'node' => array(
       'node' => array(
         'node_presave' => array(
           'runs when' => t('When either saving new content or updating existing content'),
         ),
         'node_insert' => array(
           'runs when' => t('After saving new content'),
         ),
         'node_update' => array(
           'runs when' => t('After saving updated content'),
         ),
         'node_delete' => array(
           'runs when' => t('After deleting content')
         ),
         'node_view' => array(
           'runs when' => t('When content is viewed by an authenticated user')
         ),
       ),
     ),
     ...
jhodgdon’s picture

I actually do not think that hook_action_info() should specify the triggers each action supports. I would favor putting this into hook_trigger_info instead. Things in actions.inc should not be trigger-specific.

Or maybe there is a better way to connect actions to triggers? Obviously, some action-trigger pairs do not make sense, but maybe this decision should just be left to the user, or specified in some other way?

jvandyk’s picture

An alternative way of connecting actions and triggers was suggested by cwgordon7 at Drupalcon DC. He suggested (/me squints at notes) that hook_trigger_info() could have a 'provides' key and that hook_action_info() could have a 'requires' key. The values of these keys would match up. For example, hook_comment_insert() would have 'provides' => 'comment' and hook_action_info() for the Unpublish Node action would have 'requires' => 'node'. Of course, you can derive some things from others. E.g., if you have a comment you can get the user and the node with a little extra work; trigger.module does that now.

Realistically, are we able to put that together before the code freeze? I dunno.

The current way prevents people from doing silly things with triggers. Of course, the triggerunlock module lets you shoot yourself in the foot if you want to.

I suppose we use the current 'triggers' key as a suggestion, and have a UI toggle that puts trigger in regular vs. expert mode.

jhodgdon’s picture

I like the requires/provides idea. Comment triggers could also "provide" nodes (e.g. someone adds a comment, and that node gets marked somehow by a custom action someone writes). The generic actions like "send email" and "display a message" might not require anything (though maybe "display a message" could somehow require "interactive" or "logged in", because it wouldn't work too well with cron as a trigger?).

The regular vs. expert mode on the trigger UI is another interesting idea. I think I like the requires/provides logic a bit better though.

jhodgdon’s picture

If you went with the requires/provides idea, there would be some upgrade/migration issues to deal with as well, for people moving from D6 to D7. It would be fairly complex... but would probably be a better approach in the long run.

If you get a patch in today or early tomorrow, I could potentially review it, but I do not have time to attempt writing one before the code freeze (too much paying work to do, never a terrible situation).

I should probably also read through all the above notes and make a single, coherent list (and/or file separate issues).

jvandyk’s picture

Status: Active » Needs review
FileSize
80.25 KB

Here's where I'm at. For some reason simpletest is not running for me in general. Changes in this patch (based on patch above):

- ops are removed from everywhere
- trigger_assignments table loses op column (update function translates old ops to hooks; hook node op insert becomes hook node_insert)
- hook_hook_info() becomes hook_trigger_info(). I am lukewarm on this, as I think the hook describes hooks, not just triggers, but several people have said it is more understandable.
- hook_trigger_info() moved into trigger.module. Moderate memory savings if triggers are turned off.
- _trigger_get_aids() becomes trigger_get_assigned_actions()
- special menu case for cron is gone; we call that group System now

jhodgdon’s picture

Status: Needs review » Needs work

I had problems with simpletest last week -- see #554214: Simpletest has fatal error for all tests when profile.module is enabled - might not be your problem, but maybe? Anyway, it looks like the tests passed in the testbot.

Anyway, here are a few minor comments on this patch, which I generally like:

1) Minor doc typo:

+ *  An associative array containing the results of the functions that
+ *  performs the actions, keyed on action ID.

Should be "functions that perform" not "functions that performs".

2) I'm not sure why the system_goto_action cannot be an 'any' action, as are send email and display message?

3) Inside function trigger_assign(), there are some commented-out code lines -- why? either put them in or take them out...

4) Doc for hook_action_info() says:

'triggers': (required) An associative array of the events that
+ *       can trigger this action. The keys of this array are the trigger
+ *       groups from hook_trigger_info(), and the values are the allowed
+ *       individual trigger operations within that group. You can also declare
+ *       support for any trigger by returning array('any' => TRUE) for this
+ *       value.

I think it should not be mentioning groups any more? I think it's a flat array of trigger functions?

5) Also in doc for hook_action_info():

Currently, the only behavior that is recognized by
+ *       the trigger module is 'changes_node_property': If a trigger is
+ *       assigned to an action with this behavior, any node save actions
+ *       also assigned to this trigger are moved later in the list.

This was an error in my patch above -- besides moving node save actions to later in the list, I believe this behavior also adds a node save action if there wasn't one before?

6) Example code for hook_action_info() doesn't match current expectations, as in node_action_info().

7) I think the trigger_update_7000() table row update could be done in an SQL query directly, rather than a loop, something like this:

UPDATE {trigger_assignments} ta SET ta.hook = CONCAT( ta.hook, '_' , ta.op )

8) "Get the aids of actions to be executed for a hook." as description for function trigger_get_assigned_actions($hook) --
a) Should be "gets".
b) I think we use IDs rather than aids to describe getting ID numbers in function doc.
b) hook_trigger_info() doc doesn't mention "hooks" but "operations", so this function should also use the same terminology. This applies to the $hook parameter, which should probably be $operation or something like that?

9) hook_trigger_info() example code should match trigger_trigger_info() or some subset thereof.

10)

 function trigger_user_view($account) {
   _trigger_user('view', $edit = NULL, $account, NULL);
 }

should be _trigger_user('user_view' ...) I think?

11) Maybe the taxonomy triggers should be taxonomy_term_delete etc. rather than taxonomy_delete, to match the hook names? Just in case someone wanted to do taxonomy_vocabulary_xyz hooks as well?

jvandyk’s picture

Status: Needs work » Needs review
FileSize
80.67 KB

Thanks for the review!

1) Fixed.

2) I'm not sure why the system_goto_action cannot be an 'any' action, as are send email and display message?

Well, you wouldn't want to assign that to cron...

3) Removed commented-out code.

4) Updated to:

 *     - 'triggers': (required) An array of the events (that is, hook) that
 *       can trigger this action. Example: array('node_insert', 'user_update').
 *       You can also declare support for any trigger by returning array('any')
 *       for this value.

5) Updated to:

 *       triggered. Currently, the only behavior that is recognized by
 *       the trigger module is 'changes_node_property': If an action with
 *       this behavior is assigned to a trigger other than node_presave,
 *       any node save actions also assigned to this trigger are moved later
 *       in the list. If a node save action is not present, one will be added.

6) Fixed.

7) I wondered about that too, but was concerned over how cross-database-compatible CONCAT is. Could not get an answer in #drupal and my online forays suggested that CONCAT can get weird, so I elected for the more PHP-based approach. I agree that the direct SQL query is nicer.

8) I think we use IDs rather than aids to describe getting ID numbers in function doc.

b) Not quite sure what you mean here. Changed to "action IDs" to match @return description.

* Gets the action IDs of actions to be executed for a hook.

hook_trigger_info() doc doesn't mention "hooks" but "operations", so this function should also use the same terminology. This applies to the $hook parameter, which should probably be $operation or something like that?

I updated the hook_trigger_info() doc to remove mention of operations, since the point of this patch is to get rid of them.

The structure of this hook is...worthy of some serious thought. It currently has three levels:

Level one: the name of the module providing the trigger (used to build tabs on trigger assignment page in trigger_menu()).
Level two: the group (used to build the list of triggers the assignment page in trigger_assign()).
Level three: the hooks and descriptions.

cwgordon7, in particular, has noted the relative absurdity of this structure.

Does it make sense to eliminate the top level and use the group for both menu building and lists-of-triggers?

10) Fixed.

11) Fixed.

jhodgdon’s picture

Looks good!

Regarding my (7) - PHP loop vs. direct query, I have no idea about CONCAT cross-compatibility, and as the DB table is likely to be small (<100 items) a PHP loop won't be too slow. So, your point is well taken.

Regarding your question about the structure of hook_trigger_info()...
- We were only storing the hook/op combination in the database (now you're storing just hook, which includes op) when assigning actions, so that doesn't point to a need for either module or group.
- As far as I can tell, module/group are only used for grouping on the UI pages. I agree there is no need for both of them to be there.
- Whichever is used, there should be a way to translate the name that is given. So I think using module is a good idea. In the doc, we would want to make it clear that a contrib module could define a trigger for a core module, such as node, and that the module's machine name is what should go in that field. The logic from the old trigger_menu() function can then be used to grab the module's display name (which I believe allows for translation):

   $info = db_select('system')
      ->fields('system', array('info'))
      ->condition('name', $module)
      ->execute()
      ->fetchField();
    $info = unserialize($info);
    $nice_name = $info['name'];
jvandyk’s picture

FileSize
65.77 KB

Whew. hook_trigger_info() has been flattened by one level. New opless, simpler-to-understand, hook:

function trigger_trigger_info() {
   return array(
     'node' => array(
       'node_presave' => array(
         'runs when' => t('When either saving new content or updating existing content'),
       ),
       'node_insert' => array(
         'runs when' => t('After saving new content'),
       ),
       'node_update' => array(
         'runs when' => t('After saving updated content'),
       ),
       ...

Added a test hook_trigger_info() implementation to trigger_test.module that adds a trigger to node tab and a trigger to Trigger Test tab. Revised doc to show new flattened hook_trigger_info().

jhodgdon’s picture

Status: Needs review » Needs work

I think that patch got cut off at the bottom? I don't see any tests in there, and the hook_user() etc. are cut off...

jvandyk’s picture

Status: Needs work » Needs review
FileSize
82.09 KB

Reattempt.

Dave Reid’s picture

Since we're using MySQL in strict mode w/ D7, you can use "'something' || ' anotherthing'" for concatenation inside SQL.

TheRec’s picture

Ok, the following is a code review... a long one and it should be taken positively and also as suggestions for some cases :) I hope it helps.

+++ includes/actions.inc	28 Aug 2009 20:00:45 -0000
@@ -7,35 +7,33 @@
+ * callback functions for the actions are by querying the database. Then
+ * it calls each callback using the function call $function($object, $context,
+ * $a1, $a2), passing this function's input arguments (see below) to the
+ * action function.

, passing this function's input arguments should be , passing the input arguments of this function, I think genitive cannot be used with "function".

+++ includes/actions.inc	28 Aug 2009 20:00:45 -0000
@@ -109,49 +107,18 @@ function actions_do($action_ids, $object
+ * This function contrasts with actions_get_all_actions(); see that
+ * function's documentation for an explanation.

Same goes, The documentation of this function

+++ includes/actions.inc	28 Aug 2009 20:00:45 -0000
@@ -187,21 +159,19 @@ function actions_get_all_actions() {
+ *   An associative array whose keys are md5 hashes of the input array keys, and
...
+ *   'callback', 'description', 'type', and 'configurable' from the input array.
@@ -216,17 +186,19 @@ function actions_actions_map($actions) {
+ * Faster than actions_actions_map() when you only need the function name or ID.

End of line character over the 80 characters limit.

+++ modules/node/node.module	28 Aug 2009 20:00:45 -0000
@@ -2821,123 +2821,70 @@ function theme_node_submitted($node) {
+      'triggers' => array('node_presave', 'comment_insert', 'comment_update', 'comment_delete'),
...
+      'triggers' => array('node_presave', 'comment_insert', 'comment_update', 'comment_delete'),
...
+      'triggers' => array('node_presave', 'comment_insert', 'comment_update', 'comment_delete'),
...
+      'triggers' => array('comment_delete', 'comment_insert', 'comment_update'),

Each element of this array should be put on a new line and indented as it goes past the 80 characters limit. (If you want to be consistent you could also wrap the other 'triggers' even when they do not exceed the limit.)

+++ modules/system/system.module	28 Aug 2009 20:00:45 -0000
@@ -2436,40 +2421,30 @@ function system_action_info() {
+      'triggers' => array(
+        'node_view', 'node_insert', 'node_update', 'node_delete',
+        'comment_view', 'comment_insert', 'comment_update', 'comment_delete',
+        'user_view', 'user_insert', 'user_update', 'user_delete', 'user_login',
+        'user_logout'
+      ),

Just like the previous comment, every items should be on one line according to the coding standards.

+++ modules/trigger/trigger.admin.inc	28 Aug 2009 20:00:45 -0000
@@ -142,40 +142,41 @@ function trigger_assign_form($form_state
+      'link' => l(t('unassign'), "admin/structure/trigger/unassign/$group/$hook/" . md5($aid))

Missing a comma at the end of the line.

+++ modules/trigger/trigger.admin.inc	28 Aug 2009 20:00:45 -0000
@@ -210,54 +210,53 @@ function trigger_assign_form_submit($for
+        drupal_set_message(t('You have added an action that changes a the property of some content. Your Save content action has been moved later in the list so that the property change will be saved.'));

Maybe we should put emphasis on Save content" as it is refering to a particular type of action ?

+++ modules/trigger/trigger.admin.inc	28 Aug 2009 20:00:45 -0000
@@ -210,54 +210,53 @@ function trigger_assign_form_submit($for
+ * Display actions assigned to this hook in a table.
+++ modules/trigger/trigger.api.php	28 Aug 2009 20:00:45 -0000
@@ -12,71 +12,63 @@
+ * Declare information about actions.
@@ -106,48 +98,51 @@ function hook_action_info_alter(&$action
+ * Declare triggers (events) for users to assign actions to.

These should start with a third person verb form according to the coding standards.

+++ modules/trigger/trigger.api.php	28 Aug 2009 20:00:45 -0000
@@ -12,71 +12,63 @@
+ * An action consists of two or three parts: (1) an action definition
+ * (returned by this hook), (2) a function which performs the action
+ * (which by convention is named module + '_' + description of what
+ * the function does + '_action'), and an optional form definition

While reading this I was expecting a (3) so maybe we could change it to : , and (3) and optional form definition

+++ modules/trigger/trigger.install	28 Aug 2009 20:00:45 -0000
@@ -68,4 +61,17 @@ function trigger_schema() {
+ * Hook names now include ops.

Should rather be Adds operators to the hook names and drops the "op" field. I think

+++ modules/trigger/trigger.module	28 Aug 2009 20:00:45 -0000
@@ -8,23 +8,23 @@
+ * Implements hook_help().

This contradicts the coding standards, but currently the de facto "standard" for implementations of hooks is "Implement hook_" (without s). Do not ask me why, but this is this way for the moment, there is an issue to "mass" update this but I am not sure it will ever land ;). There are many occurrences of this I let you find them. See EDIT 2.

This review is powered by Dreditor.

**EDIT** Ouch.. cross-post ... well long delay cross post..sorry it took me some time to build the review... I'll try to see if there is something important in the part I did not review.

**EDIT 2** Seems like documentation about this was changed since the last time I have read it. My bad.

jhodgdon’s picture

Dave Reid: RE #41, Will that work in all the DBs that Drupal 7 now supports? The current approach is definitely safe and as it runs once and shouldn't take long, I would advocate keeping it.

So I gave the patch in #40 a thorough read-through (haven't tried running it or installing the module). There are a couple of remaining problems... (sorry I didn't notice the pre-existing ones before):

a)

function trigger_schema() {
...
    'primary key' => array('hook', 'op', 'aid'),
...

'op' shouldn't be in there any more, and does the update function need to do something to update that index? I am not sure what happens to an index when one of its fields is dropped. Probably OK, but who knows?

b)

function _trigger_user($hook, &$edit, $account, $category = NULL) {
  // Keep objects for reuse so that changes actions make to objects can persist.
  static $objects;
  $aids = trigger_get_assigned_actions('user', $op);
function _trigger_taxonomy($hook, $array) {
  $aids = trigger_get_assigned_actions('taxonomy', $op);
 ...

I think the calls to trigger_get_assigned_actions should pass in $hook instead of $op? It worries me that the tests passed with these errors in there. Maybe the tests are no good?

c) As long as you'll be making changes, a doc issue:

/**
 * Builds the form that allows users to assign actions to triggers.
 *
 * @param $module
 *   Which tab of triggers to display. E.g., 'node' for all
 *   node-related triggers.
 * @return
 *   HTML form.
 */
function trigger_assign($module_to_display = NULL) {

@param doesn't match the actual parameter name, and description doesn't quite match either.

TheRec’s picture

Status: Needs review » Needs work

After a quick review there was only those two other things in the part I did not review :

+++ modules/trigger/trigger.module	28 Aug 2009 20:32:25 -0000
@@ -455,50 +528,66 @@ function _trigger_user($op, &$edit, $acc
+ *   Hook to trigger actions for (taxonomy_delete, taxonomy_insert,
+ *   taxonomy_update).

I do not really get the meaning of this description. Would it be possible to improve it ?

+++ modules/trigger/trigger.module	28 Aug 2009 20:32:25 -0000
@@ -455,50 +528,66 @@ function _trigger_user($op, &$edit, $acc
+    'hook' => $hook

Missing a comma at the end of the line.

This review is powered by Dreditor.

jhodgdon’s picture

Status: Needs work » Needs review

Minor detail on review in #42 --

You are incorrect about the current standard for hook implementations -- the current standard short doc header is:
Implements hook_foo()

See http://drupal.org/node/1354

jhodgdon’s picture

Also, regarding #42, I do not see why we cannot say "this function's arguments" and should have to write "the arguments of this function". Isn't the first one more compact and just as clear?

jhodgdon’s picture

Status: Needs review » Needs work

Forgot to set status after substantive problem found in #43

TheRec’s picture

jhodgdon: Agreed about Implements.. i'll remove it from my previous review (or strike it with correct reference)... wasn't in that state last time I had to create an hook implementation :) My bad.

About using the genitive (possessive case) function's, I was taught that it should not be used for "objects", only for persons (John's pants) or a group of persons (like "men's room") ... but that is only grammar... I think everyone will get that use of genitive but it I was just pointing out what I think is a grammar error. I've seen few occurences of functions's in the core so if you don't correct this I don't think it will prevent it to get commited ;D

jhodgdon’s picture

Really? You mean you have to say "the muffler of the car" and not "the car's muffler"?

Sorry to say I don't own a copy of Strunk and White, but I usually refer to my well-used copy of "Index to English" that I've had since college (i.e. it's old-school)... It doesn't mention anything about 's being forbidden for objects, and has this example on using "x of y" vs "y's x" for genitive:

For example, both "the car's tires" and "the tires of the car" are acceptable.

And later on, it has more examples like:
the wind's force
the bill's defeat

I just love having grammar discussions in the issue queue. :)

jvandyk’s picture

Status: Needs work » Needs review
FileSize
83.08 KB

All issues since #42 addressed. About #43b, yes, we can always use better and more specific tests.

jhodgdon’s picture

Status: Needs review » Needs work

This could be a US vs. British difference, by the way... I'm not sure where you grew up and/or learned English... But Drupal doc standard (for better or for worse) is to use our sometimes-sloppy "standards" of common modern American usage. To me (a native speaker of American English and a programmer geek for decades) saying "the function's return value" sounds just fine. :)

jhodgdon’s picture

Status: Needs work » Needs review

I am happy with the patch in #50 and I think it should be committed.

The status change in #51 was an artifact of me submitting my comment after #50 was submitted.

I am setting back to needs review so the test bot can test it. Assuming tests passed, I think it is RTBC.

Status: Needs review » Needs work

The last submitted patch failed testing.

jhodgdon’s picture

So much for the tests passing...

jhodgdon’s picture

So much for just looking at the code...

I tried to run the tests on a new fresh install of the latest CVS update of D7 (with the above patch applied and no other changes). The Simpletest startup is completely broken. It's getting a database error when it tries to set up the menu router table.

Message: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'title' cannot be null: INSERT INTO {menu_router} (path, load_functions, to_arg_functions, access_callback, access_arguments, page_callback, page_arguments, fit, number_parts, tab_parent, tab_root, title, title_callback, title_arguments, type, block_callback, description, position, weight, file) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8,  .....

(this error message goes on for many many many lines)

That said, the module itself appears to be somewhat but not totally working. I created a system message configured action, and it successfully triggered on:
- create new user
- create content
- update content
- add new comment
-cron

It did not trigger on
- create taxonomy term
- edit taxonomy term
- delete taxonomy term

jhodgdon’s picture

So I have to go now, and I won't be back in my office again before the code freeze (not returning until Sept 8th)...

My review of the code (from looking at it) is that it is a VAST improvement over the previous trigger/actions API.

Obviously (from the tests and from trying it out), it isn't quite working, but if it can be gotten into a state where the existing automated tests will pass, and a simple usage test like what I did in #55 shows that it works OK, then I would mark it RTBC and hope it gets committed before the code freeze. We can then write some more tests at our leisure, and fix any issues that come up (assuming someone fixes the SimpleTest module so that we can actually run tests in our own workspaces!!!!).

Good luck, and sorry I can't help get this pushed through any more!

jhodgdon’s picture

Just out of curiosity, I submitted the patch on #538058: Trigger module is not triggering taxonomy correctly for retesting as well. Possibly someone committed something else in actions/triggers that broke the tests...

jvandyk’s picture

Status: Needs work » Needs review
FileSize
87.29 KB

Fixed simpletest trouble, which was nondefensive coding in trigger_menu(). Fixed tests. And fixed taxonomy actions.

Status: Needs review » Needs work

The last submitted patch failed testing.

jvandyk’s picture

Status: Needs work » Needs review
FileSize
89.28 KB

Updated actions tests in simpletest/tests.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Moving this into the RTBC queue to look at sometime in the next couple days. Great work, folks!

sun’s picture

Wonderful.

First of all, this patch needs a CHANGELOG.txt entry. :)

+++ includes/actions.inc	29 Aug 2009 03:26:25 -0000
@@ -7,35 +7,33 @@
  * @param $object
- *   Parameter that will be passed along to the callback. Typically the
- *   object that the action will act on; a node, user or comment object.
+ *   Parameter that will be passed along to the callback function. Typically the
+ *   object that the action will act on: a node, user, or comment object.

Can we remove the "parameter" stuff, please?

The argument is named $object, and what is being called "typically" here, is the expected standard. API functions should always take and expect a certain data format for arguments. (even when another data type may be supported, which I did not look up yet)

+++ includes/actions.inc	29 Aug 2009 03:26:25 -0000
@@ -7,35 +7,33 @@
  * @param $context
...
+ *   Parameter that will be passed along to the callback function. Typically,
+ *   an associative array containing extra information about what triggered
+ *   the action call, with $context['group'] and $context['hook'] giving the
+ *   group-trigger combination that resulted in this call to actions_do().

Same here. I can only guess that *all* actions expect $context to be an array containing additional objects (?) from the environment that triggered the action.

+++ includes/actions.inc	29 Aug 2009 03:26:25 -0000
@@ -7,35 +7,33 @@
- *

Nitpick: Please leave blank lines between @param and @return. (possibly elsewhere)

+++ includes/actions.inc	29 Aug 2009 03:26:25 -0000
@@ -109,49 +107,18 @@ function actions_do($action_ids, $object
 /**
- * Discover all action functions by invoking hook_action_info().
+ * Discovers all available actions by invoking hook_action_info().
  *
- * @code
- * mymodule_action_info() {
- *   return array(
- *     'mymodule_functiondescription_action' => array(
- *       'type' => 'node',
- *       'description' => t('Save node'),
- *       'configurable' => FALSE,
- *       'hooks' => array(
- *         'node' => array('delete', 'insert', 'update', 'view'),
- *         'comment' => array('delete', 'insert', 'update', 'view'),
- *       )
- *     )
- *   );
- * }
- * @endcode
...
  * @return
- *   An associative array keyed on function name. The value of each key is
- *   an array containing information about the action, such as type of
- *   action and description of the action, e.g.:
- *   @code
- *   $actions['node_publish_action'] = array(
- *     'type' => 'node',
- *     'description' => t('Publish post'),
- *     'configurable' => FALSE,
- *     'hooks' => array(
- *       'node' => array('presave', 'insert', 'update', 'view'),
- *       'comment' => array('delete', 'insert', 'update', 'view'),
- *     ),
- *   );
- *   @endcode

Hm. It would be nice to keep (update) those example array structures.

+++ includes/actions.inc	29 Aug 2009 03:26:25 -0000
@@ -234,20 +206,20 @@ function actions_function_lookup($hash) 
 /**
- * Synchronize actions that are provided by modules.
+ * Synchronizes actions that are provided by modules in hook_action_info().
...
  * @param $delete_orphans
- *   Boolean if TRUE, any actions that exist in the database but are no longer
+ *   If TRUE, any actions that exist in the database but are no longer
  *   found in the code (for example, because the module that provides them has
  *   been disabled) will be deleted.

If that example of *disabled* modules is really true, then we need to create a separate bug report, because no data should ever be removed for *disabled* modules. Disabled modules are just - perhaps temporarily - disabled. Only uninstalled modules are really gone.

+++ includes/menu.inc	29 Aug 2009 03:26:25 -0000
@@ -2784,6 +2784,9 @@ function _menu_router_save($menu, $masks
+    if (!isset($item['title'])) {
+      $foo = 'bar';
+    }

huh? ;)

+++ modules/node/node.module	29 Aug 2009 03:26:25 -0000
@@ -2821,123 +2821,90 @@ function theme_node_submitted($node) {
 /**
- * Implement hook_hook_info().
- */
-function node_hook_info() {
-  return array(
-    'node' => array(
-      'node' => array(
-        'presave' => array(
-          'runs when' => t('When either saving a new post or updating an existing post'),
-        ),
-        'insert' => array(
-          'runs when' => t('After saving a new post'),
-        ),
-        'update' => array(
-          'runs when' => t('After saving an updated post'),
-        ),
-        'delete' => array(
-          'runs when' => t('After deleting a post')
-        ),
-        'view' => array(
-          'runs when' => t('When content is viewed by an authenticated user')
-        ),
-      ),
-    ),
-  );
-}
+++ modules/trigger/trigger.api.php	29 Aug 2009 03:26:26 -0000
@@ -12,77 +12,69 @@
+ *     - 'triggers': (required) An array of the events (that is, hook) that
+ *       can trigger this action. Example: array('node_insert', 'user_update').
+ *       You can also declare support for any trigger by returning array('any')
+ *       for this value.
...
 function hook_action_info() {
@@ -106,48 +98,51 @@ function hook_action_info_alter(&$action
-function hook_hook_info() {
+function hook_trigger_info() {
   return array(
     'node' => array(
-      'node' => array(
-        'presave' => array(
-          'runs when' => t('When either saving a new post or updating an existing post'),
-        ),
-        'insert' => array(
-          'runs when' => t('After saving a new post'),
-        ),
-        'update' => array(
-          'runs when' => t('After saving an updated post'),
-        ),
-        'delete' => array(
-          'runs when' => t('After deleting a post')
-        ),
-        'view' => array(
-          'runs when' => t('When content is viewed by an authenticated user')
-        ),
+      'node_presave' => array(
+        'runs when' => t('When either saving new content or updating existing content'),
+      ),
+      'node_insert' => array(
+        'runs when' => t('After saving new content'),
+      ),
+      'node_update' => array(
+        'runs when' => t('After saving updated content'),
+      ),
+      'node_delete' => array(
+        'runs when' => t('After deleting content')
+      ),
+      'node_view' => array(
+        'runs when' => t('When content is viewed by an authenticated user')
       ),
     ),
   );

OK. What I absolutely do not understand is why hook_trigger_info() has been removed from some core modules, but not from system.module, and why the API docs still explain this hook - and more confusingly, it contains a copy of the removed node implementation...

Is it gone or not? The rest of the patch made me assume it would have gone... but, uhm. (?)

+++ modules/system/system.module	29 Aug 2009 03:26:26 -0000
@@ -2445,40 +2430,40 @@ function system_action_info() {
     'system_goto_action' => array(
       'description' => t('Redirect to URL'),
       'type' => 'system',
       'configurable' => TRUE,
-      'hooks' => array(
-        'node' => array('view', 'insert', 'update', 'delete'),
-        'comment' => array('view', 'insert', 'update', 'delete'),
-        'user' => array('view', 'insert', 'update', 'delete', 'login'),
-      )
+      'triggers' => array(
+        'node_view',
+        'node_insert',
+        'node_update',
+        'node_delete',
+        'comment_view',
+        'comment_insert',
+        'comment_update',
+        'comment_delete',
+        'user_view',
+        'user_insert',
+        'user_update',
+        'user_delete',
+        'user_login',
+        'user_logout',
+      ),

I didn't read the entire issue, but I read the reasoning for this by coincidence.

However, we should use 'any' here. It's up to the site admin to configure it properly.

+++ modules/trigger/trigger.admin.inc	29 Aug 2009 03:26:26 -0000
@@ -7,29 +7,29 @@
+ * @param $module_to_display
+ *   Which tab of triggers to display. E.g., 'node' for all
+ *   node-related triggers.
...
-function trigger_assign($type = NULL) {
+function trigger_assign($module_to_display = NULL) {

Why not simply $module ?

+++ modules/trigger/trigger.admin.inc	29 Aug 2009 03:26:26 -0000
@@ -40,24 +40,23 @@ function trigger_assign($type = NULL) {
  * Confirm removal of an assigned action.
  *
  * @param $hook
- * @param $op
  * @param $aid
  *   The action ID.
  * @ingroup forms
  * @see trigger_unassign_submit()
  */
-function trigger_unassign($form_state, $hook = NULL, $op = NULL, $aid = NULL) {
-  if (!($hook && $op && $aid)) {
-    drupal_goto('admin/structure/trigger/assign');
+function trigger_unassign($form_state, $group, $hook = NULL, $aid = NULL) {
+  if (!($hook && $aid)) {
+    drupal_goto('admin/structure/trigger');

$group argument has been added here without PHPDoc... Fixing the other @params would be neat, too, but not strictly required (though at least the new @param should have a proper description).

+++ modules/trigger/trigger.admin.inc	29 Aug 2009 03:26:26 -0000
@@ -98,30 +98,31 @@ function trigger_unassign_submit($form, 
+ * @param $group
+ *   The name of the trigger group, e.g., 'node'.
...
-function trigger_assign_form($form_state, $hook, $op, $description) {
-  $form['hook'] = array(
+function trigger_assign_form($form_state, $module, $hook, $description) {
+  $form['group'] = array(

It seems like $group was documented here instead.... erm, and also instead of $module ;)

+++ modules/trigger/trigger.admin.inc	29 Aug 2009 03:26:26 -0000
@@ -98,30 +98,31 @@ function trigger_unassign_submit($form, 
  * @return
+ *   Form array.

Nitpick: That can go.

+++ modules/trigger/trigger.admin.inc	29 Aug 2009 03:26:26 -0000
@@ -210,54 +210,53 @@ function trigger_assign_form_submit($for
-        'aid' => $aid, 
+        'hook' => $form_values['hook'],
+         'aid' => $aid,

The Wrong Indentation police will catch you! :-D

+++ modules/trigger/trigger.module	29 Aug 2009 03:26:26 -0000
@@ -91,26 +81,101 @@ function trigger_menu() {
+function trigger_trigger_info() {
...
+     'node' => array(
+       'node_presave' => array(
...
+     'comment' => array(
...
+     'taxonomy' => array(
...
+     'system' => array(
...
+     'user' => array(

huh? Now I'm totally confused. :)

I'm on crack. Are you, too?

jvandyk’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
88.67 KB

Thank you for your review!

Nitpick: Please leave blank lines between @param and @return. (possibly elsewhere)

The rest of core does not do this, and it's not in Documenting functions. Or maybe I've misunderstood you.

Hm. It would be nice to keep (update) those example array structures.

They are now in modules/trigger/trigger.api.php. We could copy them here, I suppose (since actions.inc can be used without trigger.module) or put a @see trigger.api.php. We could put @see hook_action_info()? I choose the latter.

If that example of *disabled* modules is really true, then we need to create a separate bug report, because no data should ever be removed for *disabled* modules.

I agree that we can do this separately.

$foo = 'bar';

Curses, my attempts to get this into core are foiled again!

OK. What I absolutely do not understand is why hook_trigger_info() has been removed from some core modules, but not from system.module, and why the API docs still explain this hook - and more confusingly, it contains a copy of the removed node implementation...

It's easy. hook_hook_info() has been renamed hook_trigger_info() and had its array structure made simpler. All core hook_hook_info() implementations have been moved into trigger.module, primarily to conserve memory since ostensibly they are unused unless trigger.module is enabled anyway. So system_hook_info() is gone and the information is now provided by trigger_trigger_info(), i.e., the trigger.module implementation of hook_trigger_info().

However, we should use 'any' here. It's up to the site admin to configure it properly.

OK. Changed to 'any'.

Why not simply $module?

Because $module is used within the function. Any naming change here is fine with me.

$group argument has been added here without PHPDoc

Now using $module throughout for consistency.

fago’s picture

Status: Needs review » Needs work

The patch contains like:

+<<<<<<< node.module
+=======
+>>>>>>> 1.1116

I wonder that it was able to pass the tests!?

Regardless from that what about renaming 'runs when' in hook_trigger_info() and 'description' in hook_action_info to what they really are 'label'. That would 1. make them more consistent and 2. make them consistent with rules.

Rules currently re-introduce similar hooks, hook_rules_action_info() and hook_rules_event_info(). However with the currently proposed design the core hooks come closer to the rules hooks, which I think is good so it's good as it's easier for people to deal with those similar APIs.. Rules uses 'label' in all hooks, so that would be consistent over all then.

It would be cool, if we event could start using the same hook. For hook_trigger_info() I see no problem as trigger.module manually invokes all triggers. For hook_action_info() it's probably better to keep them separated as rules only actions sitting in hook_action_info() would probably cause troubles as well as the other way round.

Anyway, what about using hook_event_info() instead of hook_trigger_info() as name? As I think the hook really describes *events* which in turn are used to *trigger* something.

fago’s picture

edit: double-submitted comment :(

sun’s picture

I agree that a better key would make sense -- however, we usually call this "title" in core.

Renaming the hook names is a bit too late and would require more discussion, I think...

fago’s picture

Status: Needs work » Needs review
FileSize
119.37 KB

ok, I had a look at it and replace 'description' of hook_action_info and 'runs when' of hook_trigger_info with label. The description of action appeared quite often, but I think I got them all. Also I added an update to rename the description column to label. While doing this change I found some other issues I corrected too:
* hook_action_info() and co is described in trigger.api.php - I corrected this to be in system.api.php. Then action configuration form lives in system.module although the menu items already stated system.admin.inc - where it belongs too. Fixed that too.

Also I fixed the problem in node.module mentioned above and removed the old node_hook_info() which also was left over.

Updated patch attached containing all described changes.

fago’s picture

@sun:
sry I was already working on the patch, so it implements 'label' now. However the whole field API uses already 'label' so it's already in use too.

jvandyk’s picture

Checking in on the run, but +1 for 'label' and the move of hook_action_info() to system.api.php. I also think hook_event_info() is a better name than hook_trigger_info() as a purist, but as a pragmatist hook_trigger_info() is probably more intuitive. Either is fine with me.

jvandyk’s picture

Status: Needs review » Reviewed & tested by the community

Applied the patch in #67. Installed D7. Ran tests, 0 errors. Assigned nonconfigurable and configurable actions to triggers. They fired. Not sure if I should be the one to RTBC this, but I'm not physically at Drupalcon so I can't get my hand slapped except virtually.

fago’s picture

@jvandyk: Great, it's working fine for me too!

@sun: "Renaming the hook names is a bit too late and would require more discussion, I think..."

hook_trigger_info() gets introduced with that patch, so I don't think it's too late to change it. So for rules, I care much about the terminology, as it's important for people to get it right. So with the keys changed, I think it would be fine to build upon the same hook for events, so I'd prefer hook_event_info() if that's fine with others.

fago’s picture

I took a closer look and noticed that the hook_trigger_info array is also keyed by modules. I wonder whether this shouldn't be something like 'group' ? Consider a module providing more node triggers, shouldn't they appear under 'node' too?
Anyway, in rules I don't have that hierarchy, 'module' or 'group' is an info array property there. Well, with rules2 I'm moving to 'group', which is just about grouping in the UI. Consequently it should be run through t() and so be a property. Do you think we should try to streamline that too? Well, we could also do so in a follow-up and let this patch fly.

jvandyk’s picture

Group is what we had before. We had a module - group - hook - 'runs when' hierarchy. Now we have a module - hook - label hierarchy. Other modules can still add hooks to modules. For example, I can have a nodefoo module that provides a trigger for the nodefoo_bar hook that it provides. But if that's node-related, it could define it under 'node' - 'nodefoo_bar' - 'After a bar has been assigned to this node'. That puts the new trigger in among the node triggers that core provides. So we have a simpler system, and a one-to-one correspondence with the menu system.

fago’s picture

I see, also the hook documentation explains that well. So let's go with that!

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

webchick’s picture

This is probably a quick re-roll just to catch the path change of Actions. Sorry. :(

jvandyk’s picture

Status: Needs work » Needs review
FileSize
119.33 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch failed testing.

jvandyk’s picture

Status: Needs work » Needs review
FileSize
119.58 KB
cweagans’s picture

Status: Needs review » Reviewed & tested by the community

Same code as before, so re-RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

sun’s picture

Issue tags: +API clean-up

Introducing a new tag for feature freeze: API clean-up.

sun’s picture

Status: Needs work » Needs review
FileSize
116.82 KB

Straight re-roll.

Well, while being there, I also fixed plenty of PHPDoc, coding-style issues, and removed all unnecessary hunks not belonging to this monster patch.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Green, so reverting status.

Dries’s picture

I spent 30 minutes with the latest version of this patch. Not enough for an in-depth review, but here is some feedback. I haven't found anything major (yet), which is a really good sign. Based on what I saw the last 30 minutes, I think this is in really good shape.

+++ includes/actions.inc	11 Sep 2009 22:15:53 -0000
@@ -187,28 +159,26 @@ function actions_get_all_actions() {
     $actions_map[$key]['callback']     = isset($array['callback']) ? $array['callback'] : $callback;
-    $actions_map[$key]['description']  = $array['description'];
+    $actions_map[$key]['label']  = $array['label'];
     $actions_map[$key]['type']         = $array['type'];
     $actions_map[$key]['configurable'] = $array['configurable'];

Inconsistent spacing. Really minor.

+++ includes/actions.inc	11 Sep 2009 22:15:53 -0000
@@ -234,26 +206,26 @@ function actions_function_lookup($hash) 
- * Actions provided by modules are synchronized with actions that are stored in
- * the actions table. This is necessary so that actions that do not require
- * configuration can receive action IDs. This is not necessarily the best
- * approach, but it is the most straightforward.
+ * Actions provided by modules in hook_action_info() implementations
+ * are synchronized with actions that are stored in the actions
+ * database table. This is necessary so that actions that do not
+ * require configuration can receive action IDs.

The text wraps too quick now.

+++ includes/menu.inc	11 Sep 2009 22:15:53 -0000
@@ -2785,6 +2785,9 @@ function _menu_router_save($menu, $masks
+    if (!isset($item['title'])) {
+      $foo = 'bar';
+    }

This is still here?

+++ modules/system/system.admin.inc	11 Sep 2009 22:28:14 -0000
@@ -2300,3 +2300,276 @@ function theme_system_themes_form($form)
+ * Menu callback; Creates the form for configuration of a single action.

By the way, I don't like how we started to capitalize the letter after : or ;. It is not how it was teached in my school, and we always had an unwritten rule not to. I'd like to switch back to lower case letters after : or ;. Thanks. Not going to hold up this patch for it though.

+++ modules/trigger/trigger.admin.inc	11 Sep 2009 22:15:53 -0000
@@ -210,54 +211,53 @@ function trigger_assign_form_submit($for
+    // TODO: need to support the new comment_presave hook here too?

A TODO? :)

We're still missing a CHANGELOG.txt entry.

jvandyk’s picture

FileSize
99 KB

Fixed spacing, text wrapping, removed foo (though foo still appears twice in menu.inc, hee hee). Left the menu callback comment alone (I agree with Dries, this is incorrect and should be corrected throughout core in a separate patch). Removed the TODO pending expanded test coverage later. Wrote CHANGELOG.txt entry.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

jvandyk’s picture

Status: Needs work » Needs review
FileSize
116.96 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
116.96 KB

Sorry, my monster FAPI clean-up patch introducing $form as first argument to all form builder functions broke this one.

It seems that all remarks have been resolved.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
116.99 KB

Fixed 3-4 old form builder signatures of this patch.

Dries’s picture

Issue tags: +Needs documentation

I looked at this again, and I believe this is an important improvement. Much simpler and better documented.

I committed this to CVS HEAD. Thanks all!

I'm marking it 'code needs work' because we'll want to update the upgrade instructions.

neclimdul’s picture

Status: Reviewed & tested by the community » Needs work

forgotten status change.

sun’s picture

jhodgdon’s picture

Status: Needs work » Needs review

I've updated the module 6/7 upgrade guide in accordance with this (huge) patch. Would be good if someone would review what I wrote, since the patch that went in was quite different from my original proposal, and I may not have noticed all the changes or understood them fully.

http://drupal.org/update/modules/6/7#trigger_overhaul

sun’s picture

Status: Needs review » Fixed

Looks accurate to me.

mattyoung’s picture

.

Status: Fixed » Closed (fixed)

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

sun’s picture

I seriously have problems to find people who understand Trigger/actions.

It would be awesome if all of you could quickly jump over to #585868: Triggers/actions need overhaul (part II) - Presave, so we can at least push one more required clean-up through to make actions not totally suck in D7.

To accomplish that, I need peer-reviews, now.