actions.inc provides the core functions necessary to execute Drupal actions, but unless actions.module has been enabled, the tables for it are never created or populated. We should follow the model of path.module and menu.module, keeping the underlying functionality (like table creation and population of the actions table) out of actions.module.

The action-to-hook mapping functionality, and the administrative UI for actions themselves, should remain in actions.module.

CommentFileSizeAuthor
#24 actions_11.patch48.48 KBeaton
#20 actions_10.patch48.09 KBjvandyk
#18 actions_9.patch47.54 KBpwolanin
#15 actions_8.patch46.85 KBeaton
#10 actions_7.patch47.08 KBpwolanin
#1 actions_6.patch45.21 KBeaton
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eaton’s picture

eaton CreditAttribution: eaton commented
Status: Active » Needs work
FileSize
actions_6.patch45.21 KB

Here's the first draft of the patch.

The patch does four things:

  1. Moves the underlying actions *processing* code -- the stuff that any module that uses actions must have -- out of actions.module into actions.inc
  2. Moves the page for initializing actions and setting up advanced actions to system/build/actions in system.module.
  3. Removes old traces of the rolled-back DeleteAPI
  4. Includes the schema related fixes from http://drupal.org/node/156063

These changes mean that the underlying functionality of actions can be used anywhere in Drupal core (node administration screens, etc.) without requiring actions.module. Actions.module itself stays focused on what it's meant to be: a way of mapping actions to specific system hooks like node updates, user logins, etc.

This sets us up to 'eat our own dogfood' by using the actions API in core even if users have no need for the esoteric hook-to-action-mapping that actions.module implements.

eaton’s picture

eaton CreditAttribution: eaton commented

For reference: webchick wrote the bulk of the patch, migrating code from .module to .module to .inc. chx corrected the deleteAPI issues and the schema problems, and I polished up some of the internal bits that were still bleeding between actions.inc and actions.module. More testing needed, obviously, but I think this direction is very promising.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

There are at least some menu-system-related bits that need cleanup, for example:

+function system_actions_delete_form($form_state, $aid) {
+  if (!$aid) {
+    drupal_goto('admin/build/actions');
+  }
+  $action = actions_load($aid);

This is bad form, since the router path should instead use %actions, so this callback is never activated unless an action is loaded, and the loaded action can be passed in rather than $aid.

eaton’s picture

eaton CreditAttribution: eaton commented

That was something that existed in the original version of the module and was just moved from one portion of the system to another -- so thanks for spotting it :) I'll take a look at a couple of the other core modules and double-check how that would be done. actions_load() could be the callback maping function, couldn't it?

wundo’s picture

wundo CreditAttribution: wundo commented

subscribing

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

I'll work on this a bit and post a patch tonight

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

um - does the actions in core work at all? the schema and code seem to be inconsistent - the code is looking for a numeric aid, and the schema has varchar 255!

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Ok, as far as I can tell, this needs some substantial redesign - in a single table you are storing items that are referenced by an int ID (actions with parameters) as well what I guess are the building block actions which you look up based on the hash of the function name.

WHY!? These two things should be in separate tables. In fact there is already a second table, but it has only a serial int column. This is just bad. And why use the hash? The MD5 of the callback will have exactly the same information content as the callback itself. Is this just to hide the callback names in the URLs? And if you're going to be matching on the hash, why not store it in the table, rather than computing it every time in the query?

eaton’s picture

eaton CreditAttribution: eaton commented

Ok, as far as I can tell, this needs some substantial redesign - in a single table you are storing items that are referenced by an int ID (actions with parameters) as well what I guess are the building block actions which you look up based on the hash of the function name.

WHY!? These two things should be in separate tables. In fact there is already a second table, but it has only a serial int column. This is just bad. And why use the hash? The MD5 of the callback will have exactly the same information content as the callback itself. Is this just to hide the callback names in the URLs? And if you're going to be matching on the hash, why not store it in the table, rather than computing it every time in the query?

I agree that there could be better solutions to storing [actions] and [customized instances of configurable actions]. Honestly, though, that part of the architecture does work and has worked in the contrib version of actions.module for years. The pieces that this patch attempts to fix prevent it from working properly in many situations. Considering the fact that we are weeks past the code freeze, I don't want these critical fixes to get dragged down by architectural changes prompted mainly by aesthetics.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
actions_7.patch47.08 KB

attached patch mostly cleans up doxgen comment formatting to insure that there is a one-line function description.

Alters one menu path to use %actions, and fixes the function name for system_actions_delete_form_submit.

eaton’s picture

eaton CreditAttribution: eaton commented
Status: Needs work » Needs review

Thanks for the nice catch on %actions -- I had not seen that nice %foo == foo_load() trick for menu definitions.

This patch needs some review at this point, IMO -- I'd like to see if these specific fixes can get in to enable a richer mix of modules and better integration with other core pieces before 6 ships.

Any thoughts, folks?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

This is a *massive* change, so better get this reviewed soon by a few developers (including John Vandyk), to increase the chance of it being committed. Remember that we are asking people to port modules already to Drupal 6, so such massive changes are not really desired.

eaton’s picture

eaton CreditAttribution: eaton commented

This is a *massive* change, so better get this reviewed soon by a few developers (including John Vandyk), to increase the chance of it being committed. Remember that we are asking people to port modules already to Drupal 6, so such massive changes are not really desired.

This is very true.

Keep in mind, though, that this patch does not change developer APIs in any way. In fact it makes the developer APIs more reliable. As it stands right now, anyone who tried to call actions-related code in actions.inc without actions.module enabled would encounter possible PHP errors and other issues (a legitimate thing to attempt, given that actions.inc exists as a free-standing inc file and actions.module is not required.)

The size of this patch comes from moving several functions from one file to another, NOT from large re-writes.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

subscribe

eaton’s picture

eaton CreditAttribution: eaton commented
FileSize
actions_8.patch46.85 KB

Some whitespace changes broke one of the hunks, re-rolled.

eaton’s picture

eaton CreditAttribution: eaton commented

Bumping the issue. The code works, and fixes a number of serious issues. It's been almost a month since the last change to the patch; if there are any individuals with an interest in actions, please test and review the patch. It would be a pity to see D6 ship with this fundamental improvement broken.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Bumping again. patch still applies with offset/fuzz.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
actions_9.patch47.54 KB

ro-roll to remove offset/fuzz and to bump the system update number by 1.

Also, admin/build/actions/manage needs to be the default local task or this screen appears twice. So, changed system_menu and actions_menu to make this the case.

admin/build/actions also needs help text - especially for when the Actions module is disabled. In this patch I made it so it see the help text that's the same as admin/build/actions/manage and added another paragraph in system_help().

Testing this looks fine - actions work with actions module enabled, and are still available when it's disabled.

Dries’s picture

Dries CreditAttribution: Dries commented

I've asked John to look at this patch. I'll review it after he did. :)

jvandyk’s picture

jvandyk CreditAttribution: jvandyk commented
FileSize
actions_10.patch48.09 KB

Deletion of configurable actions was broken as the submit function was not properly renamed. Changed watchdog category from 'Actions' to 'actions'. Tables are correctly created now when Drupal is installed. Actions can be added and removed without enabling actions.module. Enabling actions.module correctly provides the additional functionality. Looks good.

Dries’s picture

Dries CreditAttribution: Dries commented

Thanks for the follow-up, John. Feel free to mark this RTBC after some additional testing.

eaton’s picture

eaton CreditAttribution: eaton commented
Status: Needs review » Reviewed & tested by the community

I took this through its paces and updated it to match the latest HEAD (the updated count needed bumping up, and one line needed to go in the newly split system-admin.inc). Tested it with several triggerable actions, and manually called actions with actions.module turned on and off. Marking RTBC.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Needs work

Could you please actually post the updated patch?

eaton’s picture

eaton CreditAttribution: eaton commented
Status: Needs work » Reviewed & tested by the community
FileSize
actions_11.patch48.48 KB

Wow. THAT was embarassing. ;)

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks all.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Fixed » Reviewed & tested by the community

does not seem to have made it into CVS.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)