Avoid having empty field items by default

• The map type has internally to ensure it deals with a traversable / non-emtpy value before iterating over it.
• DenormalizeTest::testMarkFieldForDeletion() needs to differentiate between NULL and an empty array instead of NULL and an array with NULL values. Since there were no other test fails we assume the services handling does this already that way.

If this removes the empty Item that's present in Field::$list by default, is there maybe some code in some isEmpty() somewhere that had to account for that and can now be simplified ?

I think we ran over that hunk already in another issue as well.

Anything to do about / because of this?

We usually use isset() for checking null values.

Changed is_null() to !isset().

I ran a performance test using the xhprof-kit(using 100 loops)
Results:

ct  : 1,973,449|1,994,736|21,287|1.1%
wt  : -2,147,483,648|-2,147,483,648|0|0.0%
cpu : 12,406,250|12,437,500|31,250|0.3%
mu  : 78,406,432|78,420,512|14,080|0.0%
pmu : 96,046,864|96,059,032|12,168|0.0%

The patched branch had a performance regression of 0.3% compared to the base line. The addition of 1.1% for function calls was expected.

I've tested using 489 comments (to avoid the BC-decorator on nodes) all of them shown on one page in a view formatted as table (because of #2020353: Comments aren't displayed in Views when using Format: Unformatted list and Show: Comment | Full comment)

@fago: I think I bumped into some code yesterday in TypedData / EntityNG that seemed to be specifically about those "present by default" items. But I can't really remember where, so, well, never mind me :-)

#14: d8-avoid-empty-field-items-1988492-14.patch queued for re-testing.

They see me re-rollin', they lovin' :P

Hmm, looks like there was a change I'm not sure how to handle.
While the updated patch should avoid lot's of errors I'm not sure if this is really the right approach.

As of now TypedDataManager::getPropertyInstance() is called by FieldItemBase::__construct() without passing a value. That's a first and it seems like the currently optional value parameter of TypedDataManager::getPropertyInstance() shouldn't be optional.
With this patch NULL is allowed and set as property-value. However, since NULL is the default value of the optional value parameter, there's no guarantee that NULL is really an appropriate property-value.

When e.g. an user object is built FieldItemBase::__construct() calls TypedDataManager::getPropertyInstance() to build the computed property for the user image. Now, since no value is provided, TypedDataManager::getPropertyInstance() set's the property-value to NULL. But later on FileWidget::formElement() expects the property to be an array.
The question now is:

• Should the value parameter of TypedDataManager::getPropertyInstance() be really optional?
  • Yes: How do we ensure to always set an appropriate value? (Default value method?)
  • No: How do we ensure FieldItemBase::__construct() sets an appropriate value?

The attached patch only changes the behaviour of FileWidget::formElement() to not assume it gets an array as property-value.

I think it's the job of the caller to set a suiting value.

Agreed.

...Still, it's fine to default to have the factory create the object with a NULL value by default imho?...
...That sounds like a bug in there somewhere, then? It cannot rely on array() if no one sets that?...

That's where I'm not sure about. Currently I assume that if TypedDataManager::getPropertyInstance() wouldn't be called by FieldItemBase::__construct() the assumption of having an array as value is valid (Even though a bit bold).
Thus the patched version of TypedDataManager::getPropertyInstance() could be held responsible for breaking this assumption.

However, let's see what the test-bot says to the patch in #20

Hmm, looks like I was looking at the wrong place to figure out why the empty array is lost.
Not FieldItemBase::__construct() but ItemList::createItem() should ensure it doesn't set NULL when an empty array would be more appropriate.
FieldAttachStorageTest has tests like this $this->assertIdentical($read->{$this->field_name}[0]->getValue(), array(), "The test entity revision $vid is deleted."); and for me that looks absolutely valid and we should try to meet these "assumptions".

#24 looks better. Callers of getValue() should not have to worry about "should I check that it's a real array". See http://www.garfieldtech.com/blog/empty-return-values

Callers of getValue() should not have to worry about "should I check that it's a real array"

I totally agree. I just wasn't sure where I've to / can fix that. And frankly speaking I'm still not sure it's fixed in the right place now.

We really need to fix that IMO. In various patches I hit bugs with $items that are "in theory empty but in reality contain an $item object with NULL properties", which break various stuff in various subtle ways. Those require adding safeguard ->filterEmptyValues() calls in more and more places. This looks messy, and it's fairly hard to predict in which part of the code flow an $items object can be considered safe or not.

#24: d8-avoid-empty-field-items-1988492-24.patch queued for re-testing.

We still very much need to get rid of that issue.

Many things might have changed meanwhile, so rebooting the issue with a silly patch that just removes the "$this->list[0] = new Item" in FieldItemList::__construct().

From manual testing, basic stuff like installing drupal, creating and viewing a node, still work. Let's see what breaks.

This is a bug and is at least major, IMO.

Re-adding the filter call before saving, this will hopefully fix most of them and is fine to keep.

ImageItem shouldn't blindly assume that $this->entity can be loaded.

This happened in #2164601: Stop auto-creating FieldItems on mere reading of $entity->field[N]