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.
This is a spinoff from #1008166-35: Actions should be a module in which @sun stated
As soon as the plugin system lands, actions should totally be plugins ;)
The new actions module soon will be committed and the plugin system has landed. Hence, it is time to convert the first action to use the plugin sub-system.
The purpose of this issue is to convert one action defined in the new actions module (as an example) and to perform a complete review before the others are potentially converted to use plugins as well.
Comment | File | Size | Author |
---|---|---|---|
#5 | 1788104-5-actions_as_plugins.patch | 42.9 KB | kotnik |
#4 | 1788104-4-actions_as_plugins.patch | 44.04 KB | kotnik |
Comments
Comment #1
larowlanTagging
Comment #2
sunComment #3
andypost@sun how actions-plugins could relate to entities when they are related to?
What kind of manager we need to manage list of actions and their relation to entities?
Suppose this will help to progress on #1823574: [Meta] Improve the Views Bulk Operations (VBOs) that are in core
Comment #4
kotnik CreditAttribution: kotnik commentedThe attached patch is to get the party started, to set the initial structure for plugins, and an example how action plugins should be implemented.
This is incomplete patch, and more eyes are welcome to provide direction. Please do comment.
Comment #5
kotnik CreditAttribution: kotnik commentedSame as in #4, but somewhat polished patch.
Comment #6
sunThis looks like an awesome start, @kotnik!
Btw, I've seen that you marked the issue "needs work" and indeed, the patch looks more like a non-functional prototype thus far. In such cases, make sure to suffix your patch file with "do-not-test.patch"; i.e.,
whatever.blub-blah.blub.do-not-test.patch
:)The static variable cache can be removed here - the plugin manager retrieves definitions only once already, unless I'm mistaken.
I think this can be easily kept: Just store the plugin ID instead of $callback?
Shouldn't $this->configuration be set only once in the constructor? (I don't know what our current practice with regard to configurable plugins is)
Would it make sense to rename these to:
getForm()
validateForm()
submitForm()
?
(We should leave room for a non-form-related ::validate() method.)
I've seen you started to convert the entity-related actions into plugins first.
That's an interesting aspect, actually. Quite a couple of those entity-specific plugins don't actually have to be entity-specific.
I wonder whether it would make sense to split all actions into entity-based and entity-unrelated actions?
That, in turn, would allow us to potentially have two separate interfaces; i.e., EntityActionInterface and GenericActionInterface (or similar).
Whereas the EntityActionInterface could take much more focused parameters in its ::execute() method; i.e.:
I'm not sure whether this idea is possible to do directly for this issue or whether it should be a follow-up — what do you think?
Aren't the hook_action_info() implementations obsolete with this patch already? If so, let's remove them entirely.
Comment #7
kotnik CreditAttribution: kotnik commented@sun: thanks for in-depth review, I am going further with this patch.
In the mean time, updating issue title to reflect the actual work inside it.
Comment #8
fagoI think the proper way for that to work is to follow the Rules-way: Support any number of parameters for actions, which are described in the action info. We have the foundation for describing the parameters in core now: the typed data API.
I'd love to go for re-done actions API that also fits the needs of Rules for that. I prepare a summary of how I see that working.
Comment #9
bojanz CreditAttribution: bojanz commentedEDIT: Nevermind.
Comment #10
andypostShow message, send mail, run cron - very useful actions.
Comment #11
fagofinally wrote it down, see #1846172: Replace the actions API
Comment #12
damiankloip CreditAttribution: damiankloip commentedAlso see the above issue fago mentions, for the sandbox we have started. Which ultimately has actions as plugins anyway already.
Comment #13
Gábor HojtsyI'm afraid we might be falling between two stools. Actions are not plugins, this issue was was stopped being worked on I guess in favor of #1846172: Replace the actions API, while that has also not been progressing lately. What we have on the other hand is very custom DB storage for very custom code and actions neither using plugins, nor config entities (both of which they should). I'm afraid if we are shooting too high and not reaching those goals, then we'll just not move actions forward in any way. That would mean actions would not have any use in core, and could just as well be removed.
(The D8MI perspective is actions in core like dsm or send email should be translatable but there is no way now since they are stored in a blob field serialized, and no general config entity system is used so we can affect this in any way).
Comment #14
fago#1743686: Condition Plugin System got fixed, so with the needed API in place already. What needs to be done for #1846172: Replace the actions API is basically doing a base class for actions + interface just as for conditions and converting existing actions over.
Comment #15
tim.plunkettI made a ton of progress on #1846172: Replace the actions API (still yet to post my writeup and code...)
But to prevent any more duplicate work this coming week, I'm closing this one.