Allow single-valued data selector input to be passed as an array for 'multiple' context fields

Thanks for the issue and the patch. However, this should not be implemented in the action or condition plugins, but as part of the context mapping system.

Generally, I'm not sure whether we should just silently support non-lists and upcast them internally to lists or whether we should having something like user.mail.value|to-list() in the selector (auto-completing of course).

I guess that the silent support option should be fine though. Setting to needs work as we need to move this to the Context/* mapping code somewhere.

I vote for silently upcasting - in essence treating single-valued data selector input as a one-element array when multiple-valued input is required for the context parameter.

Marking this as a "beta blocker" because so many people are getting tripped up on this subtle distinction (even after it's explained ...). See the related issues... Using some scheme like user.mail.value|to-list() isn't going to stop those issues, it's just going to be one more obscure thing to figure out. Plus it would require functionality like data filters, which currently only operate in tokens in direct input, to be applied to data selector values as well. While that might be a valuable new feature for power users, I think silent upcasting does what the user expects and I can't think of any bad side-effects of doing that.

This patch works for me

Hi lobodacyril,
Thanks for your interest in this issue. However, I think the idea is that the solution should be global for all multiple context fields. We want this to be fixed for all, including new actions that have not been thought of yet. See Fago's comment in #4

this should not be implemented in the action or condition plugins, but as part of the context mapping system

You are welcome to use the patch as an interim measure, and thank you for providing it, but we need to solve this issue in the proper way.

I would like to give this a go, but I need some directions:

• Regarding "should be implemented as part of the context mapping system":
  I see that context mapping happens in ContextHandlerTrait::prepareContext():

  // Map context by applying data selectors and collected the definitions of
  // selected data for refining context definitions later. Note, that we must
  // refine context definitions on execution time also, such that provided
  // context gets the right metadata attached.
  if (isset($this->configuration['context_mapping'])) {
    foreach ($this->configuration['context_mapping'] as $name => $selector) {
      $typed_data = $state->fetchDataByPropertyPath($selector);
      $plugin->setContextValue($name, $typed_data);
      $selected_data[$name] = $typed_data->getDataDefinition();
    }
  }
  

  I see the typed data is passed to the plugin as context value without checking if the data is of the correct type.

• A few lines later, I see the following:

  // Getting context values may lead to undocumented exceptions if context
  // is not set right now. So catch those exceptions.
  // @todo Remove once https://www.drupal.org/node/2677162 is fixed in core.
  try {
    $plugin->refineContextDefinitions($selected_data);
  }
  

  Some classes implement refineContextDefinitions(), though the base classes (RulesActionBase, RulesConditionBase) say:
  // Do not refine anything by default.
  So the implementation of refineContextDefinitions() seems to be plugin specific.

Questions:

1. Does making sure that a Rules plugin gets compatible context needs to happen before passing the context data to it, thus before calling setContextValue() on the plugin?
2. What's the purpose of refineContextDefinitions()? Is this the method where contexts should be converted?
3. Am I correct that we need to convert contexts so that they become compatible with what the plugin needs?

I think that the test in this patch demonstrates the issue to be fixed.

Hi MegaChriz, Thanks for looking at this.

I suggest that as well as creating a specific new RulesAction to test this in a kernel test, we should also use an existing action that we know does not work how we want it to, in a functional test. So I have done this, based on your idea in #2471481-123: Integrate Typed Data Widgets but using the actual mail.value in the data selector. This should be allowed and during save/or while being triggered, it should somehow get wrapped in an array. But at the moment, on trying to save, we get the error "Expected a list data type for context Send to but got a email data type instead.". Having both of these tests will help us to work on the solution.

@MegaChriz: Thanks, this is a good start. I also agree with #12, it would be nice to test with an existing action in a functional test.

BTW https://www.drupal.org/project/rules_data_transforms had an action similar to #11, but for D7 - in the long run I think it makes sense to evaluate whether we want some of those actions in core Rules, or in D8RE, or whether we want to try to port rules_data_transforms to D8 (are you interested in doing that?)

To try to answer your questions:

1. Does making sure that a Rules plugin gets compatible context needs to happen before passing the context data to it, thus before calling setContextValue() on the plugin?
   Yes I think this might be a good place to do the upcasting. I would try moving the getDataDefinition() call to just before the setContextValue(), then check the datatype and do the upcasting before setContextValue() if necessary.
2. What's the purpose of refineContextDefinitions()? Is this the method where contexts should be converted?
   I've thought of that, as a way to do entity upcasting (in that other issue), but I don't really want to overload refineContextDefinitions() too much - I think it would be better to have a separate upcast function used here. refineContextDefinition() is currently used to make context variable type more specific, for example when doing a "Data convert" to change a datatype, or when doing a "Entity is of bundle" to change to type to the more specific entity ("entity:node" to "entity:node:article", for instance.
3. Am I correct that we need to convert contexts so that they become compatible with what the plugin needs?
   I think the plugin defines the context it needs, and we need to make sure the user input either has the proper type or can be converted to the proper type before we try to set the plugin context. As a separate issue, I don't think we do any of this type checking yet when configuring a Rule, so it's possible to specify values that will fail at run time - that should be prevented, input type should be checked in the UI as far as possible, and submitting the form should fail validation. It is at this point, where we check that the user-provided value matches the type expected by the plugin context, that we should upcast single values to lists, or entity ids to entity objects.

Thanks TR, this is helpful.

I also agree with #12, it would be nice to test with an existing action in a functional test.

Yes this is what I added in patch #12. In #13.3 you siaid "I don't think we do any of this type checking yet when configuring a Rule, so it's possible to specify values that will fail at run time - that should be prevented". I will let @MegaChriz work on the actual solution of presenting array data to the plugin, but I can contribute here to the integrity checking as I have already done some work on this in patch #118 on #2471481-118: Integrate Typed Data Widgets. I have now discovered why we get the failure I tested for in #12. Using this multiple e-mail as an example. the integrity check should be ensuring that the selector picked is an email type, then its only later that we cast that into an array if required. The integrity check is trying to force it to be a list, which is wrong. We are using
$target_type = $context_definition->getDataDefinition()->getDataType() which gives "list". However, it should be using $target_type = $context_definition->getDataType() which gives "email". This allows the correct node.uid.entity.mail.value to be chosen. We still need to do the casting to array, but at least then we know that the element is of the desired type.

I have expanded the ActionsFormTest to cover entering values in the form and saving, to check for these validation errors/exception messages. Patch #14 has the additional tests, so this will fail. Note, the interdiff/patch looks like I have re-written the entire test data, I have just changed it to have three separate array parameters, rather that one, and missing parameters now default to an empty array. That's why all the lines have changed.

Whilst debugging the code I also found that unhelpfully we had two exceptions from different parts of the process but both gave the same mesage text "Unable to get variable '$name'; it is not defined". So I have pre-pended the exception type (IntegrityException or EvaluationException) to the message, which allows us to locate which exception is being thrown.

As expected we have the new failure

Drupal\Tests\rules\Functional\ActionsFormTest::testActionsFormWidgets with data set #25 ('rules_send_email', array('textarea'), array('node.uid.entity.mail.value', 'Some testing subject', 'The main message'), array('to'))
Behat\Mink\Exception\ResponseTextException: The text "Error message" appears in the text of this page, but it should not.

But I also forgot to check the exception message tests. Here's a patch which fails in only the intended places ;-)

@TR
Thanks for your answers - I now know for sure that I don't have to consider to do something with refineContextDefinitions(). I think adding a method to ContextHandlerTrait that handles the conversion would be good to do. I'll be focussing on only these two types of conversions:

1. Single value to lists
2. Entity ID to entity object

I hope I can find time to work on that in the coming week. If I'm short on time, I might ditch implementing the entity ID to entity object conversion. Adjusting input type checking seems like an other can of worms to me - so initially I'll not focus on that part.

Rules Data Transforms

BTW https://www.drupal.org/project/rules_data_transforms had an action similar to #11, but for D7 - in the long run I think it makes sense to evaluate whether we want some of those actions in core Rules, or in D8RE, or whether we want to try to port rules_data_transforms to D8 (are you interested in doing that?)

I didn't know that module existed! If I can find the time for it, I would love to bring something like that to D8. I and a few others have been working on a generic module for doing transformations: the Tamper module. I would love to see that bridged to the Rules module. The Tamper module was written to be able to do transformations in Feeds, but I've also worked on making it usable with Migrate in Migrate Tamper.
Do we need to create a separate issue to discuss this topic further?

I see more things in Rules I'd like to help with (TR, your method of showing where in the UI features are missing [D8 Rules Essentials] looks awesome), but finding the time for all that is going to be a challenge. I've also loads of stuff to do for Feeds and related projects. In the next three weeks I've been given a small portion of the time from my client to work on Rules.

2. Entity ID to entity object

If I'm short on time, I might ditch implementing the entity ID to entity object conversion.

@MegaChriz, I have good news for you, this is already tackled in #2800749: Support upcasting entity IDs to full entity contexts so you do not need to include that here.

Alright, I gave this a shot. I'm quite new to Context, TypedData and their definitions. Not completely sure about the implementation yet. The code is very much focussed on converting a \Drupal\Core\TypedData\ListInterface or a TypedData object representing a singular value to an array. With ListInterface conversion supported, both node.uid.entity.mail and node.uid.entity.mail.value can be used.

The method matchTypedData() could potentially get very long if all kinds of typed data conversions are needed. So I feel like we maybe need a service or a plugin system that can convert TypedData objects. Something like core's ParamConverter, though that one seems to be orientated around routes.

For now, let's see what the tests have to say about my patch.

Thanks MegaChriz, I'll do some testing on this. I have also further expanded the tests, will report back later.

No code changes, but patch needed a re-roll to remove the updated tests that are now committed in #3160067: Refactor and expand ActionsFormTest and ConditionsFormTest

@jonathan1055 works like a charm. All patches are working together at this time! Keep you the great work as rules is awesome!

I have numbered the points continously throughout this post to help with referring back later.

Results of manual testing

I've done some testing on this, deciding to focus on the four actions and two conditions which have multiple = TRUE, as these should be the only ones which are affected by this issue. (need to check the others too, of course). The point of this issue is to allow a single value from a data-selection to be used in place of the array which is created when multiple values are input directly.

1. Action
rules_send_email
context $name = to
ContextDefinition dataType = email
DataDefinition dataType = list

2. Action
rules_email_to_users_of_role
context $name = roles
ContextDefinition dataType = entity:user_role
DataDefinition dataType = list

3. Action
rules_user_role_add
context $name = roles
ContextDefinition dataType = entity:user_role
DataDefinition dataType = list

4. Action
rules_user_role_remove
context $name = roles
ContextDefinition dataType = entity:user_role
DataDefinition dataType = list

5. Condition
rules_user_has_role
context $name = roles
ContextDefinition dataType = entity:user_role
DataDefinition dataType = list

6. Condition
rules_node_is_of_type
context $name = types
ContextDefinition dataType = string
DataDefinition dataType = list

I have put the detailed results in a google drive sheet
https://docs.google.com/spreadsheets/d/1U16iDfj-b_R4ktf5wBAkpPDP-HKl7dF4...

In summary, action 1 and condition 6 now work ok and a single value from data-selection is wrapped in an array and the rule executes as expected.

With the other four, the use cases are slighly more obscure but we should be able to either make them work or give a better validation message and avoid exceptions or runtime errors. For example, the action to send an email to all users of a role selected from the node author's roles is a bit obscure. Likewise checking that the current user has a role matching one of roles of the node author. Also this issue is about selecting one value, whereas most of these other cases are more likely to want the whole set of roles of an author, eg. does the current user have any roles in common, etc. So maybe it's fine that this patch does not make any improvement on cases 2 - 5. Although case 3 & 4 are interesting in that the new code was run, the values wrapped in arrays, but the same runtime error occurred.

7.

The method matchTypedData() could potentially get very long if all kinds of typed data conversions are needed.

This comment worries me. The issue should be simply about wrapping single values in an array if the context expects an array. I don't think we should be trying to do any more than that here.

Review of patch #18 / #20

8. In src/Context/ContextHandlerIntegrityTrait.php checkDataTypeCompatible()

+    // If the target type is a list, get the item data type instead.
+    if ($target_type != $provided->getDataType() && $context_definition->isMultiple()) {
+      $target_type = $context_definition
+        ->getDataDefinition()
+        ->getItemDefinition()
+        ->getDataType();
+    }

I think the assignment of $target_type here can be simplified to just

+    $target_type = $context_definition->getDataType();

I have checked all six 'multiple' cases, and this returns exactly the same as the longer route you had. Actually, when multiple is not set, $context_definition->getDataDefinition()->getDataType() returns the same as $context_definition->getDataType().

9. Here's an improvement for accuracy, readability and maintainabilty - $target_type should be reused directly in the message, so that if it is changed (like it is now in this patch), we don't have to alter it in two places. Here's my suggested change:

     if ($target_type != $provided->getDataType()) {
-      $expected_type_problem = $context_definition->getDataDefinition()->getDataType();
       $violation = new IntegrityViolation();
       $violation->setMessage($this->t('Expected a @expected_type data type for context %context_name but got a @provided_type data type instead.', [
-        '@expected_type' = $expected_type_problem,
+        '@expected_type' = $target_type,
         '%context_name' = $context_definition->getLabel(),

In src/Context/ContextHandlerTrait.php matchTypedData()
10. I have run the full test suite and nothing used the 'isList()' part of the new function, not even in the new tests.
11. Many many times the target type = 'any' so the call did nothing. Can this be made more efficient by not calling in the first place?
12. When conversion is required, we do not want to create a new Typed Data object, as this loses some properties e.g. the name. Simply use ->setValue($convert), like this:

-      // @todo use dependency injection.
-      $typed_data = \Drupal::service('typed_data_manager')->create($plugin_context_definition->getDataDefinition(), $convert);
+      // Set the converted data.
+      $typed_data->setValue($convert);

which I have tested and works.

Overall this is a good start to solving the problem. I think we need to be careful about making special cases and doing too much to fix things in specific ways, when we need to stay generic and remain consistent with the ideals of typed data. (I'm not sure exectly what that means, though :-)

Here is a reroll of the patch in #20.

For successful using the "send email" action I also need the patch from #2471481: Integrate Typed Data Widgets. With that one you'll get a textarea instead of a textfield for the message body.
Since the patch from that issue and the patch from here conflict, here is also patch that is built on top of #2471481-174: Integrate Typed Data Widgets.

Converting patch into an MR and rebasing to 4.0.x.

Saved changes from #25 MR as a static patch for rules v4.0.

The patch in #26 works with rules 4.0.0 and drupal core 10.3.14