Follow ups:

Problem/Motivation

The Actions API is split across the System module and actions.inc in Drupal core. It's code is not required to be run on every Drupal website, and it's difficult to track down where all of the code is.

Proposed resolution

Move the Actions API to an Actions module.

Remaining tasks

User interface changes

No user interface changes, other than the module needing to be enabled.

API changes

Modules that use the Actions API directly will now have to depend on the Actions module.

Original report by catch

Opening this as part of the overall effort at #679112: Time for system.module and most of includes to commit seppuku.

Actions is currently split between includes/actions.inc and a bunch of badly namespaced functions in system.module, this doesn't make any sense to me, and wasn't the case with the original actions patch (which had an actions module handling both the actions UI and what is now 'triggers').

I'd suggest:

1. Move includes/actions.inc into modules/actions/actions.module
2. Move the actions ui out of system module into actions module.
3. Consider moving system_action_info() into actions.module, since there's no reason for actions.module not to provide those basic actions.

CommentFileSizeAuthor
#93 platform.actions.92.patch78.81 KBsun
#93 interdiff.txt1.49 KBsun
#89 platform.actions.89.patch78.24 KBsun
#89 interdiff.txt8.95 KBsun
#87 platform.actions.87.patch81.85 KBsun
#87 interdiff.txt548 bytessun
#81 platform.actions.81.patch81.77 KBsun
#81 interdiff.txt47.39 KBsun
#76 platform.actions.76.patch61.67 KBsun
#76 interdiff.txt2.04 KBsun
#68 platform.actions.68.patch61.99 KBsun
#68 interdiff.txt3.19 KBsun
#66 platform.actions.65.patch61.97 KBsun
#66 interdiff.txt518 bytessun
#63 platform.actions.62.patch62.06 KBsun
#63 interdiff.txt1.06 KBsun
#61 platform.actions.61.patch62.37 KBsun
#61 interdiff.txt647 bytessun
#53 platform.actions.53.patch62.86 KBsun
#53 interdiff.txt1.78 KBsun
#52 platform.actions.52.patch62.54 KBsun
#52 interdiff.txt36.46 KBsun
#45 1008166-actions.patch74.44 KBRobLoach
#42 1008166-make_actions_module-42.patch90.58 KBParisLiakos
#42 1008166-interdiff-40vs42.diff3.15 KBParisLiakos
#40 1008166-make_actions_module-40.patch90.48 KBParisLiakos
#39 1008166-make_actions_module-39.patch90.48 KBParisLiakos
#37 drupal8.actions.37.patch62.14 KBsun
#34 1008166-diff.patch73.87 KBRobLoach
#34 1008166-formatpatch.patch64.22 KBRobLoach
#28 actions.patch102.48 KBcatch
#26 actions.patch103.04 KBcatch
#24 actions.patch103.04 KBcatch
#22 actions.patch128.57 KBcatch
#19 actions_1008166.patch103.13 KBcatch
#18 actions_1008166.patch50.94 KBcatch
#12 actions_1008166-12.patch101.48 KBcatch
#11 actions_1008166-11.patch89.3 KBJody Lynn
#10 actions_1008166.patch89.29 KBcatch
#5 actions_1008166.patch88.88 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Subscribe - this looks ++ good

andypost’s picture

Agree that system_action_info() should be actions_action_info()

Questions left:
1) Should actions module be visible/disable-able
2) Triggers module - maybe just split actions hunk with trigger module - as it was before core's life of actions
3) Another point of view: actions & triggers => actions (hidden module) & triggers (UI module), I really like UI separation

catch’s picture

I would hope we could make it an optional module, and rely on dependencies for modules that want to use the functionality. I think we should go that way with just about every core module, while also moving more bits of system.module and/or /includes (especially the bits hardcoded into _drupal_bootstrap_full()) into modules at the same time.

catch’s picture

Status: Active » Needs review
FileSize
88.88 KB

Completely untested patch. Did not touch trigger module at all apart from adding a dependency.

Idea here is to not change any UIs, and as little in the UI as possible (only hook_help() changes in that regard), and just move the code around.

catch’s picture

Category: bug » task
sun’s picture

+1

Jody Lynn’s picture

+++ modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+// $Id

Missing $

+++ modules/actions/actions.module
@@ -0,0 +1,715 @@
+function actions_permission_info() {

Wrong function name

+++ modules/actions/actions.module
@@ -0,0 +1,715 @@
+    'file' => 'system.admin.inc',

These all need to point to actions.admin.inc

I tried to enable actions module and test this out, but I couldn't even get it to enable so far...

Powered by Dreditor.

Jody Lynn’s picture

Status: Needs review » Needs work
catch’s picture

Status: Needs work » Needs review
FileSize
89.29 KB

Fixes the points raised by Jody Lynn, thanks! Actions module now installs via the UI for me.

Jody Lynn’s picture

FileSize
89.3 KB

I got the actions UI working.

Still needs some things moved out of system.api.php and system.install as well

catch’s picture

FileSize
101.48 KB

Re-rolled #11 including hunks from system.install and system.api.php.

We'll need an update to enable actions on existing sites as well, that's not done yet.

Jody Lynn’s picture

Also there are now some functions named as 'actions_actions_' (used to be 'system_actions_') which could get their names simplified.

The actions menu item is still in the 'System' section of 'Site configuration'. I think it would be better to move it to 'Structure' so it can be closer to Triggers.

9byte1’s picture

Should actions module be visible/disable-able?

Jody Lynn’s picture

Definitely. (Can't wait to disable it... All it's every done for me is make me worry about its orphans)

Jody Lynn’s picture

We should also rename hook_action_info to hook_actions_info. All the other actions hook use the plural and we need the hook names to match the new module name.

sun’s picture

Actually... the module and function names should be singular, like most/all other core modules. The "entity" is an action.

catch’s picture

FileSize
50.94 KB

system_mail() and a couple of other functions were thinly disguised actions functionality. This patch moves those as well.

I do not want to rename the module and function name in this patch if we can avoid it:

1. Current patch should be completely backwards compatible with D7, it is purely moving code around. Renaming in the same go means rewriting a bunch of strings and will make the patch unreviewable.

2. With git renaming modules and keeping history should be easy.

catch’s picture

FileSize
103.13 KB

Well it would move them if I'd done git add.

catch’s picture

cweagans’s picture

Now that the git migration is done, we need to remove all $Id$ tags from incoming patches.

Will try to actually review the code later.

catch’s picture

FileSize
128.57 KB

Re-rolling minus the $Id tags, didn't run tests or anything but putting past the bot to see how bad it is.

Status: Needs review » Needs work

The last submitted patch, actions.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
103.04 KB

Cruft in me patch, this should at least apply.

Status: Needs review » Needs work

The last submitted patch, actions.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
103.04 KB

Fixed the syntax error and also two constants s/WATCHDOG/LOG/g

Status: Needs review » Needs work

The last submitted patch, actions.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
102.48 KB

Sorry..

Status: Needs review » Needs work

The last submitted patch, actions.patch, failed testing.

cweagans’s picture

+++ b/modules/actions/actions.installundefined
@@ -0,0 +1,55 @@
+/**
+ * Implements hook_schema().
+ */
+function system_schema() {

This should probably be actions_schema, no?

Also, is there any reason to keep a separation between action and trigger module? I don't know of anybody who uses trigger without actions, or actions without trigger. I'm in favor of merging the two modules into actions module.

catch’s picture

It doesn't make sense to have triggers without actions, but i think it could make sense to have actions without triggers (if it was cleaned up and improved it could be a better basis for Rules to build on for example).

sun’s picture

Trigger is not to be discussed here: #764558: Remove Trigger module from core

RobLoach’s picture

Sign me up.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
64.22 KB
73.87 KB

Would love for this to move forward. Is this the right idea for the update?

/**
 * Enable the Actions module without re-installing its database schema.
 */
function system_update_8011() {
  // Reset the module data so that the actions module appears in the list.
  drupal_static_reset('system_rebuild_module_data');
  system_rebuild_module_data();

  // Actions is now in the system table, so we can now enable the module without
  // re-installing the schema.
  db_update('system')
    ->fields(array(
      'status' => 1,
      'schema_version' => 0,
    ))
    ->condition('type', 'module')
    ->condition('name', 'actions')
    ->execute();
}

We need the module to be enabled as the actions schema is already currently installed. This just enables the module without re-installing its schema.

sun’s picture

I think that's a good first step. As soon as the plugin system lands, actions should totally be plugins ;)

Will try to review later.

RobLoach’s picture

#34: 1008166-diff.patch queued for re-testing.

sun’s picture

Assigned: catch » Unassigned
FileSize
62.14 KB

Regular diff against HEAD.

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/actions/actions.module
@@ -2,26 +2,336 @@
+function actions_send_email_action($entity, $context) {
+  if (empty($context['node'])) {
+    $context['node'] = $entity;
+  }

php-doc does not tells about node tokens. suppose node module should cares about inserting 'node' key into context.
Node module is optional for now so no reason to reference it as special case

+++ b/core/modules/actions/actions.module
@@ -2,26 +2,336 @@
+    '#description' => t('The message to be displayed to the current user. You may include placeholders like [node:title], [user:name], and [comment:body] to represent data that will be different each time message is sent. Not all placeholders will be available in all contexts.'),

[user:name] should be first in list, do we have difference between user: and account: tokens?

+++ b/core/modules/actions/actions.module
@@ -2,26 +2,336 @@
+ * Sends a message to the current user's screen.
+ *
+ * @param object $entity
+ *   An optional node entity, which will be added as $context['node'] if
+ *   provided.
+ * @param array $context
+ *   Array with the following elements:
+ *   - 'message': The message to send. This will be passed through
+ *     token_replace().
+ *   - Other elements will be used as the data for token replacement in
+ *     the message.
  *
- * Action functions are declared by modules by implementing hook_action_info().
- * Modules can cause action functions to run by calling actions_do().
+ * @ingroup actions
+ */
+function actions_message_action(&$entity, $context = array()) {
+  if (empty($context['node'])) {
+    $context['node'] = $entity;
+  }

so this will only use current logged-in user as token and 'node' somehow

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
90.48 KB

I completely removed $object parameter from actions_do()

It doesn't make any sense, having it there, it just make everything more confusing. The caller of actions_do should make sure that the node, comment or user object is inside the context variable, so it can be used in token_replace.
Unless we want to make this more general and also pass the $entity_type of the object in actions_do and actions_do supply it as an argument in the context, which is pointless imo.

This patch conflicts with #1161486: Move IP blocking into its own module depending which get commited first, since block_ip should be in ban module and renamed to ban_ip action. but like i said i will wait to see which gets commited first

ParisLiakos’s picture

missed

[user:name] should be first in list
andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+  foreach ($actions_map as $key => $array) {
+    if ($array['configurable']) {
+      $options[$key] = $array['label'] . '...';
+    }
+    else {
+      $unconfigurable[] = $array;

Needs better name then $array, suppose $action is more sane

+++ b/core/modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+ * Define the form for the actions overview page.

Defines

+++ b/core/modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+ * @return
+ *   Form definition.

@return should have blank line before. see http://drupal.org/node/1354#functions

+++ b/core/modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+ * Process actions_admin_manage form submissions.

Processes

+++ b/core/modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+ * Menu callback; Creates the form for configuration of a single action.

Form builder functions should follow http://drupal.org/node/1354#forms

+++ b/core/modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+ * @return
+ *   A form definition.

same

+++ b/core/modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+ * Validate actions_admin_configure() form submissions.

Validates

+++ b/core/modules/actions/actions.admin.inc
@@ -0,0 +1,280 @@
+ * Process actions_admin_configure() form submissions.

Processes

+++ b/core/modules/actions/actions.module
@@ -2,26 +2,336 @@
+ * Return a form definition so the Send email action can be configured.

Returns

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
90.58 KB

I hope i did not miss anything:)

Status: Needs review » Needs work

The last submitted patch, 1008166-interdiff-40vs42.diff, failed testing.

andypost’s picture

Status: Needs work » Needs review

Patch seems rtbc!
Suppose it should be commited and actions could be converted to plugins in follow up issue

RobLoach’s picture

FileSize
74.44 KB

Was missing a newline at the top of actions.admin.inc. Rolled with -M25%. Not really required, but might help patch size.

dagmar’s picture

+++ b/core/modules/actions/actions.module
@@ -0,0 +1,692 @@
+  if ($stack > variable_get('actions_max_stack', 35)) {

Question. Should this be converted into the new Configuration System?

RobLoach’s picture

In a separate issue, sure! Mind creating an issue about converting Actions over to config so we can track it? :-)

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in! Do we need another review?

RobLoach’s picture

#45: 1008166-actions.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Framework Initiative, +API clean-up

The last submitted patch, 1008166-actions.patch, failed testing.

Lars Toomre’s picture

I am aware that most of this new module is moving around existing code. However, if this patch gets re-rolled again can we try to bring it up to D8 documentation standards since it now will be its own module?

The types of documentation problems I saw include:
a) Missing @param/@return type hints.
b) Some missing @param directives.
c) Some places can better wrap code to 80 characters.

The specific details are:

+++ b/core/modules/actions/actions.admin.incundefined
@@ -0,0 +1,276 @@
+ * @param $options

Can we add type hinting here please?

+++ b/core/modules/actions/actions.admin.incundefined
@@ -0,0 +1,276 @@
+ * @param $action
+ *   Hash of an action ID or an integer. If it is a hash, we are
+ *   creating a new instance. If it is an integer, we are editing an existing

Again can we add type hinting here e.g 'mixed'? Also this needs to explain that the default is NULL and what happens in that case.

+++ b/core/modules/actions/actions.admin.incundefined
@@ -0,0 +1,276 @@
+ * @param $orphaned
+ *   An array of orphaned actions.
+ */

Can we add array type hinting here?

+++ b/core/modules/actions/actions.admin.incundefined
@@ -0,0 +1,276 @@
+/**
+ * Removes actions that are in the database but not supported by any enabled module.

This needs to be reworked to fit within 80 characters.

+++ b/core/modules/actions/actions.api.phpundefined
@@ -0,0 +1,98 @@
+ * @return
+ *   An associative array of action descriptions. The keys of the array
+ *   are the names of the action functions, and each corresponding value
+ *   is an associative array with the following key-value pairs:

This should be rewrapped to fit better within 80 characters as well as adding a type hint.

+++ b/core/modules/actions/actions.api.phpundefined
@@ -0,0 +1,98 @@
+ *   - 'type': The type of object this action acts upon. Core actions have types

I am pretty sure that keys of a returned associative array should not be enclosed in single quotes for D8.

+++ b/core/modules/actions/actions.api.phpundefined
@@ -0,0 +1,98 @@
+ * @param $aid

For clarity, can we add type hinting here. Is this an integer or a string like a machine name?

+++ b/core/modules/actions/actions.api.phpundefined
@@ -0,0 +1,98 @@
+/**
+ * Alters the actions declared by another module.
+ *
+ * Called by actions_list() to allow modules to alter the return values from
+ * implementations of hook_action_info().
+ *
+ * @ingroup actions

Missing @param docblock describing what $actions variable is...

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * @param $context

Please add an array type hint here.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * Processes actions_send_email_action form submissions.

Missing @return directive here explaining what in $params array.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ *   - 'recipient': E-mail message recipient. This will be passed through

Again, I believe the D8 standards do not require single quotes around these array keys.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * Constructs the settings form for actions_message_action().
+ *

I think we are missing @param array $context here.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * Action functions are declared by modules by implementing hook_action_info().

I think this explanation belongs after the one line summary and before @param line with blank line in between.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * Constructs the settings form for actions_goto_action().
+ *
+ * @see actions_goto_action_submit()

Again, I think this is missing @param array $context with an explanation of what keys are expected.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * Given the IDs of actions to perform, this function finds out what the
+ * callback functions for the actions are by querying the database. Then
+ * it calls each callback using the function call $function($context, $a1, $a2),
+ * passing the input arguments of this function (see below) to the

This should be rewrapped to better fit 80 characters per line with @param type hints added below.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * This function contrasts with actions_get_all_actions(); see the
+ * documentation of actions_get_all_actions() for an explanation.
+ *
+ * @param $reset
+ *   Reset the action info static cache.
+ *
+ * @return
+ *   An associative array keyed on action function name, with the same format
+ *   as the return value of hook_action_info(), containing all
+ *   modules' hook_action_info() return values as modified by any
+ *   hook_action_info_alter() implementations.

This needs to be rewrapped for 80 characters and type hinting added.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ *   An associative array with function names or action IDs as keys
+ *   and associative arrays with keys 'label', 'type', etc. as values.

This needs to be rewrapped for 80 characters and the full list of expected array keys specified. Also could use a type hint.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * @param $hash
+ *   Hash of a function name or action ID array key. The array key
+ *   is a key into the return value of actions_list() (array key is the action
+ *   function name) or actions_get_all_actions() (array key is the action ID).

This should be rewrapped to better fit 80 characters per line.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+  // Must be a configurable action; check database.
+  $result = db_query("SELECT aid FROM {actions} WHERE parameters <> ''")->fetchAll(PDO::FETCH_ASSOC);

This comment needs to be reworked. Could be configurable action or not defined at this point.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * @param $delete_orphans
+ *   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

This comment should start with '(optional) ' along with an explanation of what the default value is.

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * @param $function
+ *   The name of the function to be called when this action is performed.
+ * @param $type
+ *   The type of action, to describe grouping and/or context, e.g., 'node',
+ *   'user', 'comment', or 'system'.
+ * @param $params
+ *   An associative array with parameter names as keys and parameter values as
+ *   values.
+ * @param $label
+ *   A user-supplied label of this particular action, e.g., 'Send e-mail
+ *   to Jim'.
+ * @param $aid
+ *   The ID of this action. If omitted, a new action is created.
+ *
+ * @return

Type hinting here would be really helpful. Also $aid should start with '(Optional)'

+++ b/core/modules/actions/actions.moduleundefined
@@ -0,0 +1,692 @@
+ * @param $aid
+ *   The ID of the action to retrieve.
+ *
+ * @return
+ *   The appropriate action row from the database as an object.

Type hinting would be helpful here. Also an explanation of what happens in no $aid record found.

sun’s picture

Status: Needs work » Needs review
FileSize
36.46 KB
62.54 KB

Let's do those clean-ups in a separate follow-up issue. This patch is just moving code.

@Lars Toomre: Can you create a (postponed) follow-up issue and move/re-add your comment there? Thanks!

I also reverted the change from #39 (removal of $object parameter). Architectural changes to the Actions system should happen in separate issues. Again, this is about moving the Actions system into a module, only.

Reverted removal of $object parameter, restored @defgroup actions, relocated built-in actions to the bottom.

sun’s picture

FileSize
1.78 KB
62.86 KB

Meh, missed some more $entity parameters.

Reverted removals of $entity parameters from action callbacks.

sun’s picture

Also: Forgot to mention that this code lives in the platform-actions-1008166-sun branch of the Platform sandbox now, with the intention of making further re-rolls/fixes easier.

Status: Needs review » Needs work

The last submitted patch, platform.actions.53.patch, failed testing.

RobLoach’s picture

Added the issue summary, and created a follow up: #1787900: Clean up cruft in the Action documentation .

Lars Toomre’s picture

I am a bit puzzled about why cleaning up the documentation should be left to a follow up issue. Often, those documentation only patches receive less attention and are committed long afterwards (if at all).

I thought this issue was about creating an actions module. That in my mind means reviewing the code to make sure it meets both coding and documentation standards. I don't think we ever would accept such short-comings in a module being added from contrib. Am I wrong?

catch’s picture

@Lars, if we move a module from contrib to core, then it'd need to be cleaned up beforehand.

However code that's simply moving from one part of core to another, shouldn't be cleaned up in the same issue that it's moved, that makes it much harder to identify when and why changes were made. Ideally we want the code in the new module to be identical to the old code, then refactor as necessary.

One thing that does need to be done here is adding a dedicated hook_help() for the new module so it meets help standards.

sun’s picture

It's hard enough to move this code from A to B and resolving conflicts when chasing HEAD. Conflicts are especially hard to resolve when code is moved from one location to another, since any changes need to be properly re-incorporated and redone manually.

It's almost two years since the first patch was submitted. The more changes and adjustments are contained in this patch, the harder and longer it takes to get it into core.

We can happily adjust the documentation, coding style, change callback parameters, and other nice things after moving the code around. It's not a matter of attention, but instead, a matter of intentionally limiting the scope and making pragmatic progress.

A patch that cleans up the docs can be easily re-rolled and redone by almost everyone — compared to that, this patch here requires advanced developer skills to re-roll it properly.

Alas, testbot just proved that my re-roll is bogus. Will investigate what exactly went wrong, unless someone beats me to it.

sun’s picture

@catch: There's a new hook_help() already?

+++ b/core/modules/actions/actions.module
@@ -24,6 +24,89 @@
+function actions_help($path, $arg) {
...
+    case 'admin/help#actions':
sun’s picture

Status: Needs work » Needs review
FileSize
647 bytes
62.37 KB

Reverted removal of $log_entry parameter from actions_loop_test module.

Lars Toomre’s picture

Status: Needs work » Needs review

Thanks for the clarification @catch. I will wait until the move to a new module patch is committed. When it is committed, I believe the following items will be need attention as follow up:

- #1787900: Clean up cruft in the Action documentation
- #1788084: Convert actions variable(s) to CMI - add upgrade path
- #1788096: Modify callback parameters for actions_do()
- #1788104: Convert actions to plugin sub-system
- (possibly) Conversion of remaining actions to plugin system

Edit: Added newly opened issue numbers

Lars Toomre’s picture

Issue summary: View changes

fdsa

sun’s picture

FileSize
1.06 KB
62.06 KB

Changed System module update to use proper upgrade path helper functions to enable a new core module.

Status: Needs review » Needs work

The last submitted patch, platform.actions.62.patch, failed testing.

Lars Toomre’s picture

Status: Needs review » Needs work

I created three additional follow up issues for this new actions module and have updated the issue summary accordingly:

- #1788084: Convert actions variable(s) to CMI - add upgrade path
- #1788096: Modify callback parameters for actions_do()
- #1788104: Convert actions to plugin sub-system

sun’s picture

Status: Needs work » Needs review
FileSize
518 bytes
61.97 KB

Fixed manual call to update_module_add_to_system() in module update breaks upgrade path.

Lars Toomre’s picture

In keeping with the spirit of what @catch and @sun stated in #58-#59, it appears there are at least five unrelated modifications in #66 patch that are not directly part of the movement of code to a new module. It appears that all of these should be reverted and dealt with in follow-up issue(s). Specifically,

1) This change in actions_message_action_form() is not related to the move to a module:

-    '#rows' => '8',
-    '#description' => t('The message to be displayed to the current user. You may include placeholders like [node:title], [user:name], and [comment:body] to represent data that will be different each time message is sent. Not all placeholders will be available in all contexts.'),
-  );
+    '#rows' => '8',
+    '#description' => t('The message to be displayed to the current user. You may include placeholders like [user:name], [node:title] and [comment:body] to represent data that will be different each time message is sent. Not all placeholders will be available in all contexts.'),
+  );

2) In actions_message_action(), the added comment should be dealt with in a follow up issue:

- * @param array $context
- *   Array with the following elements:
- *   - 'message': The message to send. This will be passed through
- *     token_replace().
- *   - Other elements will be used as the data for token replacement in
- *     the message.
- *
- * @ingroup actions
- */
-function system_message_action(&$entity, $context = array()) {

+ * @param array $context
+ *   Array with the following elements:
+ *   - 'message': The message to send. This will be passed through
+ *     token_replace().
+ *   - Other elements will be used as the data for token replacement in
+ *     the message.
+ *
+ * Action functions are declared by modules by implementing hook_action_info().
+ * Modules can cause action functions to run by calling actions_do().
+ * @ingroup actions
+ */
+function actions_message_action($entity, $context) {

3) Change of variable $array to $action is inappropriate for a pure code move. This should be reverted and done in a follow-up issue. No?

-  foreach ($actions_map as $key => $array) {
-    if ($array['configurable']) {
-      $options[$key] = $array['label'] . '...';
-    }
-    else {
-      $unconfigurable[] = $array;
-    }
-  }

+  foreach ($actions_map as $key => $action) {
+    if ($action['configurable']) {
+      $options[$key] = $action['label'] . '...';
+    }
+    else {
+      $unconfigurable[] = $action;
+    }
+  }

4) This appears to be an inappropriate docblock change. Should be left to a follow-up issue?

-/**
- * Define the form for the actions overview page.
- *
- * @param $form_state
- *   An associative array containing the current state of the form; not used.
- * @param $options
- *   An array of configurable actions.
- * @return
- *   Form definition.
- *
- * @ingroup forms
- * @see system_actions_manage_form_submit()
- */

+/**
+ * Form constructor for the actions overview page.
+ *
+ * @param $options
+ *   An array of configurable actions.
+ *
+ * @see actions_admin_manage_form_submit()
+ * @ingroup forms
+ */

5) Documentation change unrelated to the move. Should be part of follow-up issue, no?

-/**
- * Return a form definition so the Send email action can be configured.
- *

+/**
+ * Returns a form definition to configure the 'actions_send_email_action' action.
+ *
sun’s picture

FileSize
3.19 KB
61.99 KB

Thanks! Well spotted!

At least the string change was a caused by a merge conflict and would have been an unintended regression. :)

Fixed user interface string regression in actions_message_action_form().
Fixed bogus phpDoc comment in system_message_action().
Fixed inconsistent use of $action instead of $array in actions_admin_manage().
Reverted phpDoc for actions_admin_manage_form().
Reverted phpDoc for actions_send_email_action_form().

Lars Toomre’s picture

There may well be several more inadvertent changes @sun. I was disgusted when I got to a count of five unrelated changes.

I am unsure about how to create interdiff patches, but it appears that we need a difference patch comparing your proposal in #68 vis-vis what was in #37. I strongly suspect that there are/were other changes that need to be reverted.

Lars Toomre’s picture

@sun... Looks like the last patch was not included in your sandbox code for this issue. Could you please apply it there? Thanks.

sun’s picture

Sorry, pushed.

sun’s picture

@Lars Toomre: Granted you write access to http://drupal.org/sandbox/sun/1255586 -- if you intend to work on it, checkout the actions branch with my username suffix into a new one with your username (e.g., -lars) as suffix. Further details are explained on the sandbox project page.

sun’s picture

Looks like it would be a good idea to do #1161486: Move IP blocking into its own module first, since it moves the IP blocking action from System module into a new Ban module (which conflicts with this patch).

Lars Toomre’s picture

Status: Needs review » Postponed

Postponed until the new ban module patch is committed.

In the interim, I had a chance to review this patch once more. The few issues I found include:

+++ b/core/modules/actions/actions.moduleundefined
@@ -41,7 +124,8 @@
- *   that resulted in this call to actions_do().
+ *   that resulted in this call to actions_do(). Additional parameters

This should be reversed in the move patch and added in a follow up patch.

+++ b/core/modules/actions/actions.moduleundefined
@@ -385,3 +469,265 @@ function actions_delete($aid) {
+ */
+function actions_message_action(&$entity, $context = array()) {

I realize this is being copied over from existing code, but calling &$entity as a reference appears to be a past mistake that needs to be corrected.

+++ b/core/modules/actions/actions.moduleundefined
@@ -385,3 +469,265 @@ function actions_delete($aid) {
+/**
+ * Blocks the current user's IP address.
+ *
+ * @ingroup actions
+ */
+function actions_block_ip_action() {
+  $ip = ip_address();
+  db_insert('blocked_ips')
+    ->fields(array('ip' => $ip))
+    ->execute();
+  watchdog('action', 'Banned IP address %ip', array('%ip' => $ip));
+}

As part of decoupling the actions and new ban module, shouldn't the block_ip_action be moved to the new ban module? It makes no sense to keep this action here if ban module is disabled.

Lars Toomre’s picture

Status: Postponed » Needs work

The ban module has been committed so moving this issue back to needs work status. The block_ip_action() needs to removed since that action is now part of the ban module.

sun’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
61.67 KB

Merged in HEAD.
Added renaming of former System module actions to module update.

I really wonder whether the module name shouldn't be action (singular), instead of actions (plural). It manages actions, but operates on a single action each, just like Node and Field module and others operate on a single node or field.

Lars Toomre’s picture

I agree with your thought about naming this module 'action'. I always wondered why the *.inc file was named with a plural form.

If we change the name here, we need to go through core looking at all strings with actions. As I recall, some should remain in plural form.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

+1 to leave it as Actions - it was name same as before
let's get this in

geerlingguy’s picture

I agree we should just keep the name Actions for now. If we want to change the name, that can be done in a follow-up. Otherwise, I fear this issue might stall a bit.

sun’s picture

Assigned: Unassigned » sun
Status: Reviewed & tested by the community » Needs review

This issue won't stall. The new module name is essential.

What made me raise the question is that "actions" module kinda violates the "action" module namespace in various ways. It also contains an "entity" loader that is named actions_load($aid) — but which only loads a single action.

sun’s picture

FileSize
47.39 KB
81.77 KB

Renamed actions.module to action.module.

I wasn't sure how to rename actions_do(), so I left that function alone.

The human-readable module name is still "Actions" in the UI, and the @defgroup actions is also retained. Both of these are on purpose.

Aside from that, "action" makes much more sense. It also reduces a huge ambiguity with $form['actions'] = array('#type' => 'actions');

ParisLiakos’s picture

no offense, but what happened to

Again, this is about moving the Actions system into a module, only.

That said i agree that this module's name should be action singular.

geerlingguy’s picture

We will definitely need a change notice and some more work making sure anything calling actions_* is changed to action_*, then.

sun’s picture

That's exactly the point and scope of this issue. When decoupling functionality into a new module, then choosing a proper name for that new module is an essential part of the architectural change. The actual change is what needs attention.

sun’s picture

Issue tags: +API change

x-post. (also, "Needs change notification" is only added after the fact, never before.)

Status: Needs review » Needs work

The last submitted patch, platform.actions.81.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
548 bytes
81.85 KB

Fixed {actions} table is not renamed into {action} upon upgrade from D7.

catch’s picture

Status: Needs review » Needs work

'action' is an SQL reserved word, can't be used for a table name. http://drupal.org/node/141051

sun’s picture

Status: Needs work » Needs review
FileSize
8.95 KB
78.24 KB

Fixed 'action' is a reserved SQL keyword.

ParisLiakos’s picture

Status: Needs review » Needs work

I tested the patch manually.

Created a new action for Display a message to the user action. Just entered some random text and saved.
Then applied the patch and run update.php
Old action was maintained in my list actions.
But..when i click configure i got a fatal error:

Fatal error: Call to undefined function _form() in /*/core/modules/action/action.admin.inc on line 168

Url was: admin/config/system/actions/configure/action_message_action

I created a new action for Display a message to the user after the patch and when i clicked configure URL was:

admin/config/system/actions/configure/4

Seems action id should be numeric, so i guess something goers wrong when runing update.php

Lars Toomre’s picture

Seeing those error messages in #90, I am wondering whether the path element should be 'actions' or 'action'? Is that part of renaming the module to 'action'?

ParisLiakos’s picture

ok this fixes #90:

  foreach ($map as $old => $new) {
    db_update('actions')
      ->fields(array('aid' => $new, 'callback' => $new))
      ->condition('callback', $old)
      ->execute();
  }

should be

  foreach ($map as $old => $new) {
    db_update('actions')
      ->fields(array('type' => 'action', 'callback' => $new))
      ->condition('callback', $old)
      ->execute();
  }

in system_update_8021.
Not sure if we want to change the type to action from system. But aid should be left as is.

I wont post a patch, since the one i got is very different from sun's and cant get a proper diff.
Will let that for sun since he assigned himself..I need to get more familiar with his platform sandbox.

sun’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
78.81 KB

Fixed bogus updates for action callbacks and action IDs.

ParisLiakos’s picture

Ok tested #93 extensively, everything works as supposed, upgrade path is fine.
If answer to #92 is that the path stays as is, this patch should be considered rtbc. thank you sun

sun’s picture

The menu router path for the administrative configuration of actions remains to be 'actions', since we generally use human-friendly URL paths in core.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

rtbc it is then:)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Heh, I was going to leave this for catch to review, only to discover that this started as his patch. ;)

This is clean-up consistent with other de-coupling we've done with system module and it makes about 99.9% of bootstrap requests slightly lighter, so bonus.

Committed and pushed to 8.x. Thanks!

ParisLiakos’s picture

Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Awesomeness!!
I guess we need a change notification for this describing the fact that actions functionality moved to a seperate module and what described in #83

catch’s picture

Title: Actions should be a module » Change notice for: Actions should be a module

Just updating the title. Great to see this one in.

sun’s picture

Title: Change notice for: Actions should be a module » Actions should be a module
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record
RobLoach’s picture

RobLoach’s picture

Issue summary: View changes

Added three follow-up issues to Remaining tasks section

Status: Fixed » Closed (fixed)
Issue tags: -Framework Initiative, -API change, -API clean-up

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.