List Contextdefinition must be multiple

I don't see anything obvious that would have caused those test failures.

Yes I also discovered this during manual testing on #2471481: Integrate Typed Data Widgets. On pressing any of the "switch to data selection" buttons for the conditions "List contains item" and "List count comparison" we get:

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

I note that if you just add multiple = TRUE to the condition definition docblock it works fine and the error is avoided when using manually, and the tests are not affected. So that actually fixes the problem really easily. But are we sayng that the code should be clever and know that for a List context the setting of Multiple = True is implict, and if the developer did not add it then we go ahead and make sure it is set anyway? Is it acceptable to alter the definition? What if they explicitly coded Multiple = False, and then Rules code changed that to True. I'm not saying we should not do this, just prompting the discussion about the solution.

Clearly, checking these buttons all work as expected is not covered by the tests yet, so that might be something to add. I may cover that in the new tests for the widget integration work.

I don't understand the premise that "List context definition must be multiple" (emphasis mine).

I don't think it's correct to change list type contexts to be multiple, unless of course you want to allow multiple lists to be passed in. Another way of thinking about it, an entity contains a collection of data, just as a list contains a collection of data. Why should we define one as "multiple" and not the other? I think multiple is intended to mean multiple values of the base type (entity in one case, list in the other), and NOT one singular non-primitive object which just happens to hold several pieces of data.

If we look at primitive types, for example 'email' which holds one single e-mail address string, an email type context variable can be defined as multiple or not multiple, meaning we can input either one and only one string (not multiple) or we can input one or more strings (multiple). This makes clear that multiple is not referring to how many values are stored in the email data type, but how many pieces of data of type email we can enter.

See my comments at #2987505-20: WSOD "Cannot set a list with a non-array value" where I explain how I think the error in #6 above should be addressed. I think that issue is independent of this "multiple" question. IMO the best way to make progress on the InvalidArgumentException shown in #6 is to fix #2804941: Implement assignment restriction in plugin context - all the list conditions and actions should have their list context variables restricted to data selector input, which is how it was in D7.

Hi geek.merlin aka axel.rutz,
What I found was the same as what you found, that isMultiple() does not return True for those two List conditions. Yes, the patch fixed those two specific scenarios, and I could then click "switch to data selection" without getting the InvalidArgumentException. So I ran the full set of tests locally and got the same failures as in #2 above.

I then looked at the definition of the parent Drupal\Core\Plugin\Context\ContextDefinition and noticed that it had new static($data_type) whereas your patch just had new static() and you had missed the parameter. Fixing that would probably make a difference! It solved all the really odd test failures, but the tests still failed. We now get just one consistent type of test failure:

InvalidArgumentException: Cannot set a list with a non-array value.

I've uploaded a patch with this change, which I know will fail, so you can see the different results.

I then did some more thinking, and looked for other examples of "list" context. The only other two in Rules are Actions not conditons, they are ListItemAdd and ListItemRemove. This is what was causing the other test failures! These are references to lists, not a list of items. Setting these to 'multiple = true' is wrong, but they get that treatment too. Setting multiple=true manually in the annotation causes exactly the same test errors as the updated patch.

So the essence of the problem is that we only want to force multiple=true for lists in Conditions (i.e. the list contains values to match against). We should not be changing those lists in Actions where the list is the target of adding or removing items. I don't know if it is an oversight of the naming convention that both of these are called "list" so we cannot use the name to make the distinction. Or maybe that's acceptable, and we cannot/should not force multiple=true at creation time, but do it somewhere later, when we can tell whether it is a Condition or an Action. But even then, the distinction of condition vs action would be making assumptions that I am not sure are correct. I could imagine an action that has a component of a list of things that are added to another list. Then it would be correct to force multiple=true for lists in Actions. So I don't think we can be making assumptions about when to set multiple=true.

I will let you digest this lot, then we can discuss the next steps.

Jonathan

My post overalpped with TR's so I did not see his before hitting save on mine. But I think we probably agree that we should not be forcing anything to Multiple if the definition does not have it. I think that is where I was heading in my comments in #6 anyway.

Edit: Maybe we should just fix the code in these two specific Condition definitions to include multiple = true as that is how the current UI works. But I also get TR's point that what this really means is that "list" context type might not be the best to use for these conditions. They should really be a primitive type with multiple=true.
I now get what is meant by list as opposed to a list of multiple values to check against. Yes, all the "list" items should be restricted to Data Selector.

I have posted a patch on #2804941-7: Implement assignment restriction in plugin context to act on the restriction setting.

@geek.merlin aka axel.rutz Since you're the one who raised the issue, can you comment on what I said in #8? If I'm wrong I'd like to know what it is that I'm missing. Otherwise, my inclination is to close this issue with status "Works as designed".