Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#24 | actions_11.patch | 48.48 KB | eaton |
#20 | actions_10.patch | 48.09 KB | jvandyk |
#18 | actions_9.patch | 47.54 KB | pwolanin |
#15 | actions_8.patch | 46.85 KB | eaton |
#10 | actions_7.patch | 47.08 KB | pwolanin |
Comments
Comment #1
eaton CreditAttribution: eaton commentedHere's the first draft of the patch.
The patch does four things:
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.
Comment #2
eaton CreditAttribution: eaton commentedFor 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.
Comment #3
pwolanin CreditAttribution: pwolanin commentedThere are at least some menu-system-related bits that need cleanup, for example:
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.
Comment #4
eaton CreditAttribution: eaton commentedThat 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?
Comment #5
wundo CreditAttribution: wundo commentedsubscribing
Comment #6
pwolanin CreditAttribution: pwolanin commentedI'll work on this a bit and post a patch tonight
Comment #7
pwolanin CreditAttribution: pwolanin commentedum - 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!
Comment #8
pwolanin CreditAttribution: pwolanin commentedOk, 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?
Comment #9
eaton CreditAttribution: eaton commentedI 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.
Comment #10
pwolanin CreditAttribution: pwolanin commentedattached 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.
Comment #11
eaton CreditAttribution: eaton commentedThanks 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?
Comment #12
Gábor HojtsyThis 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.
Comment #13
eaton CreditAttribution: eaton commentedThis 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.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe
Comment #15
eaton CreditAttribution: eaton commentedSome whitespace changes broke one of the hunks, re-rolled.
Comment #16
eaton CreditAttribution: eaton commentedBumping 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.
Comment #17
pwolanin CreditAttribution: pwolanin commentedBumping again. patch still applies with offset/fuzz.
Comment #18
pwolanin CreditAttribution: pwolanin commentedro-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.
Comment #19
Dries CreditAttribution: Dries commentedI've asked John to look at this patch. I'll review it after he did. :)
Comment #20
jvandyk CreditAttribution: jvandyk commentedDeletion 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.
Comment #21
Dries CreditAttribution: Dries commentedThanks for the follow-up, John. Feel free to mark this RTBC after some additional testing.
Comment #22
eaton CreditAttribution: eaton commentedI 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.
Comment #23
Gábor HojtsyCould you please actually post the updated patch?
Comment #24
eaton CreditAttribution: eaton commentedWow. THAT was embarassing. ;)
Comment #25
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks all.
Comment #26
pwolanin CreditAttribution: pwolanin commenteddoes not seem to have made it into CVS.
Comment #27
Gábor HojtsyCommitted, thanks.
Comment #28
(not verified) CreditAttribution: commented