Remove special support for NULL values in FieldItemList

Patch
- makes FieldItemList::setValue() call the parent, keeping only a thin override for 1).
- changes ItemList::setvalue() to call $item->setValue($value, FALSE)

Let's see what bot says.

Well, the $notify param in ItemList::setValue($values, $notify) is about notifying the parent *of the List*.

Here we're talking about $list->setValue($values) assigning a $value to an $item, and chosing to notify (or not) the parent of the item (i.e itself). One implementation notifies, the other doesn't, but no reasons given.

Honestly, not sure of what's the correct behavior here. I'm trying "don't notify", since it feels weird for the list to notify itself. But I might be missing something.

Asked for feedback in #1869562: Avoid instantiating EntityNG field value objects by default, that added the $notify param in TypedData::setValue(), and had FieldItemList::setValue() & ItemList::setValue() use it differently on the items :

- FieldItemList.php (was called Field.php back then...) :

@@ -90,7 +94,7 @@ public function setValue($values) {
-          $this->list[$delta]->setValue($value);
+          $this->list[$delta]->setValue($value, FALSE);

- but didn't change ItemList similarly.

Logic error in the override. We don't want to catch $values === array().

Remaining fails seem due to item 2) in the OP, and the semantics of "$entity->field is empty"

- In HEAD,
$entity->field->setValue(NULL) ---> $entity->field has an internal $list === NULL
because FieldItemList::setValue($value) sets $this->list to $value if $value is NULL or array()
while ItemList::setValue() sets $this->list to array()

- With patch,
$entity->field->setValue(NULL) ---> $entity->field has an internal $list === array()
because FieldItemList::setValue() behaves like ItemList::setValue()

I do think the patch is right - ItemList::setValue(NULL) behavior is correct, and FieldItemList should not override it :

- Having two flavors of "empty" ($list = NULL / $list = array()) is a WTF. An ItemList is a List of Items, an empty List is one with an empty array of Items. A NULL list of items makes no sense.
- There should be no semantic difference between List::setValue(NULL) and List::setvalue(array()). In HEAD, there is none for ItemLists, but there is one (quite unclear) for FieldItemList.
- If an entity "has a field" (i.e. there is a field definition for the field name in that entity type & bundle), then there is no "unsetting" $entity->field or "setting it to NULL", it always exists as a FieldItemList object, which might be "empty" as a list:

$entity->field = NULL; // or unset($entity->field), both do $entity->field->setValue(NULL)
$entity->field instanceof FieldItemListInterface; // TRUE

So in a way, unsetting / setting to NULL is a lie, most it can actually do is emptying the ItemList. And it should do it in a special flavor.

This being said, aside from the fails in EntityFieldTest that test the current behavior and will need to be updated, it does look like some existing code relies on those "two flavors of empty" - mostly in the normalize / rest area. Will look into that.

Also, bumping to "major" for the "two flavors of empty" WTF.

Yeah - rest PATCH :-/

In D7, we supported saving "incomplete entities", where "missing field" meant "leave the current value unchanged". Some forms used that, for example. Thus we had to support some NULL/unspecified concept on fields.

We don't do incomplete entities in D8. Entities are always saved as a whole. Yet we currently maintain a hidden/undocumented notion of "NULL != empty" in our FieldItemLists, just to support the specific case of rest PATCH. That feels very wrong :-).

More on that in next comment.

First, a patch that fixes the non-rest tests for the new code behavior.

Still needs some thinking on what unset($entity->field) / isset($entity->field) should mean exactly since $entity->field is *always* a FieldItemListInterface object, but for now this just makes the tests pass.

Then, about rest PATCH requiring "NULL" field values:

The PATCH case seems to be mostly a question of how rest code flow is currently organized and who receives what information.

The current code doesn't unserialize the incoming PATCH request $data on top of the entity to update, but :
- ContentEntityNormalizer::denormalize() creates a fresh $entity from the $data. That $entity thus also contains "other stuff" (default values for unrelated fields)
- EntityResource::patch() receives the fresh $entity from denormalization and the $original_entity to update, and copies field values from one to the other.
It doesn't receive the $data itself, so it doesn't know which fields it should copy over from $entity to $original_entity. Thus it relies on the denormalization code putting some special "NULL / don't mind me" field value on the unserialized $entity, for which it requires the whole "field values" data structure to support the concept. Again, that feels really wrong.

Attached patch simply uses a custom, hardcoded property on the temporary $entity to pass the "which fields should be merged" information down to EntityResource::patch(), instead of using a "special field value" to convey that info.

Not ideally clean either, but at least the weirdness is kept between ContentEntityNormalizer and EntityResource and how they chose to work together, instead of spilling on the whole runtime values structure we have to support, so IMO this is still way cleaner than the current.

Side note : can I get a quick RTBC on #2369639: EntityFieldTest helper methods makes debugging tests impossible ? :-)

DenormalizeTest needs to be updated for the new PATCH behavior.

Reroll after #2369639: EntityFieldTest helper methods makes debugging tests impossible

Reroll, just in case

Disccussed with @fago on IRC, making isset($entity->field_name) behave like "field has values" doesn't fit too great in the Entity/Field syntax metaphor.

- Would be weird to have isset($entity->field_name) == FALSE;
while $entity->field_name is still an object you can call methods on.

- It would make empty($entity->field_name) work as "field has no values"
because PHP evaluates empty($object->magic) as isset($object->magic) ? empty(__get($object->magic)) : TRUE.
In our case, that de facto translates to empty($object->field_name) === !isset($object->field_name)

That might sound kind of nice, but it's actually really surprising magic, since PHP doesn't offer magic callbacks for empty(), so you don't expect magic behavior for empty($something_that_is_an_object).

- We do have $entity->field_name->isEmpty() or $entity->field_name->count() for "does the field have values ?".

[edit - also:
- It would be a bigger API change, which we don't necessarily want if we're not convinced it's a good idea]

As per IRC discussion summarized in #26, attached patch goes for "isset($entity->property) is always TRUE if 'property' is a Field API field".

Plus, changes __unset() so that unset($entity->field_name) assigns value array() rather than NULL. This is just for clarity, since FieldItemList::setValue() turns NULL into array() anyway.

OK, we are all in agreement on what the patch does, but what it does really is two things, that would be best separated for commit message and history tracking :

1) Remove support for NULL values for FieldItemList, and adjust rest PATCH accordingly.
Since most of the discussions here are about this aspect, repurposing this issue here to do just that.

2) Switch FieldItemList::setValue() to do $item->setvalue($value, FALSE), like FieldItemList does - as a side effect, once 1) is done, this also lets us turn FieldItemList::setValue() into mostly a parent call rather than a full code duplicate.
Will move that part to a separate issue.

Attached patch is just the 1) part. All of the code has been vetted already.

Opened #2381777: Unify setValue() implementations in ItemList & FieldItemList for the $item->setValue($value, FALSE) part, postponed on that one.

Still thinking about the "consistent metaphor" aspect for our magic syntax for entity fields.

The current patch does (from the issue summary) :

- $entity->field_name = NULL / unset($entity->field_name) is striclty equivalent to :
$entity->field_name = array()
- isset($entity->field_name) is always TRUE for Field API fields.

What this end up with, though, is:

unset($entity->field_name);
isset($entity->field_name) === TRUE; // Try again, still set :-)...

Since we state that "$entity->field_name is always set for Field API fields", it also means that you shouldn't be able to unset it... - and there's no actual use case for that: you can empty it, if that's what you want.

That is:
- isset($entity->field_name) is always true.
- unset($entity->field_name) and $entity->field_name = NULL both throw an exception. That's consistent with the above.
- $entity->field_name = array() empties the field, also consistent with the above.
- $entity->field_name->setValue(NULL) is still allowed, but internally sets array(), just like ItemList does.

@fago (& others) : toughts ?

Patch does as per #31 : exception on unset($entity->field_name) / $entity->field_name = NULL

Oops, #32 has the wrong interdiff.

Minor - unintended change (PhpStorm glitch ?)

Node forms were doing $node->revision_log = NULL

Found this while working on the fails : #2382493: Population of default field values in entity translation is incorrect - reviews welcome :-)

So, well, turns out there's a lot of code out there that uses unset($entity->field) / $entity->field = NULL for "empty the field".
Which means forbidding it as proposed in #31 is a fairly large API change at this point.

This circles back to the question of "should isset() mean empty ?", though, since the de-facto use is "unset() / assign NULL to empty the field"...

OK, forget #31 to #40 - reroll & revert to the approach in #29 : unset($entity->field_name) and $entity->field_name = NULL do not throw an exception, and only empties the field.

Then, checking what happens if we chose to do "isset($entity->field_name)" == "the field is not empty".

- Fixed merge error that caused the rest test fails
- Fixed embarrasing logic error (isset($entity->field) = "field is empty" instead of "field is NOT empty") :-p.
Surprisingly little things broke though, which proves that isset($entity->field) is used very rarely in core at the moment :-)

#45.1 : yep, reverted
#45.2 : right, was not a fan either. moved to an assertFieldIsEmpty() helper.

Also, updated the issue summary for the current approach.

Er, and the patch

Opened #2392805: Remove useless isset() checks in ItemList / FieldItemList as a followup.

Added Beta phase evaluation

Unfinished sentence :-)

Reroll ?

Green, back to RTBC

Aw shoot, InstallUninstallTest :-(

Hm, yeah, but green locally :-)

Green, was a bot fluke apparently.

Back to RTBC...

Cleanup followups :
- #2381777: Unify setValue() implementations in ItemList & FieldItemList should be an easy RTBC
- #2392805: Remove useless isset() checks in ItemList / FieldItemList has a patch for review