Actions in core

I did a review of this patch, and while I like the functionality of it there are a couple of remarks though.
I listed them under here:

+ watchdog('actions', t('Stack overflow; aborting.'), WATCHDOG_ERROR);
Ehm.. What is a stack overflow? Why is it aborting what? And what can I do to get around it?

+ require_once('./includes/actions.inc');
Every other inc-file that get's loaded, uses also require_once but without the brackets around it.
require_once './includes/$file.inc';

+ watchdog('action', t('Set comment %cid to unpublished', array('%cid' => $cid)));
Why are you using the $cid in here? It would be much better if you could include the comment title in here, and the author.

+ watchdog('action', t('Unpublished comment ' . $cid));
Same here..

+ watchdog('action', t('Set node id %id to published', array('%id' => $node->nid)));
+ watchdog('action', t('Set node id %id to unublished', array('%id' => $node->nid)));
+ watchdog('action', t('Made node id %id sticky', array('%id' => $node->nid)));
+ watchdog('action', t('Made node id %id sticky', array('%id' => $node->nid)));
+ watchdog('action', t('Promoted node id %id to front page', array('%id' => $node->nid)));
+ watchdog('action', t('Removed node id %id from front page', array('%id' => $node->nid)));
+ watchdog('action', t('Changed owner of node %id to uid %uid', array('%id' => $node->nid, '%uid' => $context['new_uid'])));
+ watchdog('action', t('Saved node %nid', array('%nid' => $node->nid)));
We try to omit terminology words like 'node' to be presented to the drupal user. It would be nicer if you used the $node->type name or the title here.
And, why are you using the id's everywhere? We don't do that anywhere inside drupal.. We try to give usable and good feedback..

+ '#description' => t('The message that should be sent. You may include the following variables: %site_name, %username, %node_url, %node_type, %title, %teaser, %body. Not all variables will be availablein all contexts.'),
Typo: 'availablein' => 'available in'

+ form_set_error('recipient', t('Please enter a valid email address or %author.', array('%author' => theme('placeholder', t('%author')))));
This doesn't seem right to me.

+ '%username' => $user->name ? $user->name : variable_get('anonymous', 'Anonymous'),
I thought we had to write:
'%username' => $user->name ? $user->name : variable_get('anonymous', t('Anonymous')),

+ '#description' => t('A unique description for this configuration of this action.'),
My native language isn't English, but this doesn't look like a good description.

+ watchdog('actions', t('Action %action deleted', array('%action' => check_plain($actions[$aid]['description']))));
+ drupal_set_message(t('Action %action deleted.', array('%action' => $actions[$aid]['description'])));
Again, pls no id's.. Give valuable and usable feedback, display the name of the actions or something..

A lot of (watchdog) messages and user feedback needs work imo. It's not straightforward and easy to understand what is going on after an action has been done.

This was the code review, I'll test the patch in a couple of hours from the UI pov..

Man, this is cool stuff! :-)

John,

from your code I learned what the:
+ watchdog('actions', t('Stack overflow; aborting.'), WATCHDOG_ERROR);
is about. But I was wondering if you could give it a more ehm.. Descriptive error message. "Stack overflow" doesn't say anything to the normal user.. Unfortunatly I can not come up with something better myself..

+ return array('#value' => t('This action requires the taxonomy module to be enabled.'));
It would be nice if you present a link in there which links to the modules page so people could enable the taxonomy module.

There are a lot of instances where you do:
+ watchdog('action', 'Unpublished comment %subject.', array('%subject' => $comment->subject));
Shouldn't it be better if we do:
+ watchdog('action', 'Unpublished comment "%subject".', array('%subject' => theme('placeholder', $comment->subject)));
Not sure what the coding guidelines say about the quotes and the theme('placeholder') usage though..

+ 'description' => t('Publish node'),
+ 'description' => t('Unpublish node'),
+ 'description' => t('Make node sticky'),
+ 'description' => t('Make node unsticky'),
+ 'description' => t('Promote node to front page'),
+ 'description' => t('Remove node from front page'),
+ 'description' => t('Change the author of a node'),
+ 'description' => t('Save node'),
Here you still use the word node, instead of post or even more preferable node_get_types('name', $node)..
We really have to omit the specific drupal terminology to make the user experience easier.

Furthermore this patch truly rocks! The code looks pretty good..

<offtopic>
As a sidenote John, would it - as an example - be possible to use actions to build derivatives from uploaded images on the fly? (this would make me extremely happy!)
And is there any documentation already written how people (like me :-) could implement actions into their module, or further down into drupal core?
</offtopic>

%var does an automatic theme('placeholder')... if you theme('placeholder') it again you get doubly-<em>ed text.

Here is my review after applying the patch:

After applying the patch, I went to 'admin/modules' to enable the actions.module.
Directly after hitting the 'Save configuration', there were the following status messages appearing which imo shouldn't be there:
* Action 'Unpublish comment' added.
* Action 'Publish node' added.
* Action 'Unpublish node' added.
* Action 'Make node sticky' added.
* Action 'Make node unsticky' added.
* Action 'Promote node to front page' added.
* Action 'Remove node from front page' added.
* Action 'Save node' added.
* Action 'Block current user' added.
* Action 'Ban IP address of current user' added.

When I navigate to 'Administer' > 'Site building' > 'Actions', the primary tab 'Assign actions' is selected, and as a secondary there is 'node' (which is the 3rd). This is weird..
We normally have an overview page to introduce people with what we can do there..

To be honoust, I never used actions and I did not had a clue what it would do. Now that I did have a look at it, I'm still clue less and still don't know what this is all about..

I'm sorry.. I really tried to understand what the meaning was of all this, but I failed.
Or fail to see the logic of all this..

What I expected to see:
I expected a UI which provides a way to bind certain actions to other actions.
- when we have a new comment in the comment queue, we automatically sent an e-mail to the comment approvers.;
- when a new user signs up and his account needs approvial, there is a mail sent to the administrator;
- when a user uploads an image, it could be resized to fit a certain width/height or generates a thumbnail of it;

And perhaps all this functionality is there, and I'm really some dump-ass who could not figure how to get things out.

John, if you want to talk to me on irc, or sent me a private mail to talk about things, that would be cool by me. I truly think this patch has great potential, but unfortunatly the UI ehm... sucks? Or I completely misunderstood the point..

Hope to speak to you soon!

I'm getting a crapload of errors when I apply the latest patch to the latest CVSed Drupal:
When I go to http://drupalhead/admin/build/actions/assign/node OR http://drupalhead/admin/build/actions/assign/comment OR http://drupalhead/admin/build/actions/assign/user I get a list of these two errors:

1. warning: Illegal offset type in isset or empty in /home/bennybobw/www/drupalhead/drupal/modules/actions/actions.module on line 534.
2. warning: htmlspecialchars() expects parameter 1 to be string, array given in /home/bennybobw/www/drupalhead/drupal/includes/bootstrap.inc on line 650.

when I go to http://drupalhead/admin/build/actions/assign/cron:

1. notice: Undefined index: actions_cron_run_assign_form in /home/bennybobw/www/drupalhead/drupal/includes/form.inc on line 245.
2. warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'actions_cron_run_assign_form' was given in /home/bennybobw/www/drupalhead/drupal/includes/form.inc on line 258.

When I go to http://drupalhead/admin/build/actions/manage, I get:

1. notice: Undefined index: field in /home/bennybobw/www/drupalhead/drupal/includes/tablesort.inc on line 151.

Looks like once the bugs get worked out, this is going to be an awesome add to Drupal core. Sorry I can't be of too much more help on the errors above, but I'd be happy to do another review!

It seems to be quite a similar framework (in terms of defining and mapping arguments to functions) to the XML-RPC server - is there code that could be shared here?

Would it be possible to automatically provide an XML-RPC handler for each action (or vice versa)?

When i try to delete a node i get :
( ! ) Fatal error: Call to undefined function: db_next_id() in /Users/adrian/dev/head/modules/user/user.module on line 3205

which is called from : /Users/adrian/dev/head/includes/actions.inc:90

Patch applied against HEAD-->
Patch succeeds but I get this error in admin > build > modules:

This version is incompatible with the 6.0-dev version of Drupal core.

I'd like to see this patch reviewed by 1-2 people. And when I say 'reviewed' I don't mean a two-line review. :)

looks like the files for the new actions module didn't get rolled into that patch; i've attached a patch for JUST those files to this post.

I'm getting a couple of errors. One appears when I hit the Manage Actions tab. It appears to be working properly, but the notice is there.

* notice: Undefined index: field in /Users/jeff/Sites/actions/includes/tablesort.inc on line 151.
* notice: Undefined index: field in /Users/jeff/Sites/actions/includes/tablesort.inc on line 151.

Then, on hitting the 'Cron' tab under Assign actions:

* notice: Undefined index: actions_cron_run_assign_form in /Users/jeff/Sites/actions/includes/form.inc on line 245.
* warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'actions_cron_run_assign_form' was given in /Users/jeff/Sites/actions/includes/form.inc on line 258.

I'm guessing a menu defintion was set up pointing to a bad function name but I haven't checked closer, yet.

Actions.inc:

- The phpdoc on actions_do() includes improper @param names and too generic descriptions
- Hm, your book has better examples then this for such dynamic where clauses, page 312 :) "$where .= 'OR aid = ' . $action_id . ' ';"
- If $function('do', $context, $object, $a3, $a4) is used, and $context and $objects is seemingly always the same, $a3 and $a4 could be $a1 and $2 maybe?
- You have quite a few end of line comments, which are not a prefered practice in core development AFAIK
- I don't buy your 'node' vs 'post' argument, all Drupal core interfaces use 'content' or 'post' in place of 'node', so the user whether a developer or not should be familiar with this terminology
- Would not $action be more specific here, $data is vague... "$result = db_query("SELECT * FROM {actions}"); while ($data = db_fetch_object($result)) {"
- "$data->params ? TRUE : FALSE" could just as well be (bool) $data->params
- Why don't you do the md5() in the SQL query to avoid traversing through all actions aids in actions_function_lookup().
- BTW the column 'func' is probably named so to avoid conflict with the reserved word 'function', but such abbreviations are not considered good in core AFAIS... Maybe use the 'callback' name as other core functionality does? Params to parameters similarly.
- Why is actions.inc a generic include file (not in the module folder and included in bootstrap)? What uses it is actions module is disabled? I cannot see.

Comment.module:

- Seems like the status of the comment is set to COMMENT_PUBLISHED while comment unpublishing :)
- Watchdog calls should not have t() in them in Drupal 6
- Instead of enforcing the awkward taxonomy module dependency, I would rename taxonomy_explode_tags() to drupal_explode_tags() or something similar and make it a utility function shared between modules.
- implode(',', $context['keywords']) will not with the quoted example, and would break that value... IMHO reuse the implode mechanism of taxonomy module and make that a shared function too.
- This action also identifies partial words, so it actually searches for *character sequences*, not *keywords*. To look for keywords, a word boundary should have been checked for. This should either be fixed or documented better.
- comment_unpublish_by_keyword_action() should not use query concatenation, but %d as everywhere else

Node.module:

- Again, I don't think 'node' on the interface is consistent and useful
- No t() should be used on watchdog()
- I am not sure that the node_publish_action() and node_unpublish_action() pair as well as the other action pairs are nicely done as standalone functions. As a first idea, making configurable actions our of them and saving the two preconfigured actions could look nicer, but making all of these configurable does not make sense as we don't have more than two options to further configure them. Anyway, these extremely simple actions somehow make me uncomfortable.
- node_assign_owner_action_form() could reuse the existing autocomplete for user names... I would not implement a custom UI concept there.
- Where is the save node action useful?

System.module:
- Why not use $account in system_send_email_action() in place of $user, as it is elsewhere
- The identation on the "$variables =" lines are interesting, I would put $variables and array( on their own lines to begin with
- You use check_plain() and XSS filtering, which is not appropriate for TEXT email, and url() is not wrapped in drupal_urlencode() for proper encoding in the mail
- drupal_mail() uses Drupal 4.7-era parameters, look http://api.drupal.org/api/HEAD/function/drupal_mail
- The ifs at the top of system_send_email_action_form() does not adhere to coding standards
- array('%author' => t('%author') is not good, as it should not be possible to translate the placeholder name (you do not use a translated name when replacing that, and anyway...), this can be safely removed as the placeholder will be kept intact in the text as intended, just as in the next long description you used.
- system_send_email_action_validate() should also have t() removed from around '%author'. If you would like to have placeholder formatting in the form error message, do array('%author' => '%author'), which might look odd, but that replaces the placeholder with the same text, and % instructs it to make it a placeholder too. I would add a comment there to document this interesting approach :)
- The array closing indentation is bogus in system_send_email_action_submit()
- The function content indentation is bogus in system_message_action_form()
- Some of your descriptions include one space after a dot at the end of sentence, some include one. Drupal uses one as far as I remember.
- system_send_email_action() uses the node of the comment from the context, system_message_action() loades that itself... inconsistent
- I would not burry global $user down into the system_message_action() function

User.module:

- Again, forget about check_plain() in t() in user_block_user_action() (forget t() in the watchdog too :)
- user_block_ip_action() could provide a more useful message like 'Banner IP address %ip'
- What happens if there is already such a rule in user_block_ip_action()? the database will throw us an error.

Actions.module

- "Enables functions to be stored as scripts to be triggered by other modules." is very unclear
- Concatenations in actions_help() are not coding style conformant
- It would be nice to break out the common firstsentence from the help texts and reuse that
- Indentation problems in actions_manage()
- The $action param to actions_configure() could be an md5 id and an action id too, but the configuration is misleading! Needs description on why it accepts both and when to use what.
- End of line comments again
- actions_delete_form(): coding style issue on the first line, improper t() usage later on.. Use %action if you would like to have a placeholder, which also check_plain()s the description, which would be required here.
- "note that in the future we need to check for dependencies here" what does that mean? Maybe mark it as TODO
- actions_delete_form_submit() watchdog should not be t()-ed :), placeholder in t() issue again
- Inconsistent placeholder usage in t() in actions_save()
- Is it the current practice to use variables instead of sequences???
- Too much whitespace at end of phpdoc of actions_delete() and actions_synchronize()
- You have calls like actions_synchronize(actions_list()), but that first parameter defaults to actions_list() anyway, so why pass that on?
- Why not use an array and then implode() or join() for $orphaned? String concatenation and rtim(rtrim())-ing is quite ugly.
- Put all the text into format_plural(), this $phrase based trickery is very translation unfriendly. End the sentence there. Then add some information about what is in the parenthesis. A warning message should be very descriptive to help the user troubleshoot the issue.
- The slash in "type/hook/op combination" means 'or' to me, not 'and'.
- At least two indentation problems in actions_assign_form()
- "Actions to execute during the @hook %view operation:" might get better with %view replaced with %operation as it is replaced with $op anyway.
- Lots of copy-pasted code in actions_forms() and actions_assign()... Maybe try to somehow reuse some values better?
- The confirm message you used in actions_assign_remove() is much more professional then the one in actions_delete_form()
- Again, no t() on watchdog, no check_plain to use if you use % in actions_assign_remove_submit()
- _actions_normalize_node_context() has an empty case for comments, which does not look right... If it is intended, include that in a comment, and explain there, why.
- actions_nodeapi() should collect static stuff to the top as everywhere else
- For actions_comment() the same applies, and it also has mysterious empty comment lines
- actions_cron() is in a quite interesting place, between the comment and user context handling
- _actions_normalize_user_context() does nothing, but in many lines of code

Huh. I took more then two hours with this patch, and I am quite exhausted. Maybe you excuse if I will not apply and try it in Drupal now to give UI feedback :)

md5(): John, even if it is not a performance issue, if you can do the code in one row which simply shows what is happening, it is better then having a query and then a while, checking and converting each row. I stopped at it and wondered why might it be important to have this out of sql (all supported DBMS have md5() supported, or am I missing something?)

actions.inc: yes, well, I did not see that anything calls action_do() if actions module is turned off (the assigned actions will not run as the nodeapi, user api and so on hook mappings will not fire if I understand it right) In this sense this seems to be a lot different to menu module, which is only an interface on the menu table and the menu will work fine without the module.

save post: I did not see the interface yet, so I cannot comment on whether this is obvious, that a save post action should be added when I have an action which modifies a node. I have also seen actions_do "Fires actions, in no particular order.". The save post action does not seem to be special... So what guarantees that it runs last?

dots and whitespace: Oh, I made an error there. So I have recorded this small nitpick for you when I noticed that some sentences have one space between them, some have two. I am not sure Drupal has some standard on this, but your patch is inconsistent in it. I think Drupal uses one space after sentences in descriptions and help text.

ip blocking: ok.

I'm going to be looking at this patch again tonight.

This is an absolute hard _requirement_ for 6.0 in my opinion.
It's an enabler, potentially even more so than FAPI (due to it's simplicity, and the fact that it faces the site admin, not the developer).

I would like to urge anybody who has the time, to give it a look.

/me admits to not having read the code... BUT
from what I have heard, actions is a way to do well, actions
actions.inc is api, and actions.module is a front-end, right?
I am wondering if in many core operations, they could be changed to use actions, to provide an extra layer of abstraction. note: I don't know what actions does, but I am inferring/

In lieu of a more comprehensive review that I can't finish right this second, let me mention the following items:

• This patch rocks!
• the node promote and unpromote actions do not trigger; they seem to have an $op in their function signature that may no longer be needed.
• Displaying messages to the user, and sending emails both seemed to work, but try as I might and regardless of what nodeapi hook I tried, I could not get any of the token substitutions to work beyond site name and user name. That implied to me that the other values weren't populated in the node object at that moment, but I even tried this on the 'view' hook with an existing node to no avail. Note that I did get the emails (unless I addressed them to %author) and did see the messages, but they just contained the tokens I had used except for the values of my node.
• While paging through the patch, I noticed at least one instance of a variable_get default set to 'drupal'; is this usually 'Drupal'?

I have some other suggestions about help text that I'll do my best to post later today.

This is super minor, but the doxygen for actions_do() and actions_list() have an empty line at the end of the comment block.

subscribe

Thought that I would test the UI know and if everything seems right, commit the patch, but:

$ patch -p0 < actions0619.patch
patching file includes/actions.inc
patching file includes/common.inc
Reversed (or previously applied) patch detected! Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 FAILED at 1229.
Hunk #2 succeeded at 2253 (offset 18 lines).
Hunk #3 succeeded at 2309 (offset 18 lines).
Hunk #4 succeeded at 3594 (offset 388 lines).
1 out of 4 hunks FAILED -- saving rejects to file includes/common.inc.rej
patching file modules/comment/comment.module
Hunk #1 succeeded at 2088 (offset 27 lines).
patching file modules/node/node.module
Hunk #2 succeeded at 3166 (offset 55 lines).
patching file modules/system/system.module
Hunk #1 succeeded at 2744 (offset 26 lines).
patching file modules/taxonomy/taxonomy.module
Hunk #1 succeeded at 826 (offset 2 lines).
Hunk #2 succeeded at 1502 (offset 2 lines).
Hunk #3 succeeded at 1526 (offset 2 lines).
patching file modules/user/user.module
Hunk #1 succeeded at 3188 (offset 2 lines).
can't find file to patch at input line 1091
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: modules/actions/actions.inc
|===================================================================
|RCS file: /cvs/drupal-contrib/contributions/modules/actions/actions.inc,v
|retrieving revision 1.21
|diff -u -u -p -r1.21 actions.inc
|--- modules/actions/actions.inc 24 May 2007 20:30:56 -0000 1.21
|+++ modules/actions/actions.inc 19 Jun 2007 18:10:48 -0000
--------------------------
File to patch: modules/actions/actions.inc
modules/actions/actions.inc: No such file or directory
Skip this patch? [y]
Skipping patch.
2 out of 2 hunks ignored
patching file modules/actions/actions.info
patching file modules/actions/actions.install
patching file modules/actions/actions.module
patching file modules/actions/actions.schema

Seems like have outdated stuff in the patch. The modules/actions/actions.inc patch part also seems to be against an *existing* file, which is rather odd.

Great, reviewed the UI, here are my comments:

- the help text still full of "node"s: "such as when a node is added or a user logs in. Below you can assign actions to run when certain node-related operations happen. For example, you could remove a node from the front page when the node is updated. Note that if you are running actions that modify the characteristics of a node, you will need to add the Save node action to save the changes."

- "Actions to execute during the nodeapi presave operation:" and similar titles are very unhelpful... Who knows what a nodeapi presave is? Even Drupal 5 developers will not know it, presave is new stuff... With CCK and content types in core, there are even less of a need for *even developers* to know about nodeapi, let alone day to day users. Could we make these titles human readable text? A possible example "Actions to execute during the nodeapi update operation:" == "Actions to execute when a post is modified:". This could be much more accessible to actual users. Of course this would need these titles to be defined for each context instead of generating them automatically.

- I tried to add a configurable action and noticed the following; the baseline of the Configure button was at the top of the select field, which looks very odd; the action's configure page works; the actions delete operation gives a white screen and when coming back to the listing, it prints an undefined output variable error remembered from the whitescreened page probably; I added the saved configured comment action to the comment insert operation, but then when tried to remove it, I got a "Are you sure you want to delete the action Unpublish comment containing test?" titled confirmation screen with Y, Y and Y as the page text, submit button and cancel link label. All these look broken.

Now the we have the code looking good, let's fix these usability issues! The actions feature is coming along nicely!

drupal_html_to_text() spin-off: http://drupal.org/node/154218

- Fixed some PHPDoc spacing weirdness.
- Replaced instances of 'Drupal' with variable_get('site_name')
- Tried to change all "node" to "post" or "content"
- Changed the phrase "configurable action instance" (whaaa?) to "Advanced action"
- I tried to make the examples a little less arbitrary in places. For example, "you could unpublish a post when a comment is added." Why would you ever want to do that?

- There is still a lot of this:

) . '</h3>';

which should be (no space before quote):

) .'</h3>';

I caught some of them, but I'm sure there are more. Give the module a spin through with Coder module.

Usability-wise:
- I concur with Gabor's statement about the $hook $op stuff. That's a usability nightmare. The code doesn't appear to make it easy to change this, however. Maybe an array during the actions-gathering process like:

$operations[] = array($hook, $op, $description)

and display $description in the user-facing text?

- I *really* wish it wouldn't do 10,000 drupal_set_messages() about all the Actions it installed when you first install the module. This is very inconsistent with any other core module, and makes the user go "OH NO!! WHAT DID I JUST DO?!?" However, looking at the code, I didn't see a really easy way to solve this without removing the message entirely, in favour of a single "New actions were added. PLease check the actions status page for more info.

John, no hook registry system would help that we need actions module specific descriptions for all hook-op combinations appearing in connection to actions. Look at my examples above.

I have attempted to cleanly remove the old html to text stuff and switch to Steven's recently committed patch (which was causing the patch to fail to apply). I also cleaned up the comment spacing issue I mentioned earlier.

Testing the patch I get an error:

    * notice: Undefined variable: recipient in /home/obarton/workspace/drupal-head/modules/system/system.module on line 2931.
    * notice: Undefined variable: subject in /home/obarton/workspace/drupal-head/modules/system/system.module on line 2931.
    * notice: Undefined variable: body in /home/obarton/workspace/drupal-head/modules/system/system.module on line 2931.
    * notice: Undefined variable: recipient in /home/obarton/workspace/drupal-head/modules/system/system.module on line 2935.
    * warning: Illegal offset type in isset or empty in /home/obarton/workspace/drupal-head/includes/actions.inc on line 59.

- when trying to fire a configurable (e-mail) action - which prevents the action from completing. Examining the $action_id's passed in to actions_do() it appears that an array with a single element 'type' => 'system' is being passed in, rather than a string id. I don't have time to look at this now, but it is probably something simple.
Regular actions appear to fire fine, however.

I did a quick review of the patch and a tryout of the functionality:

- The misaligned Assign button on the manage page is still bogus, which is probably a result of this HTML structure:

<div class='container-inline'><div class="form-item" id="edit-action-wrapper">
 <select name="action" class="form-select" id="edit-action" >.......</select>
</div>
<input type="submit" name="op" id="edit-submit" value="Configure"  class="form-submit" />
</div>

- When I delete an action on the admin/build/actions/manage page, I get directed to the actions/assign/node page, which is bad. The cancel link in the confirm form points back to the config page properly though.

- "Remove post from node page" should be "Remove post from front page", this seems to be a typo.

- 'nodeapi' and 'node' mix up in the URLs, one bug that is a result of this is that when I remove an action on from the node assigned actions, I get to a admin/build/actions/assign/remove/nodeapi/insert/... page, which consequently redirects me back to a admin/build/actions/assign/nodeapi page (which accidentally displays the node actions page, because it is the default local task, but the node tab is not selected of course).

- It would be great to reword the delete confirm screen to indicate that the assignment of the action is deleted, not the action itself, when deassigning an action. This is nicely reflected already with the 'remove' action link text vs. the 'delete' action link text.

- Small nitpick from the source code that .' ' .t( should be .' '. t( instead in actions_help().

- + $items['admin/build/actions/assign/node'] = array('title' => 'Node', should have the title 'Content' as far as I see. Other concepts like 'Cron', 'User' and 'Comment' are well known concepts for the user, node is not. 'Post' would not be a good substitute for 'Node' in this case, as it is a verb, which could misdirect people about what is behind the tab. But 'Node' should be eliminated from here too. Otherwise it is nicely eliminated from the other places. (I do understand that the actions are listed by modules in the dropdowns, so 'node' seems to be applicable there). You used the module name to name these tabs automatically, but hook_actions_info() could return a key for the human readable module name. Think about how .info file specified module names are displayed in the module listing even, instead of module "code" names. Module code names are not good for the UI, not even for the module listing, and especially not for a user facing interface like this.

- It is odd that admin/build/actions/manage has no description, but the sister tab admin/build/actions/assign on the same level has.

- The 'Actions to execute when !description:' type of string composition is not localization friendly. Some languages need to reorder parts of the sentence, or need to use different words for different action types. Include the whole sentence in the actions info hook (without the semicolon) please. Unfortunately these nice looking simplifications you have done here with the string composition does not work well with localization. It makes life a bit harder. Maybe you can omit 'Actions to execute' from the start of the sentence altogether and not loose much understandability. I am not entirely sure, but it could be common sense to know that these are actions to execute. The wording of the action names also helps a lot already to get this mental model. That could help keep the minimal hook registry 'independent' of actions.

- The empty comment line in actions_comment() looks odd.

- I don't think it is ever practice in Drupal core to use 'global' or 'static' withing the body of a function (it is always at the top of the function to cleanly have an overview of the used globals and statics), although you do this multiple times. AFAIR, I already mentioned this above.

- There is one dependencies TODO, which seems like not applicable to the current actions implementation (there are no dependencies?!). There are three action context normalization TODOs which should be implemented though to provide a stable API (of which changes are not generally allowed beyond code freeze).

I'll make sure (as much as I can do) to have this committed before the code freeze if my concerns are fixed.

I'm not presuming to speak for anyone else, but one bit of 'mis-alignment' that I did notice when checking out various iterations of this patch, including the latest one, was the behavior demonstrated by the select box and submit button at the bottom of the attached screenshot.

This is in Firefox 2.0.0.4 on Windows Vista.

Attached an illustration of what I see (in Firefox 2.0.0.4, Ubuntu Linux). I tried to work around the display bug, and removing class="form-submit" from the configure button did the trick. This is obviously not the solution to take, but as other select box + submit button combos on the 'Assign actions' tabs are looking really nicely aligned, I think the solution is probably not far away.

OK, let's decide how the remaining three TODOs will be fixed, or omit that functionality if it is not going to work.

spent some time playing with this patch. couple of things that i noticed:

• i'm guessing this would be tricky to overcome, but there are actions listed in dropdowns which make no sense for what you're assigning it to. for example, pretty much every action under "When a user account has been deleted" makes no sense. it's a bit confusing for the end user.
• looks like you're using $node->type for the %node_type token, instead of the human-readable name -- wouldn't it be better to use the human-readable name?
• you're not properly using the deletion API :) i took mercy on you and rolled it into the attached patch. couple of things:
  • actions_delete() was not deleting actions from the {actions_assignments} table. fixed.
  • deleting actions and deleting actions assignments were both tested and working.
  • i really don't know how to test the deletion code for removing orphans, so you'll have to handle that. i rewrote that section, however, to use just one database query, as i thought it made things much cleaner.
  • i have no idea what's going on in actions_user(), actions_comment(), or actions_nodeapi(). if there are database deletions happening, then let me know and i can help with any conversion.
• the whole way you're using and generating the aid seems extremly clunky. we should either use strings for the identifier or integers, not a mix. we've got a description column, so i vote for aid as an integer.
• actions_assign_remove() is a pretty confusing function name for what it does -- how about actions_unassign()?
• i'm very confused why you need to pass the aid as a hashed value when building the link to remove actions -- is that because you're using strings at times?

Setting status accordingly. Will try and test this thoroughly again tonight, but the UI looks MUCH better. Thanks, John!!

I think all references to "post" on the "Assign actions > Content" page should be changed from "a post" to "content" ("When a post is about to be saved" to "When content is about to be saved.") From what I can see in the rest of the Drupal interface "Post" is used as a verb, not as a noun, e.g. "Post comment." To an end user I think this is confusing, especially since the tab up top says "Content" not "Post." For many end users "post" is synonymous to "Forum post" or "Blog post."

Also, I realize that all of the descriptions have been written to start with "When" but I think it might be clearer to change "When a post is about to be saved" to "Before a post is saved" and "When a post has been created" to "After a post has been created."

In addition, I think either "saved" or "created" should be used, not both. I think they're talking about the same thing.

I don't know if this is just a HEAD thing or not, but I'm getting the following errors when I use the "Send email" action:

notice: Undefined index: account in /home/bennybobw/www/drupalhead/drupal/modules/system/system.module on line 2968.
Which says:

case 'user':
      // Because this is not an action of type 'user' the user
      // object is not passed as $object, but it will still be available
      // in $context.
      $account = $context['account'];

And another error:
notice: Trying to get property of non-object in /home/bennybobw/www/drupalhead/drupal/modules/system/system.module on line 2996.
$variables['%username'] = $account->name;
Looks like they're related.

Question: is is possible to assign actions based on node type? What if I want to email Tom when node type "B" is published and Suzy when node type "A" is published?

Cool patch.

This is nitpicky, but Drupal uses "anonymous user" and 'authenticated user." Not "logged-in user."

I retested everything and this is now RTBC. I have some minor change requests outstanding, but those can come later.

i made a node_unpublish_by_keyword_action based on the comment one of same name. i think people want this for nodes as well as comments. i also expanded both to work on titles as well as body. also using a textarea to enter bad words, instead of a textfield. thus, we deprecate the clumsy wordfilter.module.

this was a minor copy/paste job to a function, so i think rtbc is still appropriate.

In general, this patch looks good. There are still some weird things:

It's possible to configure: "When a user has logged in - promote node to the front page". That doesn't look quite right. There seems to be no actions that can be associated with users. And the user-actions that are associated with nodes, look weird. Why would I want to 'block the current' user when he/she deleted a node? I might want to block the author of the node, but not the current user. Maybe the user actions are in the wrong place?

The link to remove an action is called "remove". When you click the remove link that terminology should remain. It's a bit weird to read "unasign".

Last, when I'm configuring an action (i.e. admin/build/actions/config/$something) the breadcrumb seems incorrect. Minor detail.

That said, I think we're very close to committing this patch. Maybe one more re-roll based on #57?

I've been watching the patch for a while though I haven't been able to chime in as much as I'd like. A couple observations:

1) The underlying actions functionality is tremendously solid. I've used the old actions api in a number of situations and it really is a great tool. Getting this into core is a huge win.

2) The system of mapping actions to core hooks and ops is very, very slick. I'm concerned, though, that the mix of actions available in core will leave most users scratching their heads. Dries' example above (Publish a node on user login?) is a very, very, very edgy edge case and it's likely the first thing that users will see when they start poking around the related configuration screens.

It's not a show stopper, but ultimately we need PILES of granular actions all over the place for this to go to the next level. If the actions API is in place, is there any chance that writing and polishing additional discrete actions (like touching a node's created/updated date, or adding a role to a user) might be added after the freeze, as under the hood enhancements?

3) The lack of support for conditional evaluation is very tricky. Actions are best when they are simple and granular -- promoting a node, etc. Any time the phrase, "If..." appears in the description of an action's task, I cringe. This was a decision necessitated by time constraints.

I hope, though, that we could at least keep some of the 'actions that implement their own conditional branching' to a minimum until we can solve the issue of 'arbitrary even + arbitrary conditionals + arbitrary actions' at an architectural level. This isn't a problem with the patch, just a concern about how we'll direct people to build on it in the future, and in contrib.

All that said?

We need this functionality and the actions API itself is very, very solid. It is a huge step forward in Dries' vision of 'eliminating the developer'. If I had three thumbs up, I would give it.

Alright, I've spent another 30 minutes with this patch and it's solid. An incredible amount of polish went into this. While there is room for improvements, and specifically, room for some extra actions, this shouldn't hold back this patch. Let's move on!

A big thanks, John! You rock. :)

Just installed Drupal HEAD from scratch, and enabled the actions module since this patch when in, and I get this:

* user warning: BLOB/TEXT column 'parameters' can't have a default value query: CREATE TABLE actions ( `aid` VARCHAR(255) NOT NULL DEFAULT '0', `type` VARCHAR(32) NOT NULL DEFAULT '', `callback` VARCHAR(255) NOT NULL DEFAULT '', `parameters` LONGTEXT NOT NULL DEFAULT '', `description` VARCHAR(255) NOT NULL DEFAULT '0', PRIMARY KEY (aid) ) /*!40100 DEFAULT CHARACTER SET UTF8 */ in D:\websites\drupal-HEAD\includes\database.mysqli.inc on line 154.
* user warning: Table 'drupal_head.actions' doesn't exist query: SELECT * FROM actions WHERE parameters = '' in D:\websites\drupal-HEAD\includes\database.mysqli.inc on line 154.
* user warning: Table 'drupal_head.actions' doesn't exist query: INSERT INTO actions (aid, type, callback, parameters, description) VALUES ('comment_unpublish_action', 'comment', 'comment_unpublish_action', '', 'Unpublish comment') in D:\websites\drupal-HEAD\includes\database.mysqli.inc on line 154.

...[snip]...

* user warning: Table 'drupal_head.actions' doesn't exist query: INSERT INTO actions (aid, type, callback, parameters, description) VALUES ('user_block_ip_action', 'user', 'user_block_ip_action', '', 'Ban IP address of current user') in D:\websites\drupal-HEAD\includes\database.mysqli.inc on line 154.

Patch attached to get rid of default for the text filed 'parameters'. Hope the patch file is in the correct format.

sorry if this patch should be in a new thread, been out the loop for a while.

Committed. Thanks.

hook_action_info() and hook_action_info_alter() remain undocumented.

And still, not documented on api site. the doxygen for actions_list() give most of the needed info.

I'd like to understand the rationale behind the stack overflow guard in actions_do(). Today (D6), if I understand correctly, the code aborts when it encounters more than 35 (or the value of actions_max_stack) invocations of actions_do() during one page rendering, *even if those invocations are sequential, not recursive*. I don't think that this is what's intended. Instead, I would have expected the code to abort if there are more than 35 recursive invocations of actions_do(), which would be achieved by just adding

$stack--;

just before the return of actions_do();

I am bringing this up because my module Views Bulk Operations allows one to select an arbitrary number of nodes and to apply a specific action on each of them. The code only works currently if I set actions_max_stack to a large number, which strikes me as inelegant and illogical.

@kratib - I would suggest you open this as a new issue/patch, since this issue is only open for completion of documentation

Moved my discussion to http://drupal.org/node/290282 - sorry for the inconvenience.

hook_action_info() and hook_action_info_alter() have been documented.

Just tried to find this and was completely unable to do so.