Integrate Typed Data Widgets

Hi. Can I get a little more background on this one so I can move forward with it? What are the data types we are trying to create. Is there a document I can follow that might lead me in the right direct? Or are we leveraging the ui.data.inc code and just refactoring for the Typed Data API? Feel free to hit me up on IRC if you have time to discuss this tomorrow.

I've done something similar to this for my own purposes, but I think that it's a start at abstracting to a wider use case for any typed data form/widget builder. XeroFormBuilder.

Basically a service that recursively generates form elements for Typed Data with some "smart" (i.e. dumb) logic to change things into select lists based on constraint. I was thinking of abstracting this out to a separate module that just implements this service.

@mradcliffe That sounds & looks interesting - thanks. Yeah, the idea for typed data widgets here is to basically implement a plugin system for widgets analogously to field widgets, but tied to the typed data system instead of the field system.

The XeroFormBuilder seems to go even a bit further in that it is able to generate forms for complex data items even, what can perfectly build upon the widgets and would make sense to be part of the same library imho. Would you have interest to collaborate on that?

First off, we need to separate the TypedData code from Rules though. Then we need to flush out the base framework, i.e. the plugin system and interfaces and the testing framework. Once we've got that in place, we'd have a lots of tasks to add various suiting widgets per data type.

I thought about this a week or so ago:

• Entity: Rules only needs a couple of properties (like bundle). Maybe include required properties?
• Field items should use recurse through property definitions.
  • But this also has other non-properties that might need to be set such as language
• Maps could get pretty large. Maybe only provide a form for required properties.
• Primitive types are pretty simple.

Other notes:

• computed properties should be ignored, read-only too (or make that an option).
• validation? probably later..?
• Rules then needs to add data selector stuff to the element.
• Some way to stop/prevent recursion
• Alter / Event subscriber?

I also had started working on a module forking XeroFormBuilder, and I had not published any code yet.

This morning I worked on learning Prophecy (finally!) and added some tests for string and boolean primitives. I've published the code to github and will continue adding tests for primitive types first, then removing Xero cruft and making it usable based on my above notes and any additional plan.

I'll add you as a collaborator @fago already. I think it needs more cleaned up before pushing to drupal.org

@mradcliffe: Awesome, thanks! I'll check it out and get it touch with you for further things.

Given how fundamental that is, I think that should block beta.

I had time to work on it some in the last few weeks. I think that the best use would be for non-data selectors, and to restrict narrowly to entity/typed data properties at first.

I'm not entirely sure how to handle bundles and I'm still weighing about the best way to pass in options. At the moment it has a WIP $options that is only used to pass in the bundle key for when getting entities. It would probably be better to have methods to set specific options for specific things.

Passing in field_item:PLUGINID ends up with a bunch of text fields usually. I don't think that makes sense to use for Rules.

// This should populate an entity autocomplete by default.
$element = $builder->getElementFor('entity:node', 'author');

// I don't like the way this requires passing in bundle key.
$element = $builder->getElementFor('entity:node', 'field_favorite_color', ['type' => 'person']);

Probably my blockers for getting onto drupal.org would be

- solidify doing required things like passing in options to various builders.

I think after that anything that needs to be changed can be done without being API breaking like making getMethod return callables instead of strings or any drupal_alter() nice-to-haves.

Ok, as typed data is now separated this will be solved in https://www.drupal.org/node/2809365. Once done, we just need to integrate it here. Until we can do that, I postpone this issue.

#2809365: Add typed data widgets has been completed, so we can unpostpone this.

There is \Drupal\rules\Form\Expression\ContextFormTrait which we need to work over. I think we should move it below the context form namespace better though.

There we can then use the typed data widgets to generate a suiting a form element per context definition. For having proper default per data type we need to extend the context definition class with getWidgetId() and getWidgetSettings() methods. For now we can just hard-code the data-type specific defaults in the getters there.

I did some first-draft changes:
https://github.com/Max-Kuzomko/rules/commit/b4f702e445d1519a4a6686933319...

@fago, Could you please check whether - did I move correctly \Drupal\rules\Form\Expression\ContextFormTrait below the context form namespace?
I am not sure how getWidgetId() and getWidgetSettings() should work?
Should we pass a widget to the ContextDefinition as a param. Or we should hardcode a replacement as in my commit. For example:
datatype any - texfield
datatype text - texfield

Thanks!

Or we should hardcode a replacement as in my commit. For example:
datatype any - texfield
datatype text - texfield

yes, just hardcode a mapping of data type to suiting widget.

case 'integer'
case 'string:
return 'textfield';
etc...

1. Do we need to create widgets for the following data types: any, binary, boolean, map, language_reference, language?

I'd suggest we add a "BrokenWidget", id "broken" which is default for everything that cannot be input, or is not implemented. Then in the UI we just show an error message and avoid the form to be saved by providing a validation error.

For boolean, we can go with a textfield and entering 1 or 0 for now + creating a follow-up for a proper widget.

2. Which form element should represent the ItemList data type?

Let's use the usual simple textarea with one line being used per item + split it by line on saving. E.g. see the configuration form for available options of the ListItemInteger field type.

I've re-rolled max-kuzomko's changes against HEAD and made a couple small fixes in an effort to keep this moving along: https://github.com/fago/rules/compare/8.x-3.x...acrollet:issue-2471481-test

If I get time, I'll try to work on the comments in #17

I've improved this to the point where it is basically working, in terms of using a specified widget based on the data type and using a broken field if the data type is not recognized. It relies on a fork of typed_data with the broken widget added: https://github.com/fago/typed_data/compare/8.x-1.x...acrollet:8.x-1.x

At this point this needs review to make sure the implementation is making sense.

Thank you, #19!

I've re-rolled your changes (fixed coding standards, made adjustments to the widget, added tests).
Could you please use https://www.drupal.org/node/52287 in commits messages?

@fago:
Could you please review the PR?
https://github.com/fago/typed_data/pull/12

Looks like CI build failed because of this issue: "/home/travis/.travis/job_stages: line 57: ./vendor/bin/phpcs: No such file or directory".

In response to

Looks like CI build failed because of this issue: "/home/travis/.travis/job_stages: line 57: ./vendor/bin/phpcs: No such file or directory"

The error in typed_data .travis.yml is exactly the same as I found in Rules. The fix is simple - see #2937362: Fix composer install syntax in .travis.yml

Thank you, @jonathan1055!
I have created an issue & resolved:
https://www.drupal.org/project/typed_data/issues/2940142#comment-12454406

@acrollet, thank you for your help with #18!
I have re-rolled the patch with some adjustments. Created a pull request:
https://github.com/fago/rules/pull/499
Looks like some tests for the PR failed only with Drupal 8.5 (We discussed this issue on some Rules meeting).

@fago, could you please review the issue above and the pull request?

Looks like my PR from the comment #2471481-20: Integrate Typed Data Widgets failed because of some phpcs issues also. However I checked phpcs before pushing...
https://github.com/fago/typed_data/pull/12

Anyway, @fago, please consider this PR:
https://github.com/fago/typed_data/pull/14 (all checks passed)
instead of
https://github.com/fago/typed_data/pull/12

https://github.com/fago/typed_data/pull/14 is fine, thx - I merged it. So seems we can move on with getting https://github.com/fago/rules/pull/499 green then.

we need to integrate the broken widget as fallback now.

The test run on https://github.com/fago/rules/pull/499 should now be fixed - see #2936642: Getting runtime contexts will generate an E_WARNING for anonymous users which was committed to core on 4th Feb. If the tests were to be run again on PR 499 they might/should be green now.

@fago, I have reopened the PR. Now it's green.
Could you please review?

https://github.com/fago/rules/pull/499

@max-kuzomko, forgive me if I'm missing something, but some code seems to be missing from the latest PR, getWidgetID() is not called from anywhere as far as I can tell?

@acrollet, thanks for your notice!

But I think we need to merge this task, so we can use this functionality in the tasks like this one: #2724129: Change message field to textarea in Send email action

@max-kuzomko: Maybe you could post your patch here, so the testbot can test it and those of us who participate in this queue can review it?

Just a heads-up that the patch in #27 does not currently resolve #2724129: Change message field to textarea in Send email action. After applying the patch the email message/body input remains a 128-char-limited textfield, when it should be a textarea. It sounds like @fago was hoping the resolution to this issue would also resolve #2724129: Change message field to textarea in Send email action, so I'm marking this needs-work.

Hi @chrisolof,

Thanks for your comment!

The pull request in this issue (#27) is not supposed to resolve another issue.
As I can recall on the last call with @fago we discussed that in this issue we need to integrate Typed Data Widgets, so we can use it to resolve Make message field as textarea in Send email action

Well, you need a patch for this to be "Needs review", so here's the patch taken from the pull request in #27. It was last modified 27 Jan 2018. I'm going to assume that's as far as it got.

This still needs the 'broken' widget from #19 to be put into typed_data - there's no patch/issue in that queue for that.

Also, like acrollet mentioned in #28, there is code missing that was in #18. Specifically, you have to use getWidgetId() somewhere in Rules - if you want to 'integrate' typed data widgets they have to be used!

Also, there are no test cases added for the widget integration?

So regardless of the patch test results, this really should still be "Needs work".

Here's the patch extracted from the pull request in #18 - this seems more complete than what was in #27...

Squashed and re-rolled ... this should apply.

Opened #2990333: Commit found on GitHub is missing from drupal.org for the BrokenWidget, which was committed to Typed Data API on GitHub (see #24 above) but was never pushed to Drupal.org !

Oh, and for those of you interested in this issue, @fago isn't very active here anymore - that's why this issue has been stalled since January. He's probably not going to respond to anything on GitHub. The GitHub workflow, which REQUIRES @fago's active participation, has become the #1 impediment to development of Rules for Drupal 8.

If you want to see this put into Rules, and want to get Rules into a beta release then a 8.x-3.0 release, you will have to help out here in the drupal.org issue queue with patches that can be seen/applied/reviewed by everyone in the Drupal community including the drupal.org testbot.

Oh, and for those of you interested in this issue, @fago isn't very active here anymore - that's why this issue has been stalled since January. He's probably not going to respond to anything on GitHub. The GitHub workflow, which REQUIRES @fago's active participation, has become the #1 impediment to development of Rules for Drupal 8.

Well as said I'm happy to grant you access there, but if it's preferred to move development back to d.o. that's fine to me as well. Please let me know your preferences and let's document that!

I think the Github workflow is preferable on a project the size of Rules. Fago has done a great job and it worked well in the past. Plus the integrated testing on travis-ci.org shows all contributors that the changes work (or not). That testing allow non-project maintainers to alter the test setup (eg for new core releases) without having to change the tests on d.o. (which we know is cumbersome and has a few glitches when it comes to setting core version/db/php combinations)

Re-rolled the patch against the latest 8.x-3.x and adjusted the default field type from textfield to textarea, which solves #2724129: Change message field to textarea in Send email action. Probably still needs work but this at least gets basic rules functionality like sending an email working.

with patch from #40, textarea are rights. But data selection still be limited as my screenshot.
EDIT: I can access to more options with adding "." after a choice...It could be great to add this info to the help description because it's not very intuitive.

I don't see anything wrong with that image.

@TR: I don't have entity choice in the provided list as I saw it in tutorials.

The available variables depend on the context. Evidently there is no 'entity' in your context, so you're obviously not reacting on an entity event. I don't know what your event is, but if it doesn't provide an entity then how do you expect to set values on that entity?

Because I try to make a component, but perhaps it would be better to create a rules reaction instead to have ability to use en event.

The component UI doesn't work yet in D8 - that's the subject of another issue. Regardless, this has drifted way off topic. Let's keep further discussion in this issue focused on the patch in #40 and whether it does what it is supposed to.

I'm getting an error when attempting to apply the patch against the current dev:

patching file src/Context/ContextDefinition.php
patching file src/Context/ContextDefinitionInterface.php
patching file src/Context/ContextHandlerTrait.php
Hunk #3 FAILED at 269.
1 out of 3 hunks FAILED -- saving rejects to file src/Context/ContextHandlerTrait.php.rej
patching file src/Form/Expression/ContextFormTrait.php
patching file src/Form/Expression/ActionForm.php
patching file src/Form/Expression/ConditionForm.php

@jwineichen: There have been hundreds of changes to Rules since the patch was made back in June. In this case, all the patch needs is a trivial re-roll to accommodate those changes. Looks like all you have to do is remove the ":" after the @todo. Perhaps you could re-roll the patch and post it here to help move this issue forward?

Re-rolling #40 against latest 8.x-3.x-dev (as per request in #48).

Hi @bisonbleu, I've tested out the patch from #49

I am getting wsod error when trying to add condition or action:

Fatal error: Trait 'Drupal\rules\Form\ContextFormTrait' not found in /home/public_html/web/modules/contrib/rules/src/Form/Expression/ConditionForm.php on line 19

Hi @muaz91, part of the patch in #40 did not make it into my re-roll, namely:

rename from src/Form/Expression/ContextFormTrait.php
rename to src/Form/ContextFormTrait.php

Which explains te error you are getting. My bad. I'll re-roll when I have a few minutes.

Here is a new patch that fixes the issue reported in #50.

Thanks bisonbleu for re-rolling the patch. I noticed that it added the new file src/Form/ContextFormTrait.php but did not remove/rename the old file src/Form/Expression/ContextFormTrait.php so it was hard to see what had changed. Patch #53 now detects that this is a rename + changed file, so we can see the differences.

I also did some testing and discovered where the error Undefined index: data in Drupal\rules\Form\Expression\ConditionForm->buildContextForm() was coming from and have fixed it hopefully (see interdiff). At least, that single RulesUiEmbed test on my local testing runs OK.

I've tried to work out what was going wrong with those last two tests. The problem is that
\Drupal::entityManager()->getStorage('field_storage_config')->load('node.title') returns a NULL object. The reason seems to be that the function
\Drupal\field\Entity\FieldStorageConfig::loadByName($entity_type, $field_name) only works for the configurable fields, not the entity's base fields, according to the Field info methods change record. So instead I have used
\Drupal::service('entity_field.manager')->getFieldStorageDefinitions($entity_type)[$field_name] which does return the field object.

Secondly, I found out that adding the full $widget->form($typed_data, $sub_form_state) to the form was wrong, as this was an array with just '#tree' and 'value'. To make it consistent with the rest of the processing we need to use just the array below 'value'. The #default_value was not being set before, so in manual testing the value was being lost on each edit. This is fixed now.

I also simplified getWidgetId() by using in_array instead of a second nested foreach. The interdiff shows all these changes.

There is probably still work to do, but this should at least be a useable version, and hopefully the tests pass on d.o. (they do locally).

Good, the tests now all pass (for the first time on this issue).

Instead of \Drupal::service('entity_field.manager')->getFieldStorageDefinitions it also works with \Drupal::entityManager()->getFieldStorageDefinitions. Is there a preference for one or the other?

jonathan1055 Not sure if it's a typo, but \Drupal::entityManager() is deprecated and will be removed in D9, so yes there is a preference!

Thanks rivimey, yes you are a right. I only tested locally on 8.7 which does not throw the @trigger_error for the deprecation, but on 8.8 it does show the message. The patch has the non-deprecated version, so that's OK.

I tried to apply this on current HEAD and it fails, partly because ContextFormTrait is in the Form\Expression folder and so not found. The attached patch fixes the code to match the file location though it could be that the file location is the wrong thing. The patch also updates to be against current HEAD = dbede18dc

I am not sure yet whether the patch is now doing the right thing - any tests?

[context: I am trying this patch because I hit the email body field using text/input not textarea widet with 128 char limit. At present the patch has made no difference to the field, but I'm not sure if it should have ...]

On further inspection I see the ContextFormTrait was supposed to move from \Expression, so here is an updated patch with that change re-made. I used 'patch" to apply the patch - perhaps it doesn't understand git patches any more?

Overlapped posts. Here is what I was just trying to submit, when your reply changed the form values. I took too long to write it.

1. I don't know why you could not apply the patch, because it applied ok on drupal.org yesterday and there have been no commits for several weeks. When you say HEAD, do you mean the 8.x-3.x branch? Maybe my understanding is not quite right. dbede18dc is the latest commit.
2. The file src/Form/Expression/ContextFormTrait.php is supposed to be moved to src/Form/ContextFormTrait.php as per @fago's request way back in comment #13 My patch 54 detected the similarity index because this was a 'move and change'. Your patch does not move the file, but makes the changes in-situ. I am assuming we still want to move the form class as per #13
3. I don't think there are tests yet which explicitly check the new code. However, we do have green tests, which is an improvement from before
4. As I understand it, this issue will not in itself fix the e-mail textarea problem directly, but it will allow #2724129: Change message field to textarea in Send email action to be sorted out properly, see comment #31 and #32

Your patch 58 is now identical to mine in 54 (apart from a couple of use statements that are in a different order).

Jonathan, sorry for disruption. I applied the patch via composer's built-in patches functionality (=="patch" prog) and it failed to apply the file rename, hence my original post. The "use" statement change was simply that a line moved in one of the previous patches in a way that just added clutter to the patch. I did mean head of 8.x-3.x branch.

I was asking about "any tests" because after I applied it properly (i.e. post-58) I couldn't see any change in the website, so "tests" here included eyeball testing :-) Thanks for your confirmation that this is the expected situation.

Returning to this patch, what is needed to get it committed? I can confirm that it doesn't appear to break anything, but as I can't see it fixing anything either (on its own) I think it at least needs some tests. Preferably there should also be some useful work it enables (eg the textarea thing) that demonstrates why the patch is needed.

Would one of the patch authors at least describe what functionality was intended so tests can be written to demonstrate that the patch does it?

Returning to this patch, what is needed to get it committed?

Tests. We need, at minimum, a test that creates a test condition or action which includes context variables of all datatypes used by Rules and checks that the appropriate form widget is used for each datatype. This sounds harder than it is, as we already have an extensive test base so it just involves some modifications to existing classes and test modules.

Also see #3091923: Add a new "text" datatype for long text, which has to be done first, and #2724129-59: Change message field to textarea in Send email action, which lays out the steps that need to be taken.

Date widgets are the subject of #2943253: Add form widgets for date fields. It would be nice to get that finished and into this patch as well.

Yes I had noticed that dates were an omission that needed fixing. I have made a comment in the getWidgetId function, after analysing the match between available widgets and the ones catered for in the existing/previous patches:

/*
Allowed values from PHPUnit exception message:
  any, binary, boolean, datetime_iso8601, duration_iso8601, email, float, integer,
  list, language, language_reference, map, string, timespan, timestamp, uri

Extras we have in getWidgetId:
  datetime, duration, link, daterange

Not catering for:
  binary, datetime_iso8601, duration_iso8601, language, language_reference,
  map, timespan, timestamp
*/

I have not investigated but I presume we ahould use the _iso8601 versions of the datetime and duration? And probably the coded 'daterange' should actually be 'timespan'. We did have url in the previous patches but I've changed that to uri to match the actual widget available.

I will have a new patch here soon.

I have not investigated but I presume we ahould use the _iso8601 versions of the datetime and duration? And probably the coded 'daterange' should actually be 'timespan'.

Yes to all.

And don't forget the new "text" type. We can put that in the patch here even if it's not committed to typed_data yet.

And don't forget the new "text" type.

Oh, yes that's already in, and working perfectly for the email body (locally with patch #3091923: Add a new "text" datatype for long text applied).

I committed #3091923: Add a new "text" datatype for long text.

I've done some more work on this and in principle I think it is working. There is no interdiff as it changed quite a bit from #54. There is still some work to do, but what I have got is:

1. Cater for new 'text' datatype - committed in Typed_data #3091923: Add a new "text" datatype for long text
2. Changed two actions which send e-mails to use the new widget, to show that it works
3. New test which adds each Rules Condition and Rules Action and checks the edit page loads. This shows that the datatypes exist
4. Locally is it working with the new datetime and datetime_range widgets - #2943253: Add form widgets for date fields. This required a significant re-work because that widget has two data-input items. So now the ContextFormTrait caters for any number of input items and does not need prior knowlegde of their names (previous attempts had hardcoded 'value', and Rules had the fixed key 'setting'. This is now extracted from the widget)
5. Minimal form-building manipulation is done now, most of it comes from the widget form which is what we wanted

Things still to do:

1. The lookup of data type from the data-selector $property_path is not working, because I am not certain what it actually needs to do. I made two attempts, and documented the outcome in a @todo around line 60
2. Line 90 - if the returned widget is 'broken' then a message is shown on screen. The widget is used and nothing else is done, as the widget does not have any input fields. I think this is what was intended
3. Line 121 - checking if the widget has a single simple input field or multiple inputs. Not sure if if I've done it the best way, but it works for datetime_range which is the only 'multi-input' widget we have right now. There is a @todo explaining this
4. The new tests check that each condition and action can be editted. They do not do anything more yet, but this will need to be enhanced

There is probably more to say, that I have forgotten, so ask if anything looks odd. The tests pass locally, and I am hoping that Typed_data will get patched via the a drupalci.yml file (which is temporarily added to this patch) as the version loaded will be alpha3 not -dev. Let's see ...

That went well.

Applying patch /var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-12626/ancillary/2471481-69.integrate-typed-data-widgets.patch
...
Replacing build:assessment stage with /var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-12626/source/modules/contrib/rules/drupalci.yml
...
Host command.
cd modules/contrib/typed_data; sudo -u www-data curl "https://www.drupal.org/files/issues/2019-11-02/3091923-3.text-datatype.patch" | sudo -u www-data patch -p1

The tests would have failed if the new 'text' datatype was not available, so the patch of typed_data worked and all seems good.

Another strategy is to add a require-dev block to composer.json to require typed_data 1.x-dev for testing. This is what I am doing in #3095525: Run tests using Rules -dev release, for instance.

I'll take a look at the patch in #69 later today.

OK, I just started looking at this, and here are my initial notes. I haven't actually patched my local and tried it out yet. I'll try that in a while to see how it works.

For getWidgetId($dataType) I think it makes more sense to do the mapping like this:

  $widgets = [
    'any' => 'text_input',
    'boolean' => 'text_input',
    'email' => 'text_input',
    ...
  ];

That is, a direct lookup between $dataType as the key and widget name as a value, NOT nested arrays. That way you won't need to loop over values and it will be easier to read/maintain. Additionally, there should be a hook so that if a module wants to declare its own data type it can specify which widget to use for that type. I suggest looking at core/lib/Drupal/Core/Locale/CountryManager.php and copy the two functions there as an example - it has a default list of mappings and it has a function to get the list after hook_alter(). Note it only calls the alter once per page request, which is a micro-optimization that will probably be even more useful for Rules since we will usually be looking up more than one mapping at a time. I would however change it so that the first function, the default mapping, is a protected function and also change the second function to return a particular mapping like getWidgetId($dataType) currently does, rather than return the whole list. Make sense?

Is it OK just to show a message on-screen

Yes. If Rules covers entities and all core typed data data types, then I think the number of cases where 'broken' comes into play are very few. A message could say to implement the hook_alter() (from above) in order to expose your custom data type to Rules.

Get the widget and build the widget form.

Seems to me this section is a lot more complicated that it should be - perhaps there's something we need to add to typed_data to make using these widgets a little simpler. Just thinking out loud here...

It's unclear to me why you made the change from, for example, context[data][setting] to context[data][value] in the tests. It worked before, so what exactly changed that made this necessary?

public function testConditionsAndActions() is a perfect place to use a data provider. This is something we didn't have in Simpletest, which is why our tests don't yet use providers. But there are many places, like this, where they would be useful. I would split testConditionsAndActions() into two functions, one for conditions and one for actions, along with a provider function for each. Basically it works like this: define the test function with arguments. E.g. public function testConditions($id, $settings) - this test function is written to just test one ($id, $settings) value, not loop over many values. The doc comments for this test function tells PHPUnit what provider function to use. Then define a provider function which simply returns an array of [$id, $settings] pairs. PHPUnit will take care of invoking testConditions() once for each pair provided by the provider function. This is a nice way to do it because it separates the test itself from the data input to the test, making things easier to read, and makes it so the test bot can run multiple cases simultaneously. A good example of how to do this may be found in core/modules/user/tests/src/Unit/PermissionAccessCheckTest.php and core/modules/taxonomy/tests/src/Functional/Rest/TermResourceTestBase.php
Actually, maybe even take these new tests out of ConfigureAndExecuteTest and put them into a new class, since ConfigureAndExecuteTest is getting pretty big.

The only thing of real concern is the conditional which checks on the *name* of the context parameter. That is not something we should be counting on. This check introduces a bit of 'magic' in that people who write condition/action plugins just have to know that there are special parameter names that do different things. I think the only thing we should be relying on is type.

Thanks TR for the review.

1. Another strategy is to add a require-dev block to composer.json to require typed_data 1.x-dev for testing.

   That's a good idea to get the -dev using composer.json, as there could well be more patches to use next time.

2. For getWidgetId($dataType) ... a direct lookup between $dataType as the key and widget name as a value, not nested arrays.

   Yes that's better, I will make that change. I did it with nested arrays as that is how it was done right from the outset, before I got involved.

3. there should be a hook so that if a module wants to declare its own data type it can specify which widget to use for that type.

   Yes good idea, I'll do that. Thanks for the example/hint

4. ... message could say to implement the hook_alter() in order to expose your custom data type to Rules.

   That's a good idea

5. Seems to me this section is a lot more complicated that it should be - perhaps there's something we need to add to typed_data to make using these widgets a little simpler

   Could be. I will have a think about it

6. It's unclear to me why you made the change from, for example, context[data][setting] to context[data][value]

   Ah, yes I did not expain that very well. It is because for the widgets which have two (or more) entry fields, such as the new datetime_range, I get the input names from the widget, as we cant just hardcode context[data][setting] because we need two of these. For the datetime_range the two entry fields are called 'value' and 'end_value'. All other widgets (so far) have only one input field and it is named 'value'. I took the decision to use the names from the widget, so that for example when writing tests for the datetime_range it would be clear when checking the fields named 'value' and 'end_value'. Rules cannot make assumptions about how the widget fields are named so I just took the names (using that array_keys bit). But this had a knock-on effect of making those tests fail (which I only discovered at a late stage). I was not overly happy in changing the tests. So another alternative could be for rules to use 'setting', 'setting2', etc. and populate them from the widget fields in sequence. Then Rules tests would always refer to 'setting' as is done now. It has the downside that 'setting2' is not so descriptive as 'end_value' but maybe that's OK.

7. public function testConditionsAndActions() is a perfect place to use a data provider ... I would split testConditionsAndActions() into two functions ... take these new tests out of ConfigureAndExecuteTest and put them into a new class

   Yes to all three

8. The only thing of real concern is the conditional which checks on the *name* of the context parameter.

   Happy to answer, but I am not sure which bit of code you are referring to. Ironic that your main concern is one that I can't identify ...

Again, thanks for working on this. I appreciate that there's a lot of baggage from previous patches, but yours is the first that seems to be what we need to solve this issue so I'm giving it a lot closer look than I normally would because this is such a critical issue for Rules. IMO extra effort now to make sure it is done right will save us a lot of headaches in the future. Rest assured I think you're on the right track and I think we're close to getting this resolved - which will be a HUGE usability win for Rules.

Happy to answer, but I am not sure which bit of code you are referring to. Ironic that your main concern is one that I can't identify ...

Yeah, I could have been more specific. I guess I was winded from writing that long post :-)
Here is the code I was talking about:

    if ($context_name == 'value' && isset($configuration['context_mapping']['data'])) {
      // For conditions that check the value of a data item, use the entity type
      // and field name to pick a suitable widget for the value entry field.
      $property_path = $configuration['context_mapping']['data'];
      // @TODO Getting the datatype from the first two items in the patch was
      // how it was done in the first patch, but this does not work for complex
      // values such as node.uid.entity.name.value or
      // @user.current_user_context:current_user.uid.value
      // Current method: using . : or @ as delimiters, ignoring 'value' take
      // the penultimate and last items.
      $path_array = preg_split('[\.|:|\@]', $property_path);
      $field_name = array_pop($path_array);
      if ($field_name == 'value') {
        $field_name = array_pop($path_array);
      }
      $entity_type = array_pop($path_array);
      /*
      // This is still not right, so avoid error and leave widget to default.
      $dataType = \Drupal::service('entity_field.manager')->getFieldStorageDefinitions($entity_type)[$field_name]->getType();
      $widget_id = $context_definition->getWidgetId($dataType);
       */
    }
    elseif ($context_name == 'operation') {
      // @todo The choice of operators should be in a selection list/dropdown
      // not text entry. Could be done here when the selection widget is coded.
    }

This checks to see if the context variable name ($context_name) is 'value' or 'operation', and handles things in a different manner. While these are the names we've chosen to use consistently in Rules for data input values and for operations on data respectively, these names are going to be used for future Rules or for contributed module Rules - it's just a convention which is internal to the Rules module and which I'm not 100% sure we adhere to in Rules anyway.

BTW, what is datetime_range and how does it differ from the core duration_iso8601 data type?

Patch in #69 worked for me after updating typed_data to the latest dev (got the text error without it).

Thanks @jwineichen that's good to hear.

In reply to TR, #73.6

It's unclear to me why you made the change from, for example, context[data][setting] to context[data][value]

I've been working on this, with the intention of reverting back to the original rules key 'setting' instead of the widget input 'value' (and correspondingly 'setting2' for the widget's second input key, etc) and this is not going to be possible. The datetime_range widget has a validator which is expecting 'value' and 'end_value' in the form (which is perfectly acceptable, as the widget put them there). So we can't change the widget entry names to 'setting', 'setting2' as this produces:

Notice: Undefined index: value in Drupal\typed_data\Plugin\TypedDataFormWidget\DateTimeRangeWidget->validateStartEnd() (line 123 of /typed_data/src/Plugin/TypedDataFormWidget/DateTimeRangeWidget.php). => Array ( [24: Drupal\typed_data\Plugin\TypedDataFormWidget\DateTimeRangeWidget->validateStartEnd() 

Even the widgets with a single entry value could provide a validator, so its not just a problem with new the multi-value widget. Before we tried to integrate the widgets, we were free to name these form items to whatever we wanted, but now we have to use the widget entry names. That's not a bad thing, and interesting that I chose to do it that way before I discovered that it had to be that way.

So this does lead on to #73.5

perhaps there's something we need to add to typed_data to make using these widgets a little simpler

One of the complications is deriving these input names (I did it by taking the widget form, eliminating the keys which started with '#' and what we are left with is the actual input field names). Maybe those names could be supplied directly in the $widget object?

We could also consider the naming of 'data', 'operation' and 'value' as you pointed out in #74. Because most widgets (well, all so far) have an entry field named 'value' it means the overall $form often has fields called context[value][value] where the 1st 'value' is our free choice in Rules, and the 2nd 'value' is imposed by the definition of the widget. It may be less confusing if we picked a different name for the Rules 'value'.

You also said in #74

This checks to see if the context variable name ($context_name) is 'value' or 'operation', and handles things in a different manner.

That chunk of code checking 'value' is virtually a redundant piece I think, which tried (from one of the earlier patches) to derive the datatype from the field mapping. I got it running, but I don't actually understand what this was meant to achieve anyway, and it does not work either (as mentioned in the comment). If the field is a selector then we enforce 'textfield' as the input anyway. Was it supposed to alter the widget for the data comparison value input (if there was one associated with the selector). But that would not work until we knew what the user had selected in the mapping, and so would require a form refresh before entering the data. That does not seem right.

The bit looking at 'operation' was added by me as a reminder that this is where we could set a widget to give the options in a drop-down list. But maybe we need to give some more thought to where that should be done.

BTW, what is datetime_range and how does it differ from the core duration_iso8601 data type?

datetime_range is a widget, not a data type. Maybe you knew that, and you were asking "whats the difference between 'duration_iso8601' and 'timespan' datatypes?". My answer - I don't know. They were both listed in the core error message showing all possible datatypes when I tried to use a non-existant one.

#73.2, #73.3 and #73.4 are coded and working. I have reverted my attempt at #73.6 and I'm now going on to do the test changes as per #73.7

These posts are getting a bit long (!) but as you said above I think we are on the right track.

New patch with all changes #73.2, #73.3, #73.4 and #73.7
Regarding #73.8/#74 as mentioned before I don't think that section is needed (unless we discover otherwise, later) so I have commented out the part which was testing if ($context_name == 'value' && isset($configuration['context_mapping']['data'])) . The bit for "operation" was my rubbish suggestion and that's gone too. The operation/operator selection list drop-down will be the subject of another issue (probably when #2329937: Allow definition objects to provide options is done?)

I have managed to reduce the complexity in ContextFormTrait significantly now, as I had a lighbulb moment when looking at the example code in typed_data. I realised that Rules should not be attempting to get the input data directly from the form, but should get the widget to do it. The widget function extractFormValues() must be used in all cases, as this has custom code specific to each widget.

I also realised that Rules should not be setting the default values in the form directly, but we should do it via Typed_data, using $typed_data->setValue($default_value) before building the widget form. That simplifies the coding too, in addition to being the right thing to do.

There is one work-around still remainig. Rules (incorrectly, maybe) uses multiple = TRUE in a context definition to mean "we want a textarea input so you can add multiple items, eg several email addresses, or node types to check". However, core Typed Data is then expecting these values to be in an array, whereas we currently have them in a string split by newlines. This causes

InvalidArgumentException: Cannot set a list with a non-array value.
in Drupal\Core\TypedData\Plugin\DataType\ItemList->setValue()
(line 59 of core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.php)

It should be fixable, however may need a re-think on how we allow multiple values. Therefore I think it should be done in a separate follow-up issue, as this one is complex enough. So there is a @todo in function getContextConfigFromFormValues() where we avoid calling the widget's own functions and get the input directly.

There is another @todo (line 121) regarding making it easier to get the widget input names, maybe provide $widget->getInputNames() function, instead of deriving the names from the form. It might be possible to manipulate the form (lines 128-140) without needing the input name, so that is something we could work on.

I have used TR's suggestion from #71 to get Typed Data dev version via composer.json instead of patching via drupalci.yml.

Ignore the failed test in #78, I left some devel debug and my own d8testing module in the new test. The coding standards warnings are due to the commented out section.

Thanks, folks, for your work on this... looking great. Some thoughts on:

Rules (incorrectly, maybe) uses multiple = TRUE

I think there are (at least) two distinct cases here:

1. Some modules (Rules is not only one) use a single textarea to collect multiple (1-per-line) values, which may or may not then be split up into multiple actual values later, e.g. List field enumerations in core Field;

2. Some modules use a single textarea and want a simple text value - e.g. body text (email forms, descriptions, etc).

2a. and of course some modules could, plausibly, actually want multiple textareas!

While (1) is convenient it feels wrong to me and always has done so. In the 2010 era, with poor js & html, it was excusable, now not so much.

It feels as if on the cusp of D9 there is an opportunity to improve this area in-core and otherwise. Perhaps:

a) keep "multiple" to mean several distinct values, captured and delivered separately. In other words, not (1). Principle of least surprise?

b) introduce a new "data type" perhaps called "text_lines" which presents as a textarea and can take over from type=string/multiple=true? Perhaps it can deliver its values as an array of strings, one per line, not one string?

c) the UI for text_lines could perhaps be extended to emphasise the nature of the element - perhaps using multiple single line elements + DnD, or perhaps with some additional validation and inline help? I guess this would be a WiP!!

Thank you rivimey, that's the right type of discussion to be having. It bothers me (just slightly) in Rules ContextFormTrait that we alter the input #type to textarea for these cases. But I can see that this has been the acceptable expedient solution, and it works reasonably well. I like your a,b,c suggestions. I have also just found #2847804: Provide a generic multi-value handling widget which looks like the same kind of thing. Maybe we can move this discussion over there? That generic widget-of-widgets would allow not just multiple strings, but multiples of any data type.

Hi Jonathan, a multiple-widget sounds good as a first step, though the UX of it could perhaps be improved for specific types.

I would suggest that a generic multiple-widget was a good first step, better than we have now, but to build in the possibility to override that with a dedicated "multiple-of-X" widget should the UX of the generic solution prove to be poor.

I agree that 2847804 is a good place to move this part of the discussion. If this was done would the current patching be parked too?

If this was done would the current patching be parked too?

No I don't think we should hold up this widget integration just for the oddity of using multiple=true with a textarea. It has been done like that in Rules for ever, and can quite easily remain in place for now. It does not hinder the rest of the widget work, and this issue has been going on for so long, and we are so close to a solution (in TR's words, even before I made the improvements in #78) that we need to press on and get it completed. I will add a @todo at that section, referencing these comments. Thank you for your help and interest, and good suggestions.

Jonathan, sorry I didn't word my last comment well.
I didn't mean park the whole of the widget issue, just the historical multiple-lines behaviour. I agree with your summary in #83.

Textareas for "multiple" inputs is a short-term solution introduced in #2648300: Support multiple contexts in the UI. I subsequently improved the input handling quite a bit in #2855433: ContextFormTrait.php not processing multi-line conditions correctly when line break character is '\r\n', but this is still meant to be a stop-gap until options list handling can be put in. At which time, available options will be displayed in a multi-select select form element instead of using a textarea. I don't think we ever abused textarea like this in D7, it's here simply as an expedient because options list handling was stalled/blocked (still is) by lack of progress in core Drupal. (That's a totally separate issue I've been looking into recently - discussion in #2664280: Select lists in action & condition configuration forms).

There are only a few use cases in core for using textarea like this:
1) Choose one or more node types in the "Node is of Type" condition. This will be replaced by multi-select with an options list.
2) Choose one or more roles in "User has role(s)", "Add user role", "Remove user role", and "Send email to all users of a role". Again, this will be replaced by multi-select with an options list.
3) Enter one or more e-mails in "Send email". This is the only place that we would need a new multi-value textfield widget. (I picture this as something like the the "Related issues" input here in the issue queue, without the drag-and-drop reordering. Or perhaps we could simply reduce this to something like the "Issue tags" input here, where we just use a textfield to enter comma-separated email addresses.)

In the future, there might be other cases for inputting lists of values (which are not "multiple" contexts...) with each value needing an identical widget.

We will still need the "text" data type for long text that should use a textarea as input. For example, the body of an email.

To address #80:

a) keep "multiple" to mean several distinct values, captured and delivered separately.

Yes, that's what it's intended to be. Note that "multiple" is something defined by core Drupal contexts, and not something we added in Rules. Rules does extend the core @ContextDefinition, but only to add assignment_restriction and allowNull - we don't redefine what "multiple" means in core.

b) introduce a new "data type" perhaps called "text_lines" which presents as a textarea and can take over from type=string/multiple=true? Perhaps it can deliver its values as an array of strings, one per line, not one string?

I think what I said in 3) is the way to go - a new widget to use in the one case we need it in core Rules. I think that will be #2847804: Provide a generic multi-value handling widget, which could probably also be used for the separate but similar use case of entering list data.

c) the UI for text_lines could perhaps be extended to emphasise the nature of the element - perhaps using multiple single line elements + DnD, or perhaps with some additional validation and inline help

Yes, without the new data type, I think that agrees with what I'm saying.

I'm in the middle of a project and it's also a 4-day holiday here starting tomorrow (or really, in practice, already started) so it's going to take me a while to get around to looking at #79 in detail.

Hi TR, thanks for your comments and have a good break.

I mentioned "text_lines" because I can see a good reason for using a textarea for multi-value input in some situations, when the ability to easily traverse multiple values is important. It also seems to be a distinct data type from a string that happens to have line breaks, and from multiple completely independent strings. But I guess the benefit of typed_data is supposed to be that we can extend it :-)

Re the generic multi-value widget I agree that's a good way forward, but feel quite strongly that there should be a way to override the generic handling for a data type or use case with something specific.

Re (3) "Send Email", whatever solution is used, I expect there needs to be a way of "pasting" a bunch of email addresses (from, e.g., Excel) as a group, which is one of the reasons textarea works in this case. I expect something could be done with drag-drop and similar ?

Here is a re-roll following recent commits, to make patching and reviewing easier.

The new test ConditionsAndActionsTest also passes locally with patch #6 on #3092087-6: Deprecate 'context' in Rules @RulesAction annotation applied.

With the patch in #87 against latest dev I get an The website encountered an unexpected error. Please try again later. error. Watchdog entry is: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "text" plugin does not exist. Valid plugin IDs for Drupal\Core\TypedData\TypedDataManager are: entity_revision, entity_revision:backup_migrate_destination, ...... . This happens if I add the action "system - send mail" or if I edit an existing "system - send mail" action.

Hi Nchase, thanks for testing this. Yes the "text" data type is new, and was added in the Typed Data issue #3091923: Add a new "text" datatype for long text - see comments #63 to #69 above. You will need the latest -dev release of Typed Data API enhancements as that feature has not made it to a new release yet.

Let us know how you get on with it.

Hi jonathan1055, thanks for pointing the need of the latest Typed Data Api dev out.

It now looks good. Everything works. I edited an existing "system send mail" field and added additional lines. The received e-mail looks exactly like it should. I was also able to add a new "system send mail" action.

Great job!

Hi TR,
Please would you consider committing these two patches, which are part of my changes in #87 above.

The first patch (91a) simply moves ContextFormTrait from src/Form/Expression/ up one level to src/Form/, and adjusts the namespaces. I don't think there is much doubt that this change will form part of the full solution, and it would make things easier when merging the latest commits into itegrate-typed-data-widgets test branches. It will also make it easier for others to test the patch and give reviews/feedback. The current similarity index of the changed and moved ContextFormTrait file is 40% which is below the default 50% for finding renames.

The second patch (91b) adds the new test file ConditionsAndActionsTest.php. I have removed the assertions regarding checking the widget class, but everything else is exactly as per the patch in #87. This would give the curent codebase test coverage for adding and editing each condition and action. More detailed assertions can be added in due course but this gives a good framework for that to be done.

Sure, I can commit those. 91b is no problem, but I do have one question about 91a:

The first patch (91a) simply moves ContextFormTrait from src/Form/Expression/ up one level to src/Form/, and adjusts the namespaces.

What do you think about putting it in src/Context/Form instead?

Right now ContextFormTrait is only used by src/Form/Expression/ActionForm and src/Form/Expression/ConditionForm, so I guess putting it in that same directory made some sense to begin with. But @fago has said a number of times that he would like to see the whole context system separated off into it's own project. I'm not planning on doing that, BTW, but if we think of the context system as code decoupled from Rules proper then it would make more sense to put ContextFormTrait underneath src/Context. And indeed, looking at ContextFormTrait, I don't see anything in there that is specific to Rules, so to me it seems better just to put it under src/Context somewhere. And src/Context/Form seems like a reasonable place.

What do you think about putting it in src/Context/Form instead?

Yes that's fine with me and your's and fago's reasoning makes sense. I was just using it where it had been moved to, but it would be better to be under /Context/From. Do you need me to make a new 91a patch? I'm OK if you can just make that change/move and commit it, then I will re-do the full patch for this issue and test it all.

Here's a new patch for 91a. I also changed the help text slightly to remove the word "Rules", since there's nothing Rules-specific about this.

FYI, here's one issue which talked about separating Context: #2677098: [META] Provide Context related code as composer package

I think it's a good idea to do some of this reorganization of classes/tests internally to Rules, but I'm not a big fan of moving things into a separate project, or worse yet into a non-Drupal project available through packagist.

@jonathan1055: I just took a detailed look at the tests from 91b and am impressed - that's a very nice clean set of tests! The only comment I have is that it takes a long time to run, so perhaps split it into two files, one for Conditions and one for Actions? (They could share a base class if there is common setup between them.) That way the tests could run in parallel on the testbot. Right now the test takes so long that it's going to significantly increase the amount of time that Rules tests take to run, but if it is split it should affect the total time at all. Looking at the DrupalCI logs, as one long test it increases runtime about 15%-20%.

Committed #94.

Thanks for committing the move of ContextFormTrait, and pleased you like the new tests. They will be even better when they actually check the widget type being used in the form (which is already coded and exists in patch #87). There is more to add too. Yes, splitting into two is a good idea, I did not consider the time taken to run them.

Here's a patch which creates two new test classes ActionsFormTest and ConditionsFormTest. There is not much common set up for these which would make it worth having a separate base class. But on a separate issue we could move the account creation into RulesBrowserTestBase as that is used by these tests and others. The permissions are not identical in all tests, but that can be worked out (in a separate issue).

Committed #98. Thanks!

Excellent. It is much simpler now to merge new commits into this branch, and we also have expanded test coverage.

Here is a re-roll of #87.

I think I broke your patch with #3092087: Deprecate 'context' in Rules @RulesAction annotation and #3100995: Move RulesAction tests into RulesAction directory, and I know this is going to conflict with #3030295: 'context' deprecated in Rules @Condition annotation

No problem. I waited until the 3rd of those issues was committed then merged and re-rolled.

Re-rolled patch following recent commits.

Re-rolled after latest commits, no changes.

I have latest -dev of Rules and patch from #105 seemed to apply cleanly. When i go to Rules and try to add "Send email" action, site wsod with: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "text" plugin does not exist.

Hi liquidcms,
Thanks for testing this. The "text" plugin is provided by Typed Data API. What version of that module do you have? The new plugin was only added in the alpha4 release.

I had no version of that module; so that might explain it. I did however uninstall and re-enable Rules and there was no suggestion that module was a dependency.

I came upon this issue in my attempt to get Rules to send HTML Email (something used on every D7 site I have ever built). After finding a patch for mimemail to (recently) add this back, the issue became that the default rule for sending plaintext email (as well as mimemail's rule for sending html email) only provided a single text field for the body of the email. This patch (and the Typed Data module) now restores the body field back to a textarea for the Rule's rule for plaintext email. Sadly, this does not fix the body field for mimemail's html email body field. Perhaps another year. :(

Is it just me, or does anyone recall the D7 days when this fix would take 20 minutes? :(

and fixed in mimemail.. simply change @ContextDefinition in MimeMailAction.php annotation from string to text. Wil post patch to mimemail guys.

thanks for the years of work here guys.. but yikes..

I had no version of that module; so that might explain it. I did however uninstall and re-enable Rules and there was no suggestion that module was a dependency.

You must have had a very old version of the Rules code because you cannot install it now without Typed Data API, as it is a requirement in composer.json and rules.info.yml since 8.x-3.x-alpha2. But anyway, that's not the point here. At least you have shown that the patch works and e-mails now have the textarea widget. Thanks for testing it.

@liquidcms: There are many different hacks in #2724129: Change message field to textarea in Send email action that you can use to "fix" this problem within minutes - that issue was deliberately left open so that people could find it and use the patches to solve their immediate problems. But none of those are long-term solutions, and none of them will help improve Rules.

A proper fix, which is what this current issue is all about, will work for ALL current and future conditions and actions, not just the send mail action, and won't require any huge if/then/else cases to check for the data type then set the widget correspondingly - that model is just not scalable and not maintainable. That's also the method widely used in contributed modules (other than Rules) in D7, which is why there are so many D7 modules that are struggling to get ported to D8 - they are full of spaghetti code and compensation for PHP flaws.

The case with HTML e-mail is similar. It's easy and actually trivial to write a primitive HTML mail system - it's less than 10 lines of code. Maybe 30 lines if you include comments. There are literally dozens of modules that provide some form of HTML mail system for D8. But it's complicated and hard to fully support HTML mail with attachments, multipart messages, content types, etc. - that is why core Drupal does NOT support HTML mail and that is why this task is left up to contributed modules. It makes no sense at all for Rules to provide its own HTML mail capability - Rules does not have the manpower to support a full-functioned HTML e-mail system, and that is not one of the capabilities that Rules is designed for.

Instead, it makes way more sense to have a stand-alone module that does HTML mail properly. In D7 this was Mimemail and HTML Mail, neither of which is fully ported to D8. When they work in D8, Rules can easily support them. You say you've used one of these "on every D7 site I have ever built" so let me ask you - what have you contributed to porting either of those modules to D8? And how have you helped get Rules finished for D8? It is said that Drupal is not about what you get from it, but what you give to it. Your attitude seems to be the opposite.

So use the hacks if you want to get the functionality now, but I don't appreciate you denigrating the hard work (volunteer work) that a very few people have taken on to support >25% of all Drupal sites that use Rules and to port Rules to D8.

Well, some of liquidcms's comments were a bit flippant and mis-placed, but I don't think he meant to be critical of the work here. But yes it does wrankle when so few people are helping. On the positive side, he has confirmed that the patch in #105 works when 3rd-party modules provide RulesActions, so that is good to hear.

Getting back to focussing on the work done here, I have compared patch #105 back to patch #87 and it is essentially the same, there are changes to tests, re-rolls for other commits but no fundamental changes to the widget integration work. Rivimey has tested and confirmed OK in #80/82/84, Nchase confirmed OK in #90 and liquidcms confirmed OK in #106/108

There are four new @todo tags added by this patch, and these are:

1. in src/Context/ContextDefinition.php, public function getWidgetId($dataType)
   

   * @todo this function should be moved into the typed_data module. But whilst
   * still developing the integration of widgets in Rules it can stay here.
   

2. in src/Context/Form/ContextFormTrait.php, function buildContextForm()
   

   // Get the names of the data item input fields in the widget. Mostly there
   // is only one input field and often it will be called 'value', but some
   // datatypes, for example timespan, define two inputs. Remove all array keys
   // that start with # and this will leave just the input field names.
   // @todo Could this information be provided more easily, say by calling
   // $widget->getInputNames() ?
   $widget_input_names = array_keys($widget_form);
   $widget_input_names = array_filter($widget_input_names, function ($k) {
     return substr($k, 0, 1) !== '#';
   });
   

3. When checking if the widget form is already built as a fieldset (probably with multiple values) or whether we need to create the fieldset here (because the form is simple with one value)
   

   if (isset($widget_form['#theme_wrappers']) && $widget_form['#theme_wrappers'][0] == 'fieldset') {
     // @todo Is checking #theme_wrappers = fieldset the best way to do it?
     // Should we count the number of input items? What if the widget had two
     // or more inputs but was badly formed with no fieldset theme_wrapper?
   

4. In getContextConfigFromFormValues()
   

   if ($context_definition->isMultiple()) {
     // Remove the switch button then get the input directly. This is not
     // the right way. The problem is that Rules uses 'multiple = TRUE' for
     // textarea to allow multiple entry items. However, they have to be
     // in an array for the TypedData setValue to work without throwing
     // InvalidArgumentException: Cannot set a list with a non-array value.
     // @todo Fix this.
     // @see https://www.drupal.org/project/typed_data/issues/2847804
     unset($value['switch_button']);
     $input = reset($value);
   }
   

With the possible exception of (1) I was thinking that these were OK for the first commit of this work. Then they can be addressed in follow-up issues. The basic functionality now works correctly and is flexible without making any assumptions about how the value (or values) are named in the Typed Data widgets. I have also written tests that check each action and condition uses the correct widget for the datatypes specified.

So, is there anything stopping this issue becoming RTBC? It would be really really good to get this committed, then I can progress on with #2943253: Add form widgets for date fields which cannot be used until this issue is in. Third-party contribs can then use the calendar entry for setting dates (currently we only have a text entry for the numeric timestamp to enter a date!). I also have new conditions for Rules 'Date Comparison' and 'Date in Range' but that needs both of these issues.

Been very busy with google_calendar module and then other stuff but finally got back to this. I've updated my dev site to use rules -dev and typed_data -dev and the patch in #105, and it works !

Only thing I noticed not quite as expected was when entered a value considered to be a string into the mail body field. I got an error about "got string, expected text", which is clear but I would have expected automatic "upcasting" from string to text.

Thanks for all the work, folks. Well done.

Re: #113:

There's a @todo in ContextHandlerIntegrityTrait::checkDataTypeCompatible() that says:

// Compare data types. For now, fail if they are not equal.
// @todo Add support for matching based upon type-inheritance.

We should be able to change that to use an instanceof instead of an ==, and that should fix the text/string thing since text is a subtype of string. It won't fix things like integers that could be cast to string, but that can wait. I'll see if that works, and if so we can include it here.

I retested #105 and found it needed to be re-rolled after #2769511: Cache is not invalidated when Rules are changed got committed. Here's a new patch:

Yeah, testbot doesn't like fuzz. Here's a better version:

Thanks @rivimey for the feedback and @TR for the hint about improving this in ContextHandlerIntegrityTrait::checkDataTypeCompatible(). I've reworked that function and it now allows a string selected via data selection to be accepted for the text area. I actually used is_subclass_of() as I could not get instanceof to return a true value when needed. At first I thought it was giving the result the wrong way round from what we wanted, but then I realised that we need to check whether the 'expected' value (text in this case, as defined in Typed Data) is a subclass of the 'provided' value (string from the data selector). So the key new new check is if (is_subclass_of($expected_class, $provided_class)) then return without the exception. The value is accepted, and works, the e-mail is sent with the selcted value such as node.title.value or node.uid.entity.name.value resolving to the expected text.

The patch includes some debug in the form of $this->messenger()->addWarning() so that you can try this out and see the results. The interdiff makes it look like I have entirely re-written the function, but I have just re-worked it so that all the various OK cases return, then the exception is done at the end without being wrapped in a conditional.

Sort-of unrelated, but in re-working the messages I thought it was better to have the two data types emphasised, makes it more readable, so that meant a slight change to the tests, by removing the html tags in the string to test for. I think it is better to check just the text and not enforce the html tags in the unit test. But if you think this is wrong I will undo it.

Patch #118 still applies and passes. I will re-roll to remove the debug output, so that we have a comittable patch. As I said in #112 I believe that the four @todo can be addressed in follow-up issues, and that this work is RTBC. But I can't set my own patch to that, so it needs someone else (other than TR preferrably, who already knows this code) to test it and say yes. Then we can commit, move on, and address the other existing issues which depend on this one. Thanks.

Debug removed from #118. I think this is ready.

I won't comment again on the patch - would be better if someone else did so, but thanks for all your perseverance on this one :-)

Code review

+++ b/src/Context/ContextDefinition.php
@@ -2,6 +2,8 @@
+// Use Drupal\Core\Extension\ModuleHandlerInterface;
+// Currently have \Drupal::moduleHandler() instead.

I see commented out code: I think this should be removed?

Manual review

This could be totally unrelated - I assume it should be handled in #2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields - but after applying the patch I got the following fatal error when editing an action of type "System > Send email":

InvalidArgumentException: Cannot set a list with a non-array value. in Drupal\Core\TypedData\Plugin\DataType\ItemList->setValue() (line 59 of core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.php).

To reproduce:

1. Add a reaction rule for the event "After saving a new content item".
2. Add action "System > Send email".
3. For "Send to", switch to data selection, set "Data selector" to "node.uid.entity.mail".
4. Set values for the other parameters and save the action.
5. Edit the action again.

You then get the specified error. I didn't get that error before applying the patch. And note it is a fatal error - the user cannot correct the error in the UI, only when editing the YAML config file. So that's why I found it worth to mention here.

Do note that the following error occurred when creating a new node - before and after applying the patch:

TypeError: Argument 1 passed to Drupal\rules\Plugin\RulesAction\SystemSendEmail::doExecute() must be of the type array, object given

I assume the error should be handled in #2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields. Is that correct? Or should we add a test here to catch that InvalidArgumentException upon editing an action and display an error message in the user interface instead?

This adds a test and a fix for the case from #122.

When the context definition has 'multiple = TRUE', $default_value is casted to an array if it is scalar.

Not sure if it is the correct fix. I did try in the YAML to set an array for a context_mapping item, but that failed on a config import with the following error:

The configuration property expression.actions.actions.0.context_mapping.to.0 doesn't exist.

Portion of the YAML code that I tried to import:

context_mapping:
  to:
    - node.uid.entity.mail

So I assume each item in context_mapping should always be a string. This assumption looks like to be confirmed by rules.context.schema.yml:

  rules.context.mapping:
  type: sequence
  label: 'Context mapping'
  sequence:
    - type: string

I left in the following in the patch, because upon further inspection it looks like it could be intentional:

+++ b/src/Context/ContextDefinition.php
@@ -2,6 +2,8 @@
+// Use Drupal\Core\Extension\ModuleHandlerInterface;
+// Currently have \Drupal::moduleHandler() instead.

Hi MegaChriz,
Thanks for your review and the questions and patch regarding upcasting the scalar value to array.

I am testing your patch with the Send Mail action, using data selector with node.uid.entity.mail as per your steps in 122.3. On triggering the rule I get error "Argument 1 passed to Drupal\rules\Plugin\RulesAction\SystemSendEmail::doExecute() must be of the type array, object given in Drupal\rules\Plugin\RulesAction\SystemSendEmail->doExecute()". This is true, because the node.uid.entity.mail is an object.

I thought you may have mis-typed it, and meant node.uid.entity.mail.value but this is not accepted on saving, and I get the validation error "Expected a list data type for context Send To but got a email data type instead". So this needs working on.

Maybe it should be left to be sorted on #2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields and the solution could well be something like your changes for patch 123. However, it is not critical for this issue (yes I know you get a fatal error, but is it actually caused by the new changes made here?). We are close to getting this committed so lets not add new complications now, which will just delay it. When this is in, it will be so much easier to work on the related issues.

I have tidied up the commented out code regarding moduleHandler, and added it to the @todo for that function.

@jonathan1055
I'm new to the D8 version of Rules, but I have used it for D7.

I could reproduce the fatal error I experienced also in the following way:

1. Add a reaction rule for the event "After saving a new content item".
2. Add action "Add a variable"
3. Set "Type" to "list", leave "Value" empty and click "Save".
4. Add action "System > Send email".
5. For "Send to", switch to data selection, set "Data selector" to "variable_added".
6. Set values for the other parameters and save the action.
7. Edit the action again.

The error does not occur without this patch. My patch from #123 fixes the error also for the scenario above.

I do agree that we should avoid adding new complications when not necessary, but based on my observations so far it does look like this is caused by the new changes made here.

Come to think about it more, I do wonder if the following is correct:

+++ b/src/Context/Form/ContextFormTrait.php
@@ -53,17 +51,76 @@ trait ContextFormTrait {
+    // Set default before building the form, so that widget->form() can use it.
+    $typed_data->setValue($default_value);

If the default value is coming from a 'context_mapping' item - which is always a string if I'm not mistaken - then does it make sense at all to pass that to the TypedData object? I'm thinking now that my patch fixed a symptom of the issue: it fixes the case where $typed_data->setValue() expects an array. But I suppose there are cases where something other than a string or array is expected?

I'm thinking now that my patch fixed a symptom ...

Yes you are absolutely right. The problem exists in the current dev code, and the work in this issue simply means that we see a different symptom. With no patch, we can add node.uid.entity.mail and this saves ok, and the action can be re-edited. However this is the wrong selection, and at trigger time we get "Argument 1 passed to ... SystemSendEmail::doExecute() must be of the type array, object given". The correct value is node.uid.entity.mail.value but that fails the integrity check. This must be fixed on #2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields where I have added a functional test similar to yours in #123 above.

However, to assist us in developing on both of these issues, I agree that it is a real PITA when, after adding and saving node.uid.entity.mail and discovering it is wrong, you cannot re-edit the action. The only solution is to modify the annotation temporarily to 'multiple=FALSE' or just delete the action and re-add it. So I'm suggesting to temporarily add your scalar to array conversion

+    // Temporary fix: Cast default value to an array if context definition has
+    // 'multiple = TRUE' so that the action can be edited without a fatal error.
+    // @todo Remove when integrity check prevents the wrong type being saved.
+    // @see https://www.drupal.org/project/rules/issues/2723259
+    if ($context_definition->isMultiple() && is_scalar($default_value)) {
+      $default_value = [$default_value];
+    }

with a @todo stating that this is temporary, to prevent a fatal error, and to remove it when #2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields is solved. This will save a lot of hassle when testing this issue. Also fixed a couple of comments.

@jonathan1055
Note that this is not only about node.uid.entity.mail. A custom variable of type “list” - for example named variable_added - also failed (see #126). Which made me wonder: is it correct to pass the context mapping to $typed_data->setValue()? Is something like $typed_data->setValue(‘variable_added’) even correct? Or should only literal values be passed to the TypedData object?

Yes I know this is not only about node.uid.entity.mail and yes I tried your example in #123, and that is why I added the temporary casting to array to help us while developing/debugging. You will also note that without this Typed Data work you can't even get as far as adding the list variable action.

Which made me wonder: is it correct to pass the context mapping to $typed_data->setValue()?

Yes that has to be done in all cases, it is vital. Otherwise the form has an empty value instead of the previously entered selector.

Now that you are working on #2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields I think we have answered the questions on this issue, and it should be RTBC.

Patch #131 is #128 re-rolled following recent commits. However, the new assertions in #3160067: Refactor and expand ActionsFormTest and ConditionsFormTest cause failures for the two List actions and conditions.

This should be resolved in #2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields and patch #131 will fail, but adding it so that others can use it for testing.

This patch I am posting solved the following problem on Drupal v8.9.6:
Fatal error: Class Drupal\rules\Context\EntityContextDefinition contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (Drupal\rules\Context\ContextDefinitionInterface::getWidgetId, Drupal\rules\Context\ContextDefinitionInterface::getWidgetSettings) in /app/web/modules/contrib/rules/src/Context/EntityContextDefinition.php on line 21

Hi mr.valters,
Thanks for your patch. However, it would be helpful if you posted a comment, instead of just adding a patch. I only found it by using the version history https://www.drupal.org/node/2471481/revisions/view/11999508/12068969
Could you also provide an interdif so we can see what changes you have made. Also check my comment in #131 as I knew that patch #131 would fail following the expanded test coverage added in #3160067: Refactor and expand ActionsFormTest and ConditionsFormTest I'm not sure what error you are fixing, or whether it worked, as you did not queue the patch for testing. Thanks for your interest though, please do continue.

[edit: After saving, I can see that comment #133 may exist but is empty, the numbering has jumped from #132 to #134]

mr.valters is not a confirmed user yet, so his comment isn't published. I'm not sure what the procedure/rules are with unconfirmed users on drupal.org, and I'm not sure why the patch is visible but the comment isn't. I assume this will be sorted out by the drupal.org webmasters sometime in the next few days.

But yes, we definitely need an interdiff.txt to see what was changed from #131 and an explanation of why a new patch is needed.

I think what mr.valters patch was adressing, according to the error message shown, is the change caused by #3161582: EntityContextDefinition breaks the context system. The patch also appears to contain other reverse changes (ie may be based on a state earlier that 131 ?), and some coding style changes which cause 34 new coding warnings.

I had actually already fixed the EntityContextDefinition requirement locally, but not posted a patch here, so here it is.

Also included in this patch is the alignment of the ActionsFormTest and ConditionsFormTest as done in #2664280: Select lists in action & condition configuration forms so that these tests should pass here. I have posted two interdiffs, one without the test changes, to show what is actually new in this patch

[edit: I know that this issue is not top on TR's list, so not asking for any review. But I wanted to get the patch done and tested here]

Thanks for the patch, reviewed the patch and here some points:

1. +++ b/src/Context/ContextDefinition.php
   @@ -108,6 +108,13 @@ class ContextDefinition extends ContextDefinitionCore implements ContextDefiniti
   +   * @var array
   

   @var string[]
   will better fit here

2. +++ b/src/Context/ContextDefinition.php
   @@ -182,4 +189,80 @@ class ContextDefinition extends ContextDefinitionCore implements ContextDefiniti
   +   * @return array
   

   The same here

3. +++ b/src/Context/ContextDefinition.php
   @@ -182,4 +189,80 @@ class ContextDefinition extends ContextDefinitionCore implements ContextDefiniti
   +    return isset($this->widgetList[$dataType]) ? $this->widgetList[$dataType] : 'broken';
   

   a null coalescing operator will be useful here

   also, the 'broken' magic string would be better replaced with the constant value.

4. +++ b/src/Context/ContextDefinitionInterface.php
   @@ -70,4 +70,24 @@ interface ContextDefinitionInterface extends ContextDefinitionInterfaceCore {
   +   * @return string
   +   *   A string with the widget id. Will return 'broken' if the data type needed
   +   *   by the context is not supported by any widget.
   

   Also, mention of the string here which would be better to store as the constant.

5. +++ b/src/Context/ContextHandlerIntegrityTrait.php
   @@ -129,26 +129,32 @@ trait ContextHandlerIntegrityTrait {
   +    if ($expected_type == $provided_type) {
   

   why not use a strict type comparison operator here?

6. +++ b/src/Context/ContextHandlerIntegrityTrait.php
   @@ -129,26 +129,32 @@ trait ContextHandlerIntegrityTrait {
   +    if ($expected_type == 'any' || ($expected_type == 'entity' && strpos($provided_type, 'entity:') !== FALSE)) {
   

   same here, obviously that variable needs to contain string type value.

7. +++ b/src/Context/Form/ContextFormTrait.php
   @@ -53,17 +51,84 @@ trait ContextFormTrait {
   +    if ($widget_id == 'broken') {
   

   magic string, let's use a constant

Thanks for the review, voleger. I've done all the things you suggest. I also made a change to .travis.yml and a re-alignment of 'Ban Ip' in actionsFormTest to match the current dev code, but I have removed these from the interdiff so you can see more easily what I have changed following your review.

Thanks looks much better

+++ b/src/Context/Form/ContextFormTrait.php
@@ -65,10 +65,10 @@ trait ContextFormTrait {
     $widget_id = $context_definition->getWidgetId($dataType);
 
-    if ($widget_id == 'broken') {
+    if ($widget_id == ContextDefinitionInterface::BROKEN_WIDGET_ID) {
       // The datatype is unknown and/or the typed-data widget has not been coded
-      // yet. Use 'broken' which by design has no input field.

The one thing left is to make a strict comparison here.

Re-roll (added strict comparison from #139 and removed .travis.yml changes based on latest dev)

Thanks Daniel Korte for the re-roll, all good.

I've made a further small change, as I realised that the widget class should not be added when the entry is by selector as it makes no sense, and is misleading. It is only relevent when the entry mode is direct input. This is a simple change in ContextFormTrait buildContextForm() and also requires a couple of adjustments in the ConditionsFormTest. (it was actually when working on the tests that I found the problem). In "Data Comparison" the data item is always by selector, so should there should be no 'data'=>'text-input' widget check. Likewise for "List Contains" the list is always by selector, so there should not be any 'list' => 'textarea' widget check.

I also noticed that in "Entity field access" the user context can be left blank and will default to the current user, so this item has been moved from the 'required' to the 'defaulted' parameter array.

#141 worked for me.

Conflicts with #2800749: Support upcasting entity IDs to full entity contexts patch #68, but #141 also solved #2824360: Error "Call to a member function id() on string" for condition 'user has role(s)' so all good for me.

Also, using the patch to prevent infinite recursion when setting a data value.

  "Send textarea field for send email to users of role": "https://www.drupal.org/files/issues/2020-12-09/2471481-141.integrate-typed-data-widgets.patch",
  "Stop infinite recursion when saving data value": "https://www.drupal.org/files/issues/2020-09-20/3172088-2.patch"

Hey folks just coming back to this after a while... thought it would be all over by now ! :-)

Can't help feeling this should be wrapped up RSN and further tweaks left for further issues. @voelger was 99.99% happy in #139, @Chris Happy has noted he is good, as has @jonathan1055, and the tests are passing, so I'm going to take the plunge and mark RTBC.

Last patch was applying properly to the latest dev due a commit that happened yesterday whereby a property was changed. The commit in question can be found here:
https://git.drupalcode.org/project/rules/-/commit/cc5ba05040c548da590cab...

Basically the property `pos` was renamed to `position`. This made the patch no longer apply. I've updated the patch to reflect the new change and it'll properly now.

Exported the patch again as the last one failed to apply during the test.

Updating to apply to latest dev

Still seems to be working with the latest dev and Drupal Core updates! Fingers crossed this gets committed.

Patch no longer applies for D9.2.10 ??

OK, I will re-roll. There have been some changes in 8.x-3.x which affect this.

@Rar9 It is not the core version which causes the re-roll, it is new commits to the dev version of Rules.

Patch 150 is a re-roll due to #3243962: Prepare for PHP 8.1 and #3252568: Use @inheritdoc where appropriate

@jonathan1055 thanks so much for the reroll we now have all 3 patches working together:

"drupal/rules": {
                "Event Listener": "https://www.drupal.org/files/issues/2021-12-07/rules_registers_no_l-2816033-90.patch",
                "Support upcasting entity IDs to full entity contexts": "https://www.drupal.org/files/issues/2021-04-29/2800749-88.upcast.patch",
                "Integrate Typed Data Widgets": "https://www.drupal.org/files/issues/2021-12-06/2471481-150.integrate-typed-data-widgets.patch"
            },

Also great to see so many fixes being added to Rules core!

Thanks for the feedback @glynster, it's good to hear that those three patches do not conflict with each other.

I guess you are/were patching the 8.x-3.0-alpha6 version, because the 88.upcast.patch needed a re-roll for the latest dev, post alpha7, which I have now done on #2800749-97: Support upcasting entity IDs to full entity contexts. I have also made a patch which has no test file changes, to reduce the conflicts with other patches. Hope that helps when you next move forward. If you get clashes let me know, because if they are within the test files I can do the same thing and produce a -no-test version here too.
Jonathan

Hi @jonathan1055 we are on the latest dev version actually. With your latest reroll #2800749-97: Support upcasting entity IDs to full entity contexts the standard patch failed, however using the no-tests version worked perfectly.

Let me know what else you need from me and as always you guys are awesome!

@glynster thank you. yes using the no-tests version worked. the other did not. I am still on Drupal 8.9.x

@jonathan1055 great news on the upcasting getting committed! You guys have put so much hard work in there! Do you think we are close to getting this patch committed as well?

Due to several commits recently this needed a major merge and re-roll. However, the resulting patch has dropped from 1500 lines to only 930, as many of the test changes I introduced here have now been committed.

I have also rolled a 'no-tests' version of the patch for those who want to ru this alongside other Rules patches, in a hope to minimise clashes.

Unfortunately some tests now fail where they passed before. These are all with actions and condititons that contain a language entry field. There are problems rendering the language object and I have not determined which of the other recent Rules commits has caused this problem. I have made one (temporary) fix in ExpressionContainerFormBase.php which was the cause of the first problem, to allow the action or condition to be saved with a language enteted. But when this is re-edited we get a second exception, so more investigation is required

Failure seems to be because of #3255559: Re-label entity create action derivatives which enhanced the labels of entity actions. The fix should be a simple text change in the test cases.
Actually, I don't think this is the case. I spoke too soon.

I missed one change to the new RulesComponentListBuilderTest. Patch #159 fixes this.

But this is not the cause of the Language problem. I have eliminated any core change as the reason, because I can replicate the problem even at D8.9. Going to go through the commit log since 6 Dec as that was when I submitted the all-green patch #150.

The root of the problem is not caused by any later commits, it was there all along! The tests passed at #150 because every language was specified as 'language' => '?'. I have now realised that any invalid langcode simply gets ignored silently and nothing is saved for the language, hence there were no test failures. If any of those languages are set to 'en' or 'und' (which are the valid langcodes loaded currently in the tests) then we get the failures. This is what was done in #3254675: Improve ActionsFormTest and ConditionFormTest. I have proved this locally, by checking out the 8.x-3.x branch as at 6 Dec, applying patch #150 and the tests pass. Then change to 'en' or 'und' and the tests fail. You can see this interactively too, but I guess no one has used the language input settings very much during manual testing.

So now we need to find out what is wrong with the language and why it cannot be rendered like everything else. After using the fix shown in the interdiff 150 - 156, the parameters can at least be displayed. But editing the condition or action produces

PHP Fatal error:  Method Drupal\\Core\\Template\\Attribute::__toString() must not throw an exception, caught Error:
Call to undefined method Drupal\\Core\\Language\\Language::render()
in /Library/WebServer/Web/drupal89dev/sites/default/files/php/twig/... 

After a lot of work I have found what could be a reasonable solution here, although it is not ideal. Instead of using the plain "language" context definition we can use "entity:configurable_language". This solves all three of the problems I discovered when trying to enter a language code:

1. We avoid the fatal error "Object of class Drupal\Core\Language\Language could not be converted to string" in ExpressionContainerFormBase->getParameterDescription() because the context is stored as a plain langcode string. Our new upcasting functionality automatically converts this to the required language object when the rule is triggered. I have therfore removed the temporary code added to ExpressionContainerFormBase.
2. The entry form field gets a proper default value when re-editing, and we avoid the php error Method Drupal\Core\Template\Attribute::__toString() must not throw an exception, caught Error: Call to undefined method Drupal\Core\Language\Language::render() in /Library/WebServer/Web/drupal89dev/sites/default/files/php/twig/...
3. The rule can be saved sucessfully, and we do not get the error
   

   Drupal\Core\Config\UnsupportedDataTypeConfigException: Invalid data type for config element rules.reaction.condition_rules_path_has_alias:expression.conditions.conditions.0.context_values.language
   in Drupal\Core\Config\StorableConfigBase->validateValue() (line 160 of core/lib/Drupal/Core/Config/StorableConfigBase.php).

The ConfigurableLanguage class is provided by the core Configuration Translation module, so is available for any site that wants to use language criteria in the conditions or actions. One downside is that if this module is not enabled then the conditions and actions which use a language will fail. So it would mean either adding config_translation as a module requirement, or making it optional and disabling the those actions and conditions if config_translation is not enabled.

It all works ok on a real site, and the functional and kernel tests are now all passing. However, I have not worked out how to load/mock the configurable_language plugin for the unit tests. So those still fail for the bits which add a language. Maybe that could be moved into a Kernel test?

Anyway, let's see how this runs on the testbot.

Added a mocked configurable_language entity into RulesEntityIntegrationTestBase. No change to the functionality, only the tests. All should pass now.

Nice work.

Minor re-rolled of #163 because of changes from #3253968: Remove iterator_count() from tests

The root of the problem is not caused by any later commits, it was there all along!

Thanks for finding and identifying this problem with language contexts. I feel that this is a big enough problem that it probably deserves its own issue, rather than delaying this one trying to fix it? What you you think?

Thanks for the re-roll.

... probably deserves its own issue, rather than delaying this one trying to fix it? What you you think?

Yes I am not sure what the "proper" solution might be, or even if there is a problem to solve. But that would be in a separate issue. However, for this issue are you OK with the solution I have found, to use "entity:configurable_language" context definition instead of just "language". It is a simple change and solves all three of the errors as explained in #162.

Regarding the new dependency on core Configuration Translation module, we could make that optional. If it is enabled then Rules will accept a language selection in the conditions and actions that need it. But if that core module is not enabled then the language field/widget would be disabled and set to null. That would avoid the UI and runtime errors, but not add a new mandatory dependency. I would guess that if a site wants to specify a language in the condition or action then it is very likely that Config Translation would be already enabled, or it would not be too much of an overhead to enable it.

I'm a downstream user of these patches ... after updating to patch 163, I have indeed been bitten by the language stuff. My existing rules don't get fired, and if I try to edit a rule's action (send email) or create a new action, I get the PHP error:


Drupal\Component\Plugin\Exception\PluginNotFoundException: The "entity:configurable_language" plugin does not exist. Valid plugin IDs for Drupal\Core\TypedData\TypedDataManager are: filter_format, current_date, current_path, site, text, entity, entity:backup_migrate_destination, entity:backup_migrate_schedule, entity:backup_migrate_settings, entity:backup_migrate_source, entity:block, entity:block_content, entity:block_content:basic, entity:block_content_type, entity:comment, entity:comment:comment, entity:comment:workflow, entity:comment_type, entity:contact_form, entity:contact_message, entity:contact_message:feedback, entity:contact_message:personal, entity:editor, entity:environment_indicator, entity:field_config, entity:field_storage_config, entity:file, entity:filter_format, entity:image_style, entity:menu_link_content, entity:menu_link_content:menu_link_content, entity:migration, entity:migration_group, entity:node, entity:node:article, entity:node:dataset, entity:node:ontology, entity:node:organisation, entity:node:page, entity:node:register, entity:node:vocabulary, entity:node_type, entity:path_alias, entity:rdf_mapping, entity:rest_resource_config, entity:rules_reaction_rule, entity:rules_component, entity:search_page, entity:shortcut, entity:shortcut:default, entity:shortcut_set, entity:action, entity:menu, entity:taxonomy_term, entity:taxonomy_term:registry_item_status, entity:taxonomy_term:tags, entity:taxonomy_vocabulary, entity:tour, entity:user_role, entity:user, entity:pathauto_pattern, entity:view, entity:date_format, entity:entity_form_display, entity:entity_form_mode, entity:entity_view_display, entity:entity_view_mode, entity:base_field_override, entity_reference, field_item:comment, field_item:contact_storage_options_email, field_item:datetime, field_item:file, field_item:file_uri, field_item:image, field_item:link, field_item:list_float, field_item:list_integer, field_item:list_string, field_item:path, field_item:text, field_item:text_long, field_item:text_with_summary, field_item:boolean, field_item:changed, field_item:created, field_item:decimal, field_item:email, field_item:entity_reference, field_item:float, field_item:integer, field_item:language, field_item:map, field_item:password, field_item:string, field_item:string_long, field_item:timestamp, field_item:uri, field_item:uuid, any, binary, boolean, datetime_iso8601, duration_iso8601, email, float, integer, list, language, language_reference, map, string, timespan, timestamp, uri in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of
/web/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).


To be able to edit my existing rule actions or add new ones, I didn't need to enable the "Configuration Translation" module; it was enough to enable just the "Language" module.

(I should add: when I created the rules in the first place, I'm pretty sure I did so in a system that didn't have the "Language" module enabled. It seems the rule configurations get language stuff (e.g., langcode: en and expression.actions[].context_values.language: null) put in there anyway.)

I was eventually able to get my rules to fire again by disabling and re-enabling (just) one of them ....

Anyway, it seems that the Language module may now be (is?) required if this patch is used.

Hi richard.walker.ardc,
Thanks for that info, that is really helpful. Yes I have just tested and confirmed what you have said, that enabling the 'language' module is enough. I'm not sure exactly why I thought that the configurable_language plugin was provided by the Configuration Translation module. Each of the three 'translation' modules require 'language' so enabling any of them would have also enabled the 'language' module, maybe that confused me.

The data selection when picking a language context value was not allowing any language to be selected. As shown in #3263463-6: Cannot add language to send mail action we get an error for each possible selection of a type of language field. However, when using the typed data widgets, we can accept a 'language_reference' field as a valid selection, and the rules trigger correctly as intended. Here's a patch so that this can be tested manually.

I also changed the tests so that they install 'language' (the base module) instead of 'configuration_translation' as this should reduce processing overhead and is clearer when reading the test code.

As before, I have also added a 'no-tests' patch file, in case you are already running on a site with Rules patches, this will reduce the chances of conflicts.

Following discussions with TR on Slack about requiring the 'language' module, and the multiple problems caused by trying to use the widget for the core language or language_reference data types, and whether we should chnage language to just a plain text field, I realised that the widget for language gave us no increased functionality or any better ui. So here is an updated version which does not require the language module but still has all the language integrity checks and does not diminish the testing or usability. We now have a new id 'NO_WIDGET' which keeps the form element exactly as it is now. This is set for the language types, but is completely generic and can be used for any future or existing data type.

The interdif files are split into two, part a shows the additional code for the 'no-widget' option. Part b shows the removal of the various bit added earlier to cope with language

+    // Return the widget id for this data type. If none, default to 'broken' id.
+    return $this->widgetList[$dataType] ?? self::BROKEN_WIDGET_ID;

Do we need to have both BROKEN_WIDGET_ID and NO_WIDGET? Perhaps, we get rid of BROKEN_WIDGET_ID, then we can leave language and language_reference off the list and have ANY datatype not found default to NO_WIDGET? That would preserve the existing behavior for all datatypes that aren't listed, not just the language ones.

It's an idea, and you have prompted me to think more about it, but I have come to the conclusion that it would not be as simple as effectively merging the 'broken' concept (this data type has not been catered for in Typed Data widgets yet) and 'no widget' (where for Rules purposes we have decided not to use a widget). There are places in the context form where we differentiate processing for a broken widget (such as not adding a switch button) where we definitely do need to do this for the 'do not use a widget' scenario. I then realised that it should only be in Rules where we are making the decision that languages work better without a form widget. Other modules that may implement Typed Data Widgets in the future might have different requirements and configurations and might very happily use the widget. So Rules should not make the unilateral decision for everyone.

So we need to keep the 'broken/undefined' vs 'do not use' concepts separate, but it should be just the Rules module that sets 'do not use' for the elements we chose. This is easy, and I've coded it locally ansd works with my tests, so I'll post an updated patch here. When the function definition of getWidgetId() moves into Typed Data then it might be that Rules has an over-riding version of this which sets 'no widget' for the languages but calls the Typed Data function for everything else. But for now I have just coded this in the ContextFormTrait.

Here are the patches. I have expanded the ActionsFormTest and ConditionsFormTest to cover language input via data-selector as that was not covered before, and was helpful when I was making changes.

@josephgut check to see if you're affected by issue #2816033: Rules registers no listeners on rare occasions..

@josephgut
In my case combining the patch here with the patch from #2723259-23: Allow single-valued data selector input to be passed as an array for 'multiple' context fields helped with both configuring and sending email. I had to adjust the patch from there a bit in order to make it applyable on top of the patch in #174.

Uploading a patch rerolled for the 4.x version. I pushed the changes to the 2471481-integrate-typed-data-4.x branch as well. It's not letting me open it as an MR right now, but I may at some point in the future. In the meantime here's the patch file.

#180 does not apply in 4.0.0. Is there more up to date advice for how to make this work?

I have created a merge request with the existing code in the issue fork.

This is the patch in the merge request re-rolled to apply to 4.0.0.