Actions: the first sweep

I tested this patch on the latest drupal 6.x-dev, but it seems that the "unpromote" action still leaves nodes as promoted.

Also, when trying to remove an action from an event, I get the following:
Fatal error: Call to undefined function drupal_delete_initiate() in /home/flo/workspace/Drupal-head/modules/actions/actions.module on line 766

The drupal_delete_initiate() function is supposed to be in includes/common.inc (according to api.drupal.org). Tell me if I'm wrong, but this seems to be related to the deletion API (so we shouldn't have this function call here since the deletion API won't be in 6.x).

Well, on the mailing list, you wrangled about lot of coding style errors in the actions patch. Here I only see repeating hook_action_info() copy-paste coding style problems (of which the user module one was already committed a few minutes ago).

I see some interesting use of indents in your actions_schema() changes, which does not seem like coding style comformant. Also, the actions_do() example you added is great, but it is indented one space less then it should be IMHO, and it could also use @code and @endcode tags for api module.

Unfortunately while your node_change_flag() looks like a good reusable function, it also makes it impossible to translate the watchdog messages, as the extractor will not be able to pick up the text from the parameters, unless we teach it this one exception too. (The list of these exceptions is getting too long already).

Fixed the coding style problems that I could find and rolled back the DAPI related changes. I copied this replacement code from a pre-DAPI revision of the original actions patch.

I'm not certain what to do concerning the translation issue with _node_change_flag(), so haven't updated that yet (advice please?) and with the post demote issue mentioned in #2, remember to add the "save post" action just like what the help says to do.

RANT:

My point is that I took literally *hours* reading the actions patch code from line to line (way before first applying it), compiling a list of sometimes coding style errors, sometimes bigger logical mistakes. You pointed your finger on *me*:

I am deeply disappointed with the fact of the commits and non commits. I am not pointing at John, if anyone, then my finger points to Dries for making an exception here. Or to the reviewers for not catching the many code style errors.

Dries did not do a coding style review on the module, or at least he did not comment about such errors on the issue. I had a miles long comment (http://drupal.org/node/148410#comment-258989) about coding style and other issues, and followed up on them with John to fix them.

The perceived problem here was fairness. If you spent 3+ hours reviewing it then surely you have seen the code style errors which would have hold back any other patch.

Well, we probably could have took two hours with each patch update, checking for newly added coding style problems. We did not do this.

After so much finger pointing, let me not commit this patch before it gets perfect enough that someone else will not come back to point on me again for not being fair enough.

ACTUAL REVIEW:

- (array)$actions; - do we have some agreed practice about how to write casts? I use to write casts with a space after the parenthesis...

- Looking at this code, I don't think it is current practice to have the first parameter on the same line as the function call and any more then one parameter on the same line (this happens multiple times in the patch):

+ confirm_form($form,
+    t('Are you sure you want to remove the action %title?', array('%title' => $actions[$action]['description'])),
+    $destination,
+    t('You can assign it again later if you wish.'),
+    t('Remove'), t('Cancel'));

- 'Action %action has been unassigned.', array has two spaces after the comma.

- unrelated menu module changes are included in the patch

- _node_action_t_flags() looks good but could use some documentation on what it does (think API lookups)

- drupal_set_message(t("Deleted orphaned action (%action).", array('%action' => $callback))) - AFAIK messages set for the user are not to be terminated with a dot (or if they are, other spots in the module and in Drupal in general should be fixed). The same problem occurs multiple times in the patch.