When #2804941: Implement assignment restriction in plugin context gets committed, assignment restrictions will actually start to work. The next step is to ensure that all Conditions and RulesActions declare the proper values for the assignment restriction.
Using D7 as a guide, I went through and created this patch to set the D8 assignment restrictions the same as they were in D7. Then I found a few places where the assignment restriction wasn't set in D7 but should have been, and a few @todos in D8 mentioning that assignment restriction should be set. I don't know why this wasn't done (or was done rather haphazardly) as part of the D8 port of the conditions and actions, but this patch should correct that oversight.
This patch should be manually tested after #2804941: Implement assignment restriction in plugin context goes in, although it should apply and run properly before that as well.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3094046-4-assignment-restriction-context.patch | 18.83 KB | tr |
| assignment-restriction-context.patch | 17.36 KB | tr |
Comments
Comment #2
tr commentedSeems like those are real test failures because some of the UI tests are expecting to see the default "direct input" mode but this patch changes the default to "data selector" in several places.
I think I'll wait to fix this until after #2804941: Implement assignment restriction in plugin context is committed, because I don't want to cause a conflict with the changes to the tests in that patch.
Comment #3
tr commentedComment #4
tr commentedHmm, looks like it was the tests that were broken all along, this patch just exposed the bad tests! I fixed UiPageTest and TempStorageTest. Both were setting a context parameter of type
entity:nodeto the value '1', which obviously is wrong. I changed that value to 'node', which is what it should be.I think it should work this time ...
Comment #6
tr commentedCommitted.
Comment #7
jonathan1055 commentedNice work here, and in #3094053: Set 'required' in Condition and RulesAction contexts
We are making good progress.
Comment #9
jonathan1055 commentedNot sure if commenting here is any good. If not I'll raise a new issue.
I think we missed one of the assignment restrictors. In 'List Count Is' the list should be by selector only? I came across this problem when debugging the failing tests on #2664280-39: Select lists in action & condition configuration forms. Is there a scenario for this condition it makse sense to enter a list by direct input? The description under the field says "A multi-valued data element to have its count compared, specified by using a data selector, eg 'node.uid.entity.roles'"
If needed, I can make a patch for this, once you have re-opened the issue.
Comment #10
tr commentedYes, it looks like that condition is restricted to selector in D7 but not in D8. The problem is that in D7 some of the conditions (like this one) and some of the actions don't set the assignment restriction explicitly in the condition/action definition, but instead do it in an alter hook. So really the only way to be sure what D7 was doing is to manually test it in D7 and try to do the same thing in D8. If you can open a new issue that would be the best way to fix it - and please try to verify the selector setting for all the list-related conditions and actions when you do this, as the list and data plugins are IMO the most likely place where these were missed.
Comment #11
jonathan1055 commentedOpened #3159876: Set assignment restrictor for two list and data conditions as the follow-up.