Improve the TypedData API usage of EntityNG

Could you please post some concrete examples or code snippets in the issue summary so that we can understand the problem better? What are the implications for a node article for example?

Adding tags.

This patch allowed the EntityWrapper to support all entity types with an array('type' => 'entity:node') style invokation. type 'entity' is still supported, though internally it is known as entity:entity, and there's a little bit of a bc layer for that. I assume we want to remove that in the long term, but even if we don't it's only a couple small if statements.

The definition constraints didn't appear to be getting passed to the plugins appropriately, and since the derivatives were adding constraints for EntityType, I sort of needed it to work. It still respects whatever the user passed so array('type' => 'entity:node', 'constraints' => array('EntityType' => 'user')) should actually give you a user entity, but I didn't try it. The patch is very straight forward otherwise.

Eclipse

ok, as discussed on IRC we also need away to deal with data types that cannot be instantiated properly. E.g if we do this what would we instantiate for type = entity ? It works for a user, that we could instantiate, but we cannot even instantiate node without knowing the bundle (as to the current impl.).

• So the idea we came up with is the following:
• We introduce the notion of abstract types, i.e. a usual typed data plugin with a special flag, e.g. a key in the typed definition.
• Different to others is that abstract types are for dealing with their metadata only, so you cannot set/get a value on those object, i.e. we throw an exception in that case.
• Examples for abstract types would be "entity" and every entity types that has bundles (node, comment, but not user).
• If you instantiate an abstract type, you get an usual typed data object based upon new helper class. Those help you to deal with the metadata and can even work from situations where you specify a bunch of possible bundles (a node reference pointing to articles and pages only).
• For dealing with the metadata of non-abstracts type we'll continue to instantiate the usual classes without data.

I've worked a bit more on that, so we are getting closer. Attached patch is functionally mostly complete, but has test-fails as reference-updates are not reflected due to the stale $target property in the reference.

TODO:
* Complete the abstract entity implementation (metadata with multiple-bundles) and add tests for it.
* Finally make $entity implement the typed data interface
* Write tests for the new API

For the stale data in reference to get updated, we could do an interim hack for comparing the target-id with the source id. But for doing that properly + for #1869562: Avoid instantiating EntityNG field value objects by default + for being able to track changes to an entity we should best add support for notifying the parent data item when a property changes. Thus, we could introduce this as part of #1869562: Avoid instantiating EntityNG field value objects by default and build upon it here then.

This is a blocker for validation of REST write requests - #1696648: [META] Untie entity validation from form validation.

Have no clue yet what I'm doing, just a stupid re-roll and took care of the obvious fatal errors due to interface mismatches. Not yet sure what to do in some cases with setValue() and the $notify and some other changes.

This should at least fix most of those exceptions.

$node->langcode was sometimes array(0 = NULL), which seems to be related to #1957888: Exception when a field is empty on programmatic creation.

#1983548: Convert contact message entities to the new Entity Field API is an example were bundles are currently optional, would be great to get your feedback there.

Still says EntityWrapper

All the implements/overrides should be replaced with {@inheritdoc}

Why is the isEmpty() check only required in EntityReference? e in isEmpty should be uppercase.

Do we need to check if we have a source here? what happens if not?

Hm, what is getString() used for example? Would something like getType() . ':' . $this->getTargetIdentifier() be helpful?

Wondering about the return value here. Any reason to not limit it to int|string or something like that?

What's the difference between those two?

Huh? Why not just let the tests fail until this is done? Also, isn't that what we are doing here?

ok, I worked more on this and cleaning references up. So attached patch does the following:

• It remove the "source" reference of the reference object. Instead of letting the reference object read from the source, I've changed the approach to let the parent item take care of keeping ID and reference synched. We've to do that anyway in order to update the stale target object in the reference when the ID property changes, so I think it makes the overall flow much simpler and easier to follow that way.
• The language reference is now quite simple and clean, it consists of three parts: the typed data object of the referenced language, the language reference object and the language reference item. As the typed data language object holds the language object or ID for us.
• Entity references work a bit differently as there the typed data entity object is the same as the entity object and cannot handle instantiation without IDs. So for entity references the "entity_reference" object takes care of the ID vs object logic.
  Then, while $reference->getTarget() should always give you a valid typed data object, the value should be NULL when there is no data. So that's the cause of the current isEmpty() check in there, alternatively we could check for the abstract entity typed data class there. Let's revisit once we figured that part out also.
• In order to allow the parent item to handle synching of the reference property values (ID + reference object) we need to move the notify call to the parent to be POST-change instead of PRE-change. That way we allow the parent to access the changed value.
• I found some problems with the notify() code, i.e. set() did not correctly trigger the onChange() method of the changed complex-data. Fix was necessary for this and is included.

Attached patch also addresses the review.
Please review, in particular how the references work!

todo:
* Update onChange() docs.
* Make entity instantiation via typed data work and complete abstract typed data plugins.
* Fix tests and add tests.

Tried to fix some of the failures.

I'm a little bit concerned about following change I had to made:

After the change $comment->uid->entity always returns an entity object - even though an empty one. Thus the if clause has to be changed to check for a proper uid.
Is this the intended behaviour or should $comment->uid->entity return FALSE / NULL if no proper entity was found.

Next bunch of fixes:

• Converted ImageItem
• Fixed TypedDataManager - ProcessDecorator was lost (thanks to Berdir for pointing this out)
• core/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.php fixed the check if it's a valid entity reference - needs review, I've no clue if this is the right way to go.
• LocaleContentTest added static cache reset for the language list to ensure the newly registered languages are known.

I've really trouble to run the affected test locally (timeout). But I guess that's another case where resetting the static cache of language_list() before using WebTestBase::drupalCreateNode() helps after adding new languages using the UI.

#28: d8-expose-entity-typeddata-1868004-28.patch queued for re-testing.