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.
Problems
- Having a single parameter for actions is not sufficient, nor is it possible to add further metadata to the single parameter (e.g. to define whether it is required)
- We need to support actions without an entity paramter.
- Actions should follow the same API as conditions if there are no reasons to do it different.
- The action subsystem is coupled to entities.
- Finally, this is required if the action system should be useful to Rules as well.
Proposed resolution
Make the action sub-system using context, such as conditions.
Remaining tasks
Patch review.
User interface changes
-
API changes
API changes in this issue are approved by the comments on ActionInterface.php.
- The 'type' parameter of action annotations is replaced by a proper context definition.
- The execute() method on ActionConfigEntityInterface is removed.
- The executeMultiple() method signature on ActionInterface is changed to additionally take a context name argument as first parameter.
Original report by tim.plunkett
See #1846172: Replace the actions API, it currently passes entities around directly, and uses hardcoded type = node
Comment | File | Size | Author |
---|---|---|---|
#110 | 2011038-110.patch | 46.42 KB | ravi.shankar |
#106 | interdiff.2011038.105-106.txt | 8 KB | aleevas |
#106 | 2011038-9.1-106.patch | 46.7 KB | aleevas |
#81 | 2011038-81.patch | 58.66 KB | MerryHamster |
#78 | 2011038-78.patch | 70.43 KB | benjy |
Comments
Comment #1
jibranComment #2
fagoComment #3
tim.plunkettLet's explain why this is major
Comment #4
tim.plunkettComment #5
fagoAdded issue summary.
Comment #6
fagoComment #7
fagoComment #8
klausiklausi opened a new pull request for this issue.
Comment #9
klausiStarting a WIP patch here. Trying to convert EmailAction to context values.
I could not find the test cases for this plugin, so let's see where the testbot fails.
I'm removing configuration forms in this patch, because action parameters should use the context system. Not sure how this will look in the UI, but I guess the caller should be responsible for building a UI from the context definitions. Or should we keep the configuration form and populate the context values from the config?
We are already using context aware action plugins in Rules by abusing the core action system as it fits our needs. At some point this will break (as the plugins are not compatible) and we will have to fork the core action system as Rules action system, but for now we can try to move the ideas from Rules into core.
Comment #11
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #13
andypostCode looks good step forward, only views bulk ops are needs some fixes
provider is used all over plugins, maybe extension is better...
do profiles are able to expose action plugins?
Comment #14
fagoI do not think we can just do away with that?
Also we'll need some test coverage for a contextual action.
Comment #15
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #17
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #19
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #21
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #22
klausiNow including an ActionTestBase class using PHPUnit to also test the contents of the action plugin annotation and the plugin instantiation. Also inject ContextHandler properly from the service container.
Comment #24
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #25
andypostjust a few nitpicks
needs issue #
should be provider, suppose install profiles and probably themes could provide action plugns
Comment #26
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #28
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #30
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #31
klausiTests passing again and I also converted the remaining action plugins in action module, which seem to have no test coverage. I also addressed the points mentioned by andypost and we also have test coverage for contextual actions now as pointed out by fago.
There are a couple of @todos in the code left what we should do with context vs. configuration. There are now a few actions that can get their parameters either from context or from configuration values; but I think this patch is already large enough, so we should not address that also in this issue.
Comment #32
fagoThanks. Great to seeing the improved test-base from Rules being added also. Here some remarks:
Could we just add second parameter taking the context name for which the action should be multiple-executed?
executeMultiple('user', $users)
Then action implementations need an easy way to provide optimized methods for certain context parameters. But yeah, let's figure out details in a follow-up here.
Not fully true as context definitions use a slightly different format. Isn't enough to point to context definitions for docs?
This seems wrong. it's $contexts, right?
What's the get() after getLabel() for?
Ouch. I think we'll have to clean this up before commit, i.e. only use contextual values. We can keep the configuration form, but use the configuration to pre-fill context in those cases maybe? I'd do move all configuration processing to a single method that's invoked in the beginning of execute() and writes configuration values into the context - so the rest can be clean-ed up then.
This should go in the configuration processing code also then.
I do no think this action needs an entity, does it?
Comment #33
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #34
klausiI think I addressed almost all comments from fago.
We need the entity context for those actions because they rely on an entity for token replacements.
Comment #36
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #39
fagohm, an interdiff would be nice as there is no way to review intermediate commits with dreditor / posting comments to d.o. Anyway, here a review:
Generically, this makes no sense. There may be no reference of entity, also objects should not be used either (what's that?). I'd suggest using the context name as first parameter also, as this is the logical ordering. Suggestion:
;
Ouch, having that still hurts. But well, no other way to do it until we've dedicated token replacement / data processing support.
as it's called configuration else where here, it should be configuration here also imho.
Should be of type "uri" ?
Should use the same signature as on the action interface, but moreover - do we really need it here? One can just use the plugin instead?
Too much indentation.
Comment #40
fagoAlso we should update the summary with all concrete API changes.
Comment #41
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #42
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #43
klausiImplemented remarks from fago, interdiff attached.
We cannot use "url" as data type for the GotoAction because an internal path such as "node/1" might not register as valid URL, so I left it simply as string.
Comment #44
klausiGnah, of course the interdiff upload got lost.
Comment #45
fagoThanks, this looks already good. I did another close review:
The exception is only thrown for required contexts what means we have to check for non-NULL values else. But maybe best we could just add a parameter to getContextValue() to determine whether it it should throw exceptions? $fail_on_error ?
Should be multiple strings instead.
Configuration value processing should be done on form submit as before.
@summary:
This sounds quite arbitarary, I guess some short reasoning would be good to have there as well ($entity parameter gone?).
Also this needs a change record draft.
Comment #46
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #47
klausiFixed fago's remarks, interdiff attached.
EclipseGc mentioned in IRC that there’s a method on ContextHandler for this configuration to context mapping, so we could look into that as well.
Comment #48
EclipseGc CreditAttribution: EclipseGc commentedThe ContextHandler class has a helper for this. You shouldn't need this method.
I ran into the same issue with #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types. I fixed it in the Translation Annotation since it's that's class' job to make it's return usable.
don't we have an email address data type?
I think we also have a url data type.
This seems really weird. Why are we setting the node key to $entity when we clearly expect generic entities?
Doing all of this in the protected mapConfigurationToContext method will have to change as you move to using the ContextHandler.
I think this needs a try/catch cause getContextValue() throws an exception if context is not set and is required. Worth pointing out you could set that the "user" context is not required in the annotation. I think Fago mentioned something similar with regard to a parameter to NOT throw the exception. If you don't want the exception, then this is by definition an optional context, and the ContextDefinition annotation and class support, that, please use that instead of adding some switch parameters to the generic ContextAware Plugin classes.
I've seen this a couple times now in a couple of different permutations and didn't immediately pick up on it. This looks like it's probably Fago's "configuration context" thing that he wants. It's fine that he wants this, and I support doing it, but we should probably come up with a separate key and not conflate these and regular execution contexts together.
In general, the changes in this patch make a ton of sense, but it still needs a little work. This is a review of 42 since 46 was published while I was doing this. Looking at the interdiff, I don't see anything that invalidates any of my criticisms.
Eclipse
Comment #49
EclipseGc CreditAttribution: EclipseGc commentedOh, also I would expect you don't have enough test coverage for 7 if things aren't blowing up.
Eclipse
Comment #50
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #51
klausiThanks for the review!
1. ContextHandler::applyContextMapping() does not make sense here since we would have to build up artificial context objects just to invoke this method. That would just bloat the code for this easy task?
2. In #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types the Translation Annotation is not modified, you also fixed it in the ContextDefinition Annotation. Copied the code from there.
3. Right, since the token replacement is now only done on the configuration, we can make this an actual email data type. Fixed.
4. We cannot use the url data type for the GotoAction because it can be an internal path like "node/1", which is not a valid URL. Leaving as string.
5. Yep, using entity when we actually only mean nodes seems wrong. Changed it to node.
6. Not using ContextHandler, so nothing to change here.
7. The user context is required, because we have to have a user to be set as owner of the node. We can't do without user, right? So we just expect that it is always there. If there are exceptions it is the callers fault. We are not adding any switch parameters to the ContextAwarePluginBase either here.
8. There is no difference between configuration context and runtime context. All parameters the plugin needs for the execution should be in context objects. So we should not invent any new keys here, since everything is just context to the plugin.
Adding test coverage for the AssignOwnerNode action plugin is out of scope for this issue, we should open a new one if there is work remaining after this.
Comment #52
fago.
Yep, we need it for Rules.
I agree that there needs some information on what's the default intention - i.e. should it be configured or passed on the fly. Maybe we can look into adding some additional key to the context definition or so in a follow up?
Comment #53
EclipseGc CreditAttribution: EclipseGc commented@Fago
I'd prefer it if were were to just introduce a separate key in the plugin definition for this instead of adding it to the context definition since run-time context vs configuration isn't a distinction the ContextDefinition objects should care about. This is all about how you're using it.
@Klausi
1.) There's an expected configuration key of context_mapping that maps the name of the context from the context array to be used. Gathering the contexts array is the job of the system which will be executing the actions. Passing the contexts array that is gathered by that system plus the plugin & it's configuration to the ContextHandler is the appropriate way forward here, unless I misunderstand what you're doing very badly.
2.) My bad, misspoke.
4.) :-(
5.) I read that is being able to be any entity. I was suggesting you change the array key to 'entity'. If this actually only uses nodes, then ++ to your change.
6.) See 1
7.) No, you clearly do an "if ($user)" check after that and if it's not there you fallback to a configuration value. That is absolutely an "optional" context.
8.) See my response to fago.
Eclipse
Comment #54
klausiI agree, but we are far from that in core, that's why the action plugins have to supply themselves with context that is stored in their configuration for now.
A complete implementation for the BulkForm example use case would require us to pull out stored context configuration from the plugin, turn it into a context array, pass that on to ContextHandler, create a context mapping. I think this is out of scope for this issue. We can't do everything at once. Let's continue with incremental steps since you said yourself that this is a large improvement already. Ready for RTBC?
Comment #55
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #56
klausiThat was just a reroll.
Comment #58
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #59
klausiDRUPAL_ROOT does not exist anymore in PHPUnit tests, switched to $this->root which also required a slight change of order when registering modules/namespaces. Same code as used in Rules 8.x-3.x.
Comment #60
fagoImo, context vs configuration isn't a distinction the plugin should have to differentiate upon - the plugin should rely on getting it's context set from outside and be able to run from there. So all we need is a way for having defaults for generating forms for pre-configuring context values on configuration time or mapping contexts, i.e. the configuration form. But, it doesn't really make much sense to me to provide another API for getting configuration values vs context values when it's all about getting a value the plugin needs for execution, i.e. it's context (mapped + pre-configured).
But anyway, I experimented with separate context + configuration definition buckets and figured that it's end up being problematic with ordering. Given there are two different buckets for configuration and context we loose the native ordering, e.g. on the configuration form. And yes, Rules has use-cases for optional (by default contextually mapped) arguments being less important and at the end of the form, what means we'd have to introduce some weights-fu for getting back a common ordering. Thus, having it in bucket just makes our lives a lot easier here.
Related to the ordering, but not directly related to this issue, I still think about experimenting with base classes providing better DX and auto-completion, e.g. by supporting methods like
executeWithContext(EntityInterface $entity, array $some_array)
(arbitrary example) - for which we need the common ordering as well.Comment #61
EclipseGc CreditAttribution: EclipseGc commentedThis seems woefully insufficient for the way Actions handle contexts in both config and traditional contexts, furthermore, I don't think this will properly execute for an action that genuinely needs multiple contexts. The various permutations simply aren't happening here.
Totally unnecessary. The ContextHandler class has methods for doing this sort of stuff already. Please us that.
This is a classic "optional context". You don't need this custom logic in the mapper, just use the ContextHandler and you can set sane fallbacks in the plugin itself.
Perfect example of what I was saying about executeMultiple earlier. If you have a list of nodes and users, you have no way to actually set this for all of them reliably. You could set all nodes to a particular user, or assign a different owner to the same node over and over. Other options are limited.
Also, looking at your execute() logic, user should be an optional context.
I didn't get to give this all the attention it deserved, but there's a lot still wrong with this patch. We should not have competing Contextual Plugin approaches in core. Conditions does much of this same stuff very well, and #2377757: Expose Block Context mapping in the UI is the next patch I'm working on for blocks. Let's try to have a unified approach here.
Eclipse
Comment #62
EclipseGc CreditAttribution: EclipseGc commentedsorry :-(
Comment #63
klausi2 actionable items identified by Eclipse in IRC:
Comment #64
EclipseGc CreditAttribution: EclipseGc commentedDoesn't this prevent you from being able to effectively executeMultiple() and change the node you want to extract tokens from?
Comment #65
kim.pepperComment #66
klausiTried to look at this again today and how we could use ContextHandler in the BulkForm use case, but it all does not make sense to me. I'm sorry, I have to give up at this point. Feel free to pick this up and walk it to completion!
Comment #67
fagoIf there is no good way to use the context mapping in vbo, why bother with it? Main goal must be to have context-aware action plugins such that they are re-usable and can be a base for Rules. Having better VBO integration shouldn't block this!
Comment #68
EclipseGc CreditAttribution: EclipseGc commentedI'll try to give this some time in the next couple days.
Comment #69
klausiNow that #2172017: Bulk operations does not respect entity access has landed it becomes more and more clear that core action plugins are quite different than Rules action plugins. The function signature of the access() method with $object does not make sense for Rules.
Core focuses on one "object" the action is about (usually an entity), while Rules actions can have multiple objects as context that are equally important. Core actions have the concept of configuration with them, while in Rules an action never has configuration. In Rules everything the action plugin receives from the outside is handled with the Context API, which is populated when the action plugin is executed. No configuration is stored with the action plugin, all mapping configuration is stored outside in Rules config entities.
So IMO it is time to go separate ways with action plugins now and invent @RulesAction plugins in contrib. This will be a bit annoying for other contrib authors since they will have to implement @Action and @RulesAction plugins for their modules, but I think we cannot avoid that with the different goals that we have.
Comment #70
daffie CreditAttribution: daffie commented@klausi: Is there some way we can change @Action plugins so that there is no need for a seperate @RulesAction plugin.
Comment #71
fagoNow that #2172017: Bulk operations does not respect entity access went in, we'll need to fix that to work context based as well. We'll need the same for conditions though, so maybe should work on adding it to conditions first and contextualize the API that and then pick up the same solution here?
Comment #72
jhedstromFeature request => 8.1.x.
Comment #73
fagoI took a closer look at this also and tried to come up with a good solution leveraging the core context handler. However, this and the multiple execution feature of actions makes it really hard to do so - I've not been able to come up with a good way of doing this without mixing up the preparing execution environment step and multiple execution. However, the preparation is the job of the caller, the multiple execution the job of the plugin...
I'm not sure how this multiple execution feature can work without coupling the plugin with knowledge of how its context gets set (That said, there is already getContextMapping() on ContextAwarePluginInterface what seems wrong to me also).
That said, it looks like this has to be postponed indeed and Rules will have to do its own context aware actions API :-/ Maybe we are able to figure a solution here once the Rules context configuration API has matured and it's easier to see the big picture of how context & configuration work together.
Comment #74
fagoComment #75
fagoFor record or later, here is my WIP patch based upon klausi's latest one. It's WIP and has some left-over merge issues, but should show where I was headed.
Comment #77
andypostComment #78
benjy CreditAttribution: benjy at PreviousNext commentedStraight re-roll, I think there is still quite a bit todo here, I'm not sure why the work dropped off, maybe we were waiting on something else to happen in core first?
@fago, if you want to discuss, ping me on IRC, happy to help with this.
Comment #81
MerryHamster CreditAttribution: MerryHamster at Skilld for Skilld commentedRerolling patch
Comment #82
tim.plunkettLet's see what the testbot thinks.
Comment #86
basil_snowman CreditAttribution: basil_snowman as a volunteer and for Skilld commentedComment #87
andypostrun bot
Comment #88
basil_snowman CreditAttribution: basil_snowman as a volunteer and for Skilld commentedComment #91
basil_snowman CreditAttribution: basil_snowman as a volunteer and for Skilld commentedComment #93
MerryHamster CreditAttribution: MerryHamster at Skilld for Skilld commentedFixed bugs in patch from #91 commit
Comment #94
andypostComment #99
andypostComment #100
vacho CreditAttribution: vacho at Skilld commentedpatch for comment #93 rerolled
Comment #101
vacho CreditAttribution: vacho at Skilld commentedComment #104
andypostComment #105
aleevasTrying to re-rorll the latest patch
Comment #106
aleevasTried to fix failed test
Comment #108
jibranThe patch needs a reroll as it is still using
drupal_set_message
.This is a public method we can't straight away change this without deprecation warning.
All these @todos at least need an issue to properly deprecate config option.
Comment #109
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #110
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch of #106.
Mistakenly added blank comment #109.
Comment #111
TR CreditAttribution: TR commentedAlso, see change record: Plugins now use the 'context_definitions' key to define their contexts
Comment #115
andypostMeantime core enabling PHP attributes, so actions should get new contexts #3252386: Use PHP attributes instead of doctrine annotations
Comment #116
quietone CreditAttribution: quietone at PreviousNext commentedAction module is approved for removal. See #3266458: [Policy] Deprecate Action (UI) module in D10 and move to contrib in D11
This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.
It will be moved to the contributed Action project when the Drupal 11 branch is open.
Comment #117
dpi@quietone is this issue affected? its just actions UI due for removal?
Comment #118
quietone CreditAttribution: quietone at PreviousNext commentedOops, you are right. I was on 'automatic'. Feel free to change the status on any others I did incorrectly. I think I'll take a break and then come back to this task.
Comment #120
quietone CreditAttribution: quietone at PreviousNext commentedMoving Non UI issue to the base system