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.Problem/Motivation
Prior to #2932462: Add EntityContextDefinition for the 80% use case, it was possible to create a ContextDefinition for an arbitrary entity data type, e.g. ContextDefinition::create('entity:user') or ContextDefinition::create('string'). This is significantly harder in D9, because ContextDefinition's constructor requires any entity:-prefixed data type to be using an instance of EntityContextDefinition. (Note that entity -- no colon -- is still a valid data type for ContextDefinition.)
So, if the data type is variable, there's no easy way to create a context definition for it. You have to know in advance that context definitions for a specific entity type need to use EntityContextDefinition::create(). That's both poor DX, and a problem for modules like Rules that rely extensively on variable data types. (See https://drupal.slack.com/archives/CDDD98AMN/p1586802654384900 for some more discussion about the problems this causes for Rules.)
Proposed resolution
ContextDefinition::create() should helpfully call EntityContextDefinition::create() if the data type begins with entity:. That way it can continue to be a useful factory method when the data type is not known in advance.
Remaining tasks
Review the merge request and commit it.
API changes
None.
UI changes
None.
Data model changes
None.
Release note
Not sure if we need one...?
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3126747-entitycontext-3.patch | 646 bytes | tim.plunkett |
Issue fork drupal-3126747
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
TR CreditAttribution: TR commentedComment #3
tim.plunkettYou can use
ContextDefinition::create('entity')orContextDefinition::create()for generic conditions.EntityContextDefinitionis only to replace usages when the entity type is already known.Comment #4
tim.plunkettHere is a proposal that I think captures what you described in Slack.
Comment #5
TR CreditAttribution: TR commentedComment #6
tim.plunkettEntityContextDefinition isn't what's broken per se. It's the regression to being able to pass "entity:WHATEVER" as a data type.
But, the patch above points to a larger issue (that should be opened as a follow-up):
If supporting other ContextDefinition subclasses requires changing ContextDefinition itself, contrib/custom code cannot participate.
This is already partially true in
\Drupal\Core\Annotation\ContextDefinition::getDefinitionClass(), but has an affordance for custom classes.Comment #7
TR CreditAttribution: TR commentedThe patch is enough to allow me to make Rules work in D9. I think it should also be backported to D8 so that the deprecation error isn't triggered by
ContextDefinition::create('entity:' . $datatype)and so the API remains the same in D8 and D9.Looking at the way the BC layer in
ContextDefinitionwas written in D8.6+, I don't understand why delegation was used rather than the simple fix in the the above patch. If the simple fix is good enough for D9 then I think it should be fine for D8.Yes. Although
EntityContextDefintion::__construct()should still be fixed to accept "any" and "entity" as datatypes. "any" because that is the documented default value so it should work if I use "any" or leave off the datatype. And "entity" because it wasn't clear to me thatContextDefintion::create('entity')was still valid - the documentation says "Constructing a ContextDefinition object for an entity type is deprecated" so I assumed that applied to the generic 'entity' as well. Regardless, my expectation is thatEntityContextDefintion::create('entity')andEntityContextDefintion::create()should work.Yes. Even with this change, Rules still has to do some work because we rely on extending Context, which means we will have to replicate and override all the subclasses of Context. But that's at least do-able with this patch. For the follow up, perhaps we can consider a context manager service. Or better yet see if we can move any entity-specific stuff "up" a level so we can treat all typed data the same way and eliminate the need for EntityContextDefinition.
Comment #8
tim.plunkettBecause the above patch just occurred to me when I wrote it on Monday, and it hadn't occurred to me almost 2 years ago when we worked on the original implementation.
EntityContextDefintioncannot acceptanyorentity. It must know the entity type.I think we can all agree that the docs need improving.
Comment #9
TR CreditAttribution: TR commentedOK, I was worried that I was missing something fundamental. The patch in #4 is a good and simple solution without the drawbacks and the regression of the BC layer added in D8.
It must know it as it is currently written, but if 'any' or 'entity' were passed into the constructor it could just delegate the construction to its parent class
ContextDefinition, right? The problem is that sinceEntityContextDefinitionis a subclass ofContextDefinition, it inherits the signature of the__contruct()method, so it is really obligated to properly handle any arguments that are valid according to the method signature, especially arguments that are default values. PHP does not allow us to modify the method signature on a subclass, and even if it did it would be bad OO practice to do so.I will work on adding a test for #4 over the weekend.
Comment #10
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThank you @tim.plunkett and @TR for working on this. As a contributor to Rules D8, working on many of the deprecation issues, I support your effort to get this in, then we can make proper progress on Rules.
Comment #11
Yuri CreditAttribution: Yuri commentedJust wondering if anyone is successfully using Rules for D9?
I'm using D9.0.5 and installed Rules dev, installed manually the patch #4 (since composer complained the patch could not be installed), cleared caches, but entity relationships don't appear in Actions.
When I create a new component, add a new condition 'Entity is of bundle', I see a data sector where i can select @node.node_route_context:node
And I see two other required fields 'Bundle' and 'Type' which give no supportive text how to input data, apparently direct input fields. If I enter in those node and article (also tested with [node] and [article], it does not result in the custom fields in the article content type to show up in the action:
'Add list item' or 'Set a data value' allows to set a context variable Data @node.node_route_context:node with all its standard fields, but the custom fields added are not visible.
Comment #12
TR CreditAttribution: TR commented@Yuri: Your post has nothing to do with this issue.
Comment #13
tim.plunkettNW for tests
Comment #17
phenaproximaOK, I wrote a simple test of this, based on @tim.plunkett's patch in #4. Probably could use more robust coverage, but I imagine this is technically enough to remove the "Needs tests" tag. :)
Comment #18
phenaproximaI decided to rewrite the IS to more concisely communicate the nature of the problem, as best I can tell, and the fix that @tim.plunkett's patch implements.
Comment #19
phenaproximaComment #20
phenaproximaComment #21
phenaproximaThis is definitely a bug so I'm adding it to the Bug Smash Initiative.
Comment #23
Spokje- Patch is now against
9.3.x.- Looked at the changes and they make sense to me (minimalistic testing is going to be big in 2021...)
Marking as
RTBCif TestBot agrees. (Me thinks TestBot will kick it back toNWif something fails.Comment #27
catchCommitted/pushed to 9.3.x and cherry-picked to 9.2.x and 9.1.x, thanks!