Implement assignment restriction in plugin context

Confirmed that 'assignment_restriction' doesn't seem to have any effect - direct input mode is *always* the default, regardless of 'assignment_restriction', and 'assignment_restriction' doesn't force a mode to be used like it did in D7.

I'm bumping the priority because this is causing recurring issues like #2998793: Node is of Type error.

Added 'Needs test' tag because we really should have a test to make sure 'assignment_restriction' works, as part of the fix.

Whilst investigating #3047684: List Contextdefinition must be multiple @TR suggested that it would be solved by getting the Assignment Restriction code working, because then the switch button should not be displayed in those cases. And this issue seems a good thing to do anyway, as this feature has not been implemented yet in the context form, but the background settings and functions are all done.

One test had to be altered because it was pressing the button to switch to data selection whereas now with the implementaion this conditon is restricted to data selection anyway. There was even a @todo by the button press saying "this should not be necessary once the data context is set to selector by default"

Because there is another test named testMultipleContextAction I made a minor change to the test name from testMultipleContext to testMultipleInputContext to make it easier to use -filter= when running phpunit.

This patch does not make any changes to the current settings in the context definitions, it just implements the feature in the form. To assist in manual testing here are the places we need to check:

• Conditions which already have a restriction: entity is of bundle, node is of type
• Conditions which have a @todo: data list contains, data list count
• Conditions which might also benefit: Entity is of type
• Actions which already have a restriction: data set, entity fetch by id, variable add
• Actions which have a @todo: data convert, data list item add

I know that this feature will need tests to prove the setting is working, but here are the code changes, and so far it seems to work.

Nice! Thanks for working on this. I think it will be a big usability improvement, especially with the "Node is of Type" condition where people keep tripping over this.

One test had to be altered because it was pressing the button to switch to data selection whereas now with the implementation this condition is restricted to data selection anyway. There was even a @todo by the button press saying "this should not be necessary once the data context is set to selector by default"

Yup, I put that @todo in there when I wrote the test (see #2855433: ContextFormTrait.php not processing multi-line conditions correctly when line break character is '\r\n') because it did not default to 'selector' like it should have.

Because there is another test named testMultipleContextAction I made a minor change to the test name from testMultipleContext to testMultipleInputContext to make it easier to use -filter= when running phpunit.

Good idea, I hadn't noticed that.

I know that this feature will need tests to prove the setting is working, but here are the code changes, and so far it seems to work.

It's very nice! The tests shouldn't be too hard - we just have to check that the button doesn't appear and the correct mode is chosen when the assignment_restriction is set, and that the button does appear when assignment_restriction is not set. And we will also have to go through all the conditions and actions to ensure that assignment_restriction is used in exactly the same places as 'restriction' was in D7. I've found that a number of the condition/action parameters did not get ported properly, they were just left off, so I've done several rounds already of comparing and correcting the D7 condition/action definitions against the D8 annotation. (I have one patch in the works for missing descriptions in the annotation - that should get committed within the next few days.)

Here's a modified patch where instead of using the hardwired strings 'selector' and 'input' I used the constants ContextDefinitionInterface::ASSIGNMENT_RESTRICTION_SELECTOR and ContextDefinitionInterface::ASSIGNMENT_RESTRICTION_INPUT. This is something I've had fixed locally for a while, it just didn't seem important enough to open a separate issue just to do this. But as long as we're working on the assignment_restriction, we should do this now.

Give me a few days to do some manual testing and convince myself this is working ...

Yes, good idea to use this patch to change to the constants. It also showed that we were using the Core ContextDefinitionInterface not the Rules extension of it, so that's good to fix too.

I will work on the tests.

Here's a patch with a new test. The code in ContextFormTrait is not included so this will fail. I used actual conditions and actions provided in the Rules model for the examples. I did look at the ones provided in the rules_test module, but none had restrictions defined, and when I added restrictions it caused existing tests to fail (which of course could have been modified to cater for the change in default for 'selector' restrictions but I thought that was making more unnecessary work). Other existing tests use the actual conditions and actions provided by Rules. If we find that the restrictions for these four contexts have to be changed in future it will be easy enough to adjust the new test. I picked ones that are not likely to change.

As expected the patch in #10 failed. Even though it stops at the first failed assertion I have tested this locally, commenting out assertions, and it fails for the other assertions too.

Here's the new test plus implementation as per patch #8

I like this very much. I think the choices you made in #10 were correct. Just a few minor things and I think it's ready to commit. Thanks for all your work on this!

In the test, you use $this->expressionManager, $this->storage, and $this->account but none of those are defined as class variables - this only works because PHP allows you to dynamically define class variables like this, but best practice would be to add protected $account for instance to the ConfigureAndExecuteTest class definition, with appropriate documentation comments. While changing $account to $this->account is useful because it is used in more than one test method (and thus should be in the class definition and initialized in the constructor), I think $expression_managerand $storageshould probably be defined as local variables, rather than using $this>expressionManager and $this->storage because those are not used in other test methods.

Otherwise, I don't see any problems with the patch.

I like this very much.

Thank you.

best practice would be to add protected $account for instance to the ConfigureAndExecuteTest class definition

Yes, you are right, I was being lazy.

$expression_manager and $storage should probably be defined as local variables, rather than using $this>expressionManager and $this->storage because those are not used in other test methods.

Yes I agree. I do use those in the new Typed Data Widgets tests, but you are right that they should be local here. Then if I use them in a new subsequent test I can move them.

I guess in general for things like this, it depends on how many tests use it (and thus would benefit from it being a class property) vs how many tests do not use it (and thus it is an unnecessary overhead when running that test).

New patch and interdiff for review.

OK, committed! Thanks - this is going to help a lot towards reducing the support requests, because it will now be a whole lot more difficult to enter an invalid value when configuring conditions and actions.

Thanks for committing. Yes it should help a lot.

Unassigning.