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
- #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
- #1802070: Add missing type hinting to Action module docblocks
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.
Comment | File | Size | Author |
---|---|---|---|
#93 | platform.actions.92.patch | 78.81 KB | sun |
#93 | interdiff.txt | 1.49 KB | sun |
#89 | platform.actions.89.patch | 78.24 KB | sun |
#89 | interdiff.txt | 8.95 KB | sun |
#87 | platform.actions.87.patch | 81.85 KB | sun |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedSubscribe - this looks ++ good
Comment #3
andypostAgree 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
Comment #4
catchI 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.
Comment #5
catchCompletely 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.
Comment #6
catchComment #7
sun+1
Comment #8
Jody LynnMissing $
Wrong function name
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.
Comment #9
Jody LynnComment #10
catchFixes the points raised by Jody Lynn, thanks! Actions module now installs via the UI for me.
Comment #11
Jody LynnI got the actions UI working.
Still needs some things moved out of system.api.php and system.install as well
Comment #12
catchRe-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.
Comment #13
Jody LynnAlso 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.
Comment #14
9byte1 CreditAttribution: 9byte1 commentedShould actions module be visible/disable-able?
Comment #15
Jody LynnDefinitely. (Can't wait to disable it... All it's every done for me is make me worry about its orphans)
Comment #16
Jody LynnWe 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.
Comment #17
sunActually... the module and function names should be singular, like most/all other core modules. The "entity" is an action.
Comment #18
catchsystem_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.
Comment #19
catchWell it would move them if I'd done git add.
Comment #20
catch.
Comment #21
cweagansNow that the git migration is done, we need to remove all $Id$ tags from incoming patches.
Will try to actually review the code later.
Comment #22
catchRe-rolling minus the $Id tags, didn't run tests or anything but putting past the bot to see how bad it is.
Comment #24
catchCruft in me patch, this should at least apply.
Comment #26
catchFixed the syntax error and also two constants s/WATCHDOG/LOG/g
Comment #28
catchSorry..
Comment #30
cweagansThis 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.
Comment #31
catchIt 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).
Comment #32
sunTrigger is not to be discussed here: #764558: Remove Trigger module from core
Comment #33
RobLoachSign me up.
Comment #34
RobLoachWould love for this to move forward. Is this the right idea for the update?
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.
Comment #35
sunI think that's a good first step. As soon as the plugin system lands, actions should totally be plugins ;)
Will try to review later.
Comment #36
RobLoach#34: 1008166-diff.patch queued for re-testing.
Comment #37
sunRegular diff against HEAD.
Comment #38
andypostphp-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
[user:name] should be first in list, do we have difference between user: and account: tokens?
so this will only use current logged-in user as token and 'node' somehow
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedI 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
Comment #40
ParisLiakos CreditAttribution: ParisLiakos commentedmissed
Comment #41
andypostNeeds better name then $array, suppose $action is more sane
Defines
@return should have blank line before. see http://drupal.org/node/1354#functions
Processes
Form builder functions should follow http://drupal.org/node/1354#forms
same
Validates
Processes
Returns
Comment #42
ParisLiakos CreditAttribution: ParisLiakos commentedI hope i did not miss anything:)
Comment #44
andypostPatch seems rtbc!
Suppose it should be commited and actions could be converted to plugins in follow up issue
Comment #45
RobLoachWas missing a newline at the top of actions.admin.inc. Rolled with -M25%. Not really required, but might help patch size.
Comment #46
dagmarQuestion. Should this be converted into the new Configuration System?
Comment #47
RobLoachIn a separate issue, sure! Mind creating an issue about converting Actions over to config so we can track it? :-)
Comment #48
andypostLet's get this in! Do we need another review?
Comment #49
RobLoach#45: 1008166-actions.patch queued for re-testing.
Comment #51
Lars Toomre CreditAttribution: Lars Toomre commentedI 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:
Can we add type hinting here please?
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.
Can we add array type hinting here?
This needs to be reworked to fit within 80 characters.
This should be rewrapped to fit better within 80 characters as well as adding a type hint.
I am pretty sure that keys of a returned associative array should not be enclosed in single quotes for D8.
For clarity, can we add type hinting here. Is this an integer or a string like a machine name?
Missing @param docblock describing what $actions variable is...
Please add an array type hint here.
Missing @return directive here explaining what in $params array.
Again, I believe the D8 standards do not require single quotes around these array keys.
I think we are missing @param array $context here.
I think this explanation belongs after the one line summary and before @param line with blank line in between.
Again, I think this is missing @param array $context with an explanation of what keys are expected.
This should be rewrapped to better fit 80 characters per line with @param type hints added below.
This needs to be rewrapped for 80 characters and type hinting added.
This needs to be rewrapped for 80 characters and the full list of expected array keys specified. Also could use a type hint.
This should be rewrapped to better fit 80 characters per line.
This comment needs to be reworked. Could be configurable action or not defined at this point.
This comment should start with '(optional) ' along with an explanation of what the default value is.
Type hinting here would be really helpful. Also $aid should start with '(Optional)'
Type hinting would be helpful here. Also an explanation of what happens in no $aid record found.
Comment #52
sunLet'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.
Comment #53
sunMeh, missed some more $entity parameters.
Reverted removals of $entity parameters from action callbacks.
Comment #54
sunAlso: 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.
Comment #56
RobLoachAdded the issue summary, and created a follow up: #1787900: Clean up cruft in the Action documentation .
Comment #57
Lars Toomre CreditAttribution: Lars Toomre commentedI 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?
Comment #58
catch@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.
Comment #59
sunIt'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.
Comment #60
sun@catch: There's a new hook_help() already?
Comment #61
sunReverted removal of $log_entry parameter from actions_loop_test module.
Comment #62
Lars Toomre CreditAttribution: Lars Toomre commentedThanks 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
Comment #62.0
Lars Toomre CreditAttribution: Lars Toomre commentedfdsa
Comment #63
sunChanged System module update to use proper upgrade path helper functions to enable a new core module.
Comment #65
Lars Toomre CreditAttribution: Lars Toomre commentedI 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
Comment #66
sunFixed manual call to update_module_add_to_system() in module update breaks upgrade path.
Comment #67
Lars Toomre CreditAttribution: Lars Toomre commentedIn 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:
2) In actions_message_action(), the added comment should be dealt with in a follow up issue:
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?
4) This appears to be an inappropriate docblock change. Should be left to a follow-up issue?
5) Documentation change unrelated to the move. Should be part of follow-up issue, no?
Comment #68
sunThanks! 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().
Comment #69
Lars Toomre CreditAttribution: Lars Toomre commentedThere 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.
Comment #70
Lars Toomre CreditAttribution: Lars Toomre commented@sun... Looks like the last patch was not included in your sandbox code for this issue. Could you please apply it there? Thanks.
Comment #71
sunSorry, pushed.
Comment #72
sun@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.
Comment #73
sunLooks 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).
Comment #74
Lars Toomre CreditAttribution: Lars Toomre commentedPostponed 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:
This should be reversed in the move patch and added in a follow up patch.
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.
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.
Comment #75
Lars Toomre CreditAttribution: Lars Toomre commentedThe 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.
Comment #76
sunMerged 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.
Comment #77
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #78
andypost+1 to leave it as Actions - it was name same as before
let's get this in
Comment #79
geerlingguy CreditAttribution: geerlingguy commentedI 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.
Comment #80
sunThis 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.Comment #81
sunRenamed 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');
Comment #82
ParisLiakos CreditAttribution: ParisLiakos commentedno offense, but what happened to
That said i agree that this module's name should be action singular.
Comment #83
geerlingguy CreditAttribution: geerlingguy commentedWe will definitely need a change notice and some more work making sure anything calling
actions_*
is changed toaction_*
, then.Comment #84
sunThat'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.
Comment #85
sunx-post. (also, "Needs change notification" is only added after the fact, never before.)
Comment #87
sunFixed {actions} table is not renamed into {action} upon upgrade from D7.
Comment #88
catch'action' is an SQL reserved word, can't be used for a table name. http://drupal.org/node/141051
Comment #89
sunFixed 'action' is a reserved SQL keyword.
Comment #90
ParisLiakos CreditAttribution: ParisLiakos commentedI 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
Comment #91
Lars Toomre CreditAttribution: Lars Toomre commentedSeeing 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'?
Comment #92
ParisLiakos CreditAttribution: ParisLiakos commentedok this fixes #90:
should be
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.
Comment #93
sunFixed bogus updates for action callbacks and action IDs.
Comment #94
ParisLiakos CreditAttribution: ParisLiakos commentedOk 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
Comment #95
sunThe menu router path for the administrative configuration of actions remains to be 'actions', since we generally use human-friendly URL paths in core.
Comment #96
ParisLiakos CreditAttribution: ParisLiakos commentedrtbc it is then:)
Comment #97
webchickHeh, 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!
Comment #98
ParisLiakos CreditAttribution: ParisLiakos commentedAwesomeness!!
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
Comment #99
catchJust updating the title. Great to see this one in.
Comment #100
sunChange notice: http://drupal.org/node/1796904
Comment #101
RobLoachBug: #1797206: Actions module has variable name conflict which breaks tests
Comment #101.0
RobLoachAdded three follow-up issues to Remaining tasks section
Comment #102.0
(not verified) CreditAttribution: commentedUpdated issue summary.