The data type compatibility checking makes a simple equal comparison with special case of 'any' and 'entity' types. There is a @todo regarding of this at src/Context/ContextHandlerIntegrityTrait.php:

...
    132   protected function checkDataTypeCompatible(CoreContextDefinitionInterface $context_definition, DataDefinitionInterface $provided, $context_name, IntegrityViolationList $violation_list
    132 ) {
    133     // Compare data types. For now, fail if they are not equal.
    134     // @todo: Add support for matching based upon type-inheritance.
    135     $target_type = $context_definition->getDataDefinition()->getDataType();
...

As far I can see, there is not an open issue for it.

CommentFileSizeAuthor
#10 rules-data-type-inheritance-checking-3022250-10.patch863 byteslukus
#9 rules-data-type-inheritance-checking-3022250-9.patch863 byteslukus
#7 rules-3022250-7.patch1.5 KBakalam
#2 rules-inheritance_data_type_compatibility_checking-2.patch1.42 KBmanuel.adan
#5 rules-inheritance_data_type_compatibility_checking-3022250-5.patch1.33 KBmanuel.adan
#5 interdiff-3022250-2-5.txt656 bytesmanuel.adan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

manuel.adan created an issue. See original summary.

manuel.adan’s picture

manuel.adan
Location 🌌
CreditAttribution: manuel.adan at Arcovía Digital for Asociación TecnoAriadna commented
Assigned: manuel.adan » Unassigned
Status: Active » Needs review
FileSize
rules-inheritance_data_type_compatibility_checking-2.patch1.42 KB

This is not a full solution, it improves the compatibility checking between entity type and sub-types (bundles). A reference to this issue also was added.

TR’s picture

TR CreditAttribution: TR commented
Issue tags: +Needs tests

Need test cases for your changes added to Unit/Integration/Engine/IntegrityCheckTest.php

Can you give an example of where this patch helps? It's not clear to me that this will improve anything ...

TR’s picture

TR CreditAttribution: TR commented
Related issues: +#2028097: Map data types to interfaces to make typed data discoverable
manuel.adan’s picture

manuel.adan
Location 🌌
CreditAttribution: manuel.adan at Arcovía Digital for Asociación TecnoAriadna commented
FileSize
rules-inheritance_data_type_compatibility_checking-3022250-5.patch1.33 KB
interdiff-3022250-2-5.txt656 bytes

I got this error submitting a an action form with one entity argument. The expected data type was "entity:transaction" (an entity type from the transaction module) and the provided was "entity:transaction:bundle_xxx", so they did not match despite they are compatible types.

The patch tries to solve similar scenarios, such as checking the target_type 'entity:node' with the provided 'entity:node:page'.

I just found that it could fail checking a type like 'entity:test' against 'entity:test2', so I reviewed it.

And currently, a checking like "entity" against "something_entity:something" would pass, however that would be a rare scenario. The patch also solves that.

TR’s picture

TR CreditAttribution: TR commented
Related issues: +#2456831: [META] Add support for data type matching
akalam’s picture

akalam CreditAttribution: akalam at Metadrop commented
FileSize
rules-3022250-7.patch1.5 KB

Re-rolled #5 against the latest -dev

drupalfan79’s picture

drupalfan79 CreditAttribution: drupalfan79 commented

@akalam could you export your rule and paste it here to test, thanks.

lukus’s picture

lukus
he/him
CreditAttribution: lukus commented
FileSize
rules-data-type-inheritance-checking-3022250-9.patch863 bytes

Problem/Motivation
We've created a ruleset to set the schedule content moderation workflow state and schedule publishing time using actions.

In order to expose the necessary fields - we have added a condition to check for our bundle type. This functions correctly.

Entity is of bundle
Parameters: entity: node, type: node, bundle: article

Later when we create the action to set the schedule publishing time, we hit a validation problem:

Error message
Expected a entity:node data type for context Node for scheduling but got a entity:node:article data type instead.

After debugging, I can see that a comparison is being made (in drupal/web/modules/contrib/rules/src/Context/ContextHandlerIntegrityTrait.php:140)

Proposed resolution

  • In our case, 'entity:node' is being compared with 'entity:node:article'; the test is failing and we're being provided with the error message describe above.
  • $provider->getDataType() is providing the entity type with the bundle type included. The action plugin deriver only required a match on entity type.
  • We need to ensure that the comparison passes in this instance.
  • Rather than compare $target_type with $provider->getDataType(), propose we run a string comparison to check for the $target_type existing with the first part of the string that being compared.

Attached a patch which solves the issue in a slightly different way to the others.

lukus’s picture

lukus
he/him
CreditAttribution: lukus commented
FileSize
rules-data-type-inheritance-checking-3022250-10.patch863 bytes

Updated patch to leave @todo comment, as after reading through related issues, I can see that these changes don't satisfy the larger requirements mentioned in this issue(i.e. matching integer for float context).

TR’s picture

TR CreditAttribution: TR commented

We've created a ruleset to set the schedule content moderation workflow state and schedule publishing time using actions

What actions are you using? I would like to try to reproduce this. Are these Scheduler module actions? Previous posters didn't give complete information, notably leaving out information about the action used. I am curious about where entity:node:article comes from.

#2456831: [META] Add support for data type matching is more about type coercion - being able to cast a given type to the expected type. I think that can be handled independently from this issue, which is more about ensuring that subtypes can be used where the parent type is expected. Which IMO we shouldn't have to do explicitly, as this should be true at the language level, which is why I am wondering how to reproduced this. This error doesn't show up in anything I've used Rules for, which leads me to wonder whether some actions defined outside the Rules module have something to do with this.

We are going to need test cases using only Rules-defined actions that demonstrate this problem, so we can test to see if the proposed patches actually fix the problem without causing other problems.

lukus’s picture

lukus
he/him
CreditAttribution: lukus commented

Hi @TR

What actions are you using? I would like to try to reproduce this. Are these Scheduler module actions? Previous posters didn't give complete information, notably leaving out information about the action used. I am curious about where entity:node:article comes from.

The main action we're using to set the Scheduler 'publish on' date is: scheduler_set_publishing_date_action

This action requires the node input is of entity type 'entity:node'

--

We need to restrict our action to only apply to a particular bundle type.

If we add a condition 'Node is of type', the resulting node object that's supplied to our schedule action is correct. I.e. the comparison is 'entity:node' to 'entity:node'.

However, we wish to access fields specific to our bundle type; so we've chosen use an 'Entity is of bundle' condition which allows us to traverse and access the fields specific to our chosen bundle.

... the drawback of this approach is the type of the resulting node object that's supplied to our schedule action includes the bundle. I.e. the comparison is 'entity:node:bundle' (from the supplied entity being action-ed) and 'entity:node' the type required by the Scheduler publish on action.

--

On a slightly tangential note, we originally tried to set the publish date using a generic 'Set a data value' action to a timestamp contained in a variable. This wasn't easily possible, so we resorted to using the specific action supplied by the Scheduler module.

TR’s picture

TR CreditAttribution: TR commented

I really don't like what we're doing now, which is matching data types by string comparison of the name of the type. I don't really want to expand on that by trying to parse the string and make substring comparisons. That just seems wrong.

If you have a little time to work on this, my inclination would be to try to change checkDataTypeCompatible() to match based on class of the typed data object instead of the string data type name. So instead of:
$target_type = $context_definition->getDataDefinition()->getDataType() followed by if ($target_type != $provided->getDataType())
I would try to use something like $target_class = $context_definition->getDataDefinition()->getClass() followed by <if (!($provided->getClass() instanceof $target_class))
and of course there will probably still need to be special cases in the code, for example for our pseudo-type "any" and for entity references (which should be dereferenced so the pointed-to entity is the one used for comparison).

In fact, this is now sounding familiar ... after searching the issue queue I found #3059402-16: Set data action in referenced entity.. Maybe you can try the patch in #3059402-22: Set data action in referenced entity. and see if that solves this issue too?

lukus’s picture

lukus
he/him
CreditAttribution: lukus commented

I really don't like what we're doing now, which is matching data types by string comparison of the name of the type. I don't really want to expand on that by trying to parse the string and make substring comparisons. That just seems wrong.

I agree!

Maybe you can try the patch in #3059402-22: Set data action in referenced entity. and see if that solves this issue too?

It looks like this solves the exact issue! Thanks .. will try this out and report back.

lukus’s picture

lukus
he/him
CreditAttribution: lukus commented

Hey again @TR

Okay, so the patch in https://www.drupal.org/project/rules/issues/3059402#comment-13980031 does solve the issue I'm experiencing.

Looking at the resulting code, I can see that a string comparison is still occurring in various places.

If I was going to update the patch to compare based on class, would you agree it's more useful to provide the patch against this issue? The referenced issue seemed to start off as a more general query.

lukus’s picture

lukus
he/him
CreditAttribution: lukus commented
Related issues: +#3059402: Set data action in referenced entity.