Problem/Motivation

Right now the TypedDataInterface sits on all the objects and adds confusing methods there, e.g. $entity->getDefinition(), $entity->getValue().

Proposed resolution

Avoid typed data enforcing generic methods on your objects by implementing adapters instead, that translate generic typed data interface to use-case specific methods. E.g. $field->getEntity() could be translated to getRoot().

Remaining tasks

1. Start moving to adapters for ComplexDataInterface first, i.e. #2002134: Move TypedData metadata introspection from data objects to definition objects
2. Start with #2028097: Map data types to interfaces and add support for discovering typed data objects based on interfaces/classes.
3. Support adapters for TypedDataInterface + ListInterfaces also (finally this issue)

User interface changes

-

API changes

Probably some.

#2047229: Make use of classes for entity field and data definitions moves typed data definitions from arrays to objects based upon a defined interface

Original report by @fago

Let's remove the typed data interface and rely checking interfaces instead of doing $data->getType(). We'll probably need to separate out a ValidatableInterface also

This depends on doing #2002102: Move TypedData primitive types to interfaces first.

Files: 
CommentFileSizeAuthor
#82 typed-data-wrappers-82.patch37.73 KBmsonnabaum
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,851 pass(es).
[ View ]
#80 typed-data-wrappers-80-interdiff.txt2.97 KBBerdir
#80 typed-data-wrappers-80.patch37.78 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,719 pass(es).
[ View ]
#78 typed-data-wrappers-78-interdiff.txt569 bytesBerdir
#78 typed-data-wrappers-78.patch34.8 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#76 typed-data-wrappers-76.patch34.81 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#69 rest_diff.txt1.66 KBBerdir
#69 typed-data-wrappers-69-interdiff.txt7.45 KBBerdir
#69 typed-data-wrappers-69.patch44.55 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,431 pass(es).
[ View ]
#66 typed-data-wrappers-66-interdiff.txt1.51 KBBerdir
#66 typed-data-wrappers-66.patch43.29 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,279 pass(es), 11 fail(s), and 7 exception(s).
[ View ]
#62 typed-data-wrappers-62-interdiff.txt16.37 KBBerdir
#62 typed-data-wrappers-62.patch46.28 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,205 pass(es), 12 fail(s), and 7 exception(s).
[ View ]
#60 typed-data-wrappers-60-interdiff.txt4.87 KBBerdir
#60 typed-data-wrappers-60.patch31.64 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,754 pass(es), 564 fail(s), and 105 exception(s).
[ View ]
#58 typed-data-wrappers-58-interdiff.txt6.59 KBBerdir
#58 typed-data-wrappers-58.patch26.87 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#53 typed-data-wrappers-53.patch22.72 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#33 2002138-remove-typeddatainterface-32.patch50.06 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2002138-remove-typeddatainterface-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 2002138-remove-typeddatainterface-32-interdiff.txt511 bytesBerdir
#30 2002138-remove-typeddatainterface-29.patch49.98 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,263 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#21 2002138-remove-typeddatainterface-test4.patch123.51 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2002138-remove-typeddatainterface-test4_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 2002138-remove-typeddatainterface-test4-interdiff.txt54.56 KBdixon_
#18 2002138-remove-typeddatainterface-17.patch30.54 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] 53,925 pass(es), 461 fail(s), and 1,446 exception(s).
[ View ]
#18 2002138-remove-typeddatainterface-17-interdiff.txt8.57 KBdixon_
#10 2002138-remove-typeddatainterface-10.patch21.99 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] 44,807 pass(es), 1,803 fail(s), and 1,318 exception(s).
[ View ]
#4 2002138-remove-typeddatainterface-do-not-test.patch7.28 KBdixon_

Comments

tagging

Priority:Normal» Major

Promoting to major as discussed with @xjm.

Should we really remove TypedDataInterface completely? I agree that we should break out things into ValidatableInterface and maybe something like ParentAwareInterface and start checking more on interfaces. But doesn't it make sense to have a common interface for typed data?

Status:Active» Needs work
StatusFileSize
new7.28 KB

Ok, I've done some more reading on this, and I see that it probably makes sense to remove TypedDataInterface completely.

I've started breaking up TypedDataInterface into ValidatableInterface and ParentAwareInterface. The interface is obviosuly not removed yet, but I'm attaching it here to get some feedback on wether is is the right approach or not.

I'm not sure how/if we should remove the class TypedData, or if we should keep it as a base class extending the interfaces we break out from TypedDataInterface.

The patch is based on #2002102-45: Move TypedData primitive types to interfaces, so the tests will break unless that patch is applied first.

Yep, I still think we want to remove typed data interface if possible, in particular from EntityInterface.. ad ParentAwareInterface - we had ContextAwareInterface for that thing earlier, is that a better name?

We'll probably have to fix getPropertyInstance() in the typed data manager though, as it relies on being able to get the $type from the $root object. Instead, I think it's fine to going with it's class - as this maps to the type now and is used for a static-cache-key only anyway.

I went with ParentAwareInterface since that describes more explicitly what it's mean for, but also to avoid confusion with ContextAwarePlugin* classes (although those are of course another namespace). I'm fine with either really.

+1 to ContextAware, as there's more than just the parent, things like root and name.

As it's also about the name and has setContext() I think ContextAware would be better - I guess namespaces should be enough to differentiate as we do that quite frequently now (check the "field" class :D).

Assigned:Unassigned» dixon_

I have a patch coming up soon.

Status:Needs work» Needs review
StatusFileSize
new21.99 KB
FAILED: [[SimpleTest]]: [MySQL] 44,807 pass(es), 1,803 fail(s), and 1,318 exception(s).
[ View ]

TypedDataInterface is almost gone. What's still to figure out is what to do with (or where to move) TypedDataInterface::getDefinition() and TypedDataInterface::applyDefaultValue().

Some doxygen documentation also needs to be revisited, e.g. Drupal\Core\TypedData\Annotation\DataType.

With this change, TypedData has now become an even more abstract base class that data types can extend to save some code. But to complete the implementation of a data type you have to implement one of the three typed data interfaces; PrimitiveInterface, ComplexDataInterface or ListInterface.

There's a @todo in TypedDataManager::getPropertyInstance() that will be removed/simplified once #1868004: Improve the TypedData API usage of EntityNG lands.

I also had to make Drupal\Core\TypedData\Plugin\DataType\Any implement PrimitiveInterface, because all data types needs to be an implementation of one of the three typed data interfaces (as mentioned above) to not lose track of things.

Status:Needs review» Needs work

The last submitted patch, 2002138-remove-typeddatainterface-10.patch, failed testing.

Status:Needs work» Needs review

Hmm, that seems a bit random... Giving it another try.

That looks like you have fatal errors on those page callbacks.

I also had to make Drupal\Core\TypedData\Plugin\DataType\Any implement PrimitiveInterface, because all data types needs to be an implementation of one of the three typed data interfaces (as mentioned above) to not lose track of things.

hm, any as primitive data type does not make much sense to me. Why do we need to be it some of those? Any is more useful on a metadata basis so you can specify that something can be anything, but then we do not care whether it implements some of those interfaces as well it can be anything. So why care?

What's still to figure out is what to do with (or where to move) TypedDataInterface::getDefinition() and TypedDataInterface::applyDefaultValue().

getDefinition() I think should just go - do we use it somewhere except the already mentioned gePropertyInstance() ? Or maybe better, I think it would fit into ContextAwareInterface, i.e. something context aware can know about itself as well ;-)
applyDefaultValue() probably would need it's own interface? Not sure what a good name would be here, DefaultableInterface ?

Status:Needs review» Needs work

@Berdir Thanks, I'll look into that!

@fago

hm, any as primitive data type does not make much sense to me. Why do we need to be it some of those? Any is more useful on a metadata basis so you can specify that something can be anything, but then we do not care whether it implements some of those interfaces as well it can be anything. So why care?

The reason why I extended PrimitiveBase was because Any wouldn't have any get/setValue() otherwise since those are removed from TypedData that it previously extended. So why doesn't the TypedData base class have get/setValue()? Because since TypedDataInterface is going away the TypedData base class turned into an even more abstract class that only implemented things that most typed data objects would need (e.g. validation, context aware, defaultable etc.) without making any assumtions about the data itself and how it's fetched. For example; it doesn't make sense to have get/setValue() for complex data that instead has get/set($property_name).

And although we have #2028097: Map data types to interfaces, I really think that all unique interfaces should extend from a few common interfaces that we can make generic checks or generic type hinting on. My suggestion is that those generic interfaces should be: PrimitiveInterface, ComplexDataInterface and ListInterface.

When I think about "primitive" I'm thinking about a data object with a holistic/stand-alone value without much meta data. So if you look at any php variable holisticly, they're primitive ;-) But maybe that's the wrong way to look at it... :-)

But, if we don't see a use case for a few common interfaces I'd be happy to make a completely unique interface for Any.


getDefinition() I think should just go - do we use it somewhere except the already mentioned gePropertyInstance() ? Or maybe better, I think it would fit into ContextAwareInterface, i.e. something context aware can know about itself as well ;-)

Ok, I'll see if I can remove it completely.

applyDefaultValue() probably would need it's own interface? Not sure what a good name would be here, DefaultableInterface ?

Makes sense.

My suggestion is that those generic interfaces should be: PrimitiveInterface, ComplexDataInterface and ListInterface.

I'd agree with that.

But, if we don't see a use case for a few common interfaces I'd be happy to make a completely unique interface for Any.

For "any" I don't see the use-case as it just tells me it may be anything, so it could be a primitive or a list or complex data, I don't know. Likewise I don't think an interface is needed here. I guess the class has no much use anyway, it's more the data type that's useful in data definitions to refer to anything.

Also, from wikipedia the definition of primitive does not sound as being a match for "any":

In computer science, primitive data type is either of the following:[citation needed]

  • a basic type is a data type provided by a programming language as a basic building block. Most languages allow more complicated composite types to be recursively constructed starting from basic types.
  • a built-in type is a data type for which the programming language provides built-in support.

Status:Needs work» Needs review
StatusFileSize
new8.57 KB
new30.54 KB
FAILED: [[SimpleTest]]: [MySQL] 53,925 pass(es), 461 fail(s), and 1,446 exception(s).
[ View ]

TypedDataInterface still not completely gone, but I found more instances of getType() so I'll first see if I can get this to pass...

+++ b/core/modules/action/lib/Drupal/action/ActionFormControllerBase.phpundefined
@@ -94,7 +94,7 @@ public function form(array $form, array &$form_state) {
       '#type' => 'value',
-      '#value' => $this->entity->getType(),
+      '#value' => 'entity',
+++ b/core/modules/action/lib/Drupal/action/ActionListController.phpundefined
@@ -85,7 +85,7 @@ public function load() {
-    $row['type'] = $entity->getType();
+    $row['type'] = 'entity';

This is an overlap with the action config entity, there ->type()/getType() is a thing

Status:Needs review» Needs work

The last submitted patch, 2002138-remove-typeddatainterface-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new54.56 KB
new123.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2002138-remove-typeddatainterface-test4_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Now I've got up to the point where only getDefinition() is left in TypedDataInterface, but all tests should pass so far. So it's close! :)

I've done the patch on-top of #1868004-81: Improve the TypedData API usage of EntityNG, so the attached patch includes that patch as well. Reason being that there are many overlaps between these two issues and re-rolling after that goes in would have been unecessary work. However, the attached interdiff is only what this issue is changing, relative to that.

Here are some random notes:

  1. I've broken out various methods from TypedDataInterface to their own interfaces, e.g. ValidatableInterface, ContextAwareInterface and StringableInterface.
  2. There are still comment/documentation changes that needs to be done
  3. I've dumbed down the Any type and added a simple interface for it, since its not implementing any of the three main interfaces anymore. The helper method in typed data manager still recognizes it as valid typed data so it can be used to mock with, etc. So AnyInterface is almost like our fourth valid "main" typed data interface, which I think is ok.
  4. AnyInterface is not stringable anymore. I don't think that makese sense, because the value can be anything (i.e. complex) that is impossible to string. And the intent is for AnyInterface to be very dumb, so I removed that (and the previous tests for it)
  5. Same goes with the Map type, it implements ComplexDataInterface and should not be stringable, imo. Partly because the properties of a map can be anything (i.e. complex), so I removed that as well.
  6. In general the Map type implementation needs some more clean-up, because it currently has get/setValue() which a complex data type should not have. We have get/setPropertyValues() in the ComplexDataInterface for that. Either we should open up a new issue, or do this clean-up in: #1928938: Improve typed data definitions of lists
  7. I've temporarily added get/setValue() to the config schema types for backward-compatibility and not touched that further. The implementation for config schemas needs some serious love, but that's another issue: #1928868: Make typed config valid typed data

That's it for now. Now I need to figure out how to remove getDefinition() and finally kill TypedDataInterface.

Status:Needs review» Needs work
Issue tags:+Entity Field API, +Typed sanity

The last submitted patch, 2002138-remove-typeddatainterface-test4.patch, failed testing.

Not sure if all of this is still possible, despite it being a major task. Also, it doesn't yet tackle some of the reasons why this issue exists.. e.g., there's still an Entity::getType() :)

Trying to start doing this in separate steps, here's step #1: #2056721: Remove TypedDataInterface::getType()

Assigned:dixon_» alexpott

I think the overall sanity that this brings to the API is important enough to justify the BC break here. However, I would appreciate a second thumbs-up from another core maintainer, so tentatively assigning to alexpott.

Incidentally, this patch is already mighty big enough as it is; should we move the getDefinitions() stuff to a separate issue(s)?

Oh, effulgentsia pointed out #2002134: Move TypedData metadata introspection from data objects to definition objects which is where that probably makes more sense to tackle.

Probably related: #2061331: Add getEntity() helper in FieldInterface / FieldItemInterface
The Field::getEntity() method added here would just duplicate Field::getParent()

Yep this is change brings alot of sense to an Entity's API. TypedDataInterface should not pollute it.

Status:Needs work» Needs review

Re-roll.

Still not quite sure what to do with this, need to think about it.

Assigned:alexpott» Unassigned
StatusFileSize
new49.98 KB
FAILED: [[SimpleTest]]: [MySQL] 58,263 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Patch would help.

Status:Needs review» Needs work

The last submitted patch, 2002138-remove-typeddatainterface-29.patch, failed testing.

Ok, this should fix the test fail, but...

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -382,4 +384,29 @@ public function getConstraints($definition) {
+  public function isTypedData($object) {
+    if ($object instanceof ComplexDataInterface) {
+      return TRUE;
+    }
+    elseif ($object instanceof ListInterface) {
+      return TRUE;
+    }
+    elseif ($object instanceof PrimitiveInterface) {
+      return TRUE;
+    }
+    elseif ($object instanceof AnyInterface) {
+      return TRUE;
+    }
+    return FALSE;
+  }

This summarizes what my problem with this is :)

All those things have something in common. They're typed data. So what's wrong with TypedDataInterface?

Yes, we should remove all those methods that are in the way for most typed data classes. But we obviously have use case where we need to check if an object is typed data. Then let's do that using an interface?

I'm not yet sure if getDefinition() should be renamed/moved/removed, but there are valid use cases for what it does, see #2056721-45: Remove TypedDataInterface::getType() for one example.

Status:Needs work» Needs review
StatusFileSize
new511 bytes
new50.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2002138-remove-typeddatainterface-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -446,31 +445,6 @@ public function getDefinition() {
   public function getConstraints() {
@@ -500,37 +474,37 @@ public function onChange($property_name) {
   /**
-   * Implements \Drupal\Core\TypedData\TypedDataInterface::getName().
+   * {@inheritdoc}
    */
   public function getName() {
     return NULL;
   }

inherit from who?:) TypedDataInterface::getName() has been deleted in this patch. And some other similar methods.

Not sure shout this getDefinition()., but maybe

<?php
$definition
= $data->getDefinition();
?>

convert to something like:
<?php
$definition
= \Drupal:typedData()->getDefinition($data->getPluginId());
?>

Status:Needs review» Needs work

core/lib/Drupal/Core/TypedData/TypedDataManager.php:

<?php
/**
* Decided whether or not an object is typed data or not.
*
* @param mixed $object
*
* @return boolean
*   Returns TRUE if $object is an instance of any of the typed data
*   interfaces, else FALSE.
*/
public function isTypedData($object) {
  if (
$object instanceof ComplexDataInterface) {
    return
TRUE;
  }
  elseif (
$object instanceof ListInterface) {
    return
TRUE;
  }
  elseif (
$object instanceof PrimitiveInterface) {
    return
TRUE;
  }
  elseif (
$object instanceof AnyInterface) {
    return
TRUE;
  }
  elseif (
$object instanceof DataReferenceInterface) {
    return
TRUE;
  }
  return
FALSE;
}
?>

core/lib/Drupal/Core/TypedData/Validation/Metadata.php:

<?php
  
public function getMetadataFor($typed_data, $name = '') {
-    if (!
$typed_data instanceof TypedDataInterface) {
+    if (!\
Drupal::typedData()->isTypedData($typed_data)) {
?>

This worries me.

If we still have code that has to check if something is typed data, then there's no point in removing the interface. This code is a regression from what we had. It's a bit hard to grok why isTypedData is still needed, but if it is and we can't get away from it, a TypedDataInterface with no methods would be a better option. That said, I wonder if we can't have a more polymorphic way of handling this.

Status:Needs work» Needs review
Issue tags:-API change, -Entity Field API, -Typed sanity, -Approved API change

Status:Needs review» Needs work
Issue tags:+API change, +Entity Field API, +Typed sanity, +Approved API change

The last submitted patch, 2002138-remove-typeddatainterface-32.patch, failed testing.

The more I look at this the less sense it makes. I'm getting close to none at all :)

This patch does many different things and touches various different places, and it's really hard to grasp and review/work on this. It does a lot of ugly things to forcefully remove the interface (like simply removing it as a type int, leaving us with non-type hinted object arguments, crazy methods like isTypedData() to re-introduce a way to identify typed data) but we don't actually gain much in the end. ComplexData is still ContextAware data, Entities are still ComplexData and therefore ContextAware data, which means they still have stupid methods like getRoot(), getName(), setContext() that they do not and can not implement in a useful way.

There would IMHO only be one way to remove TypedDataInterface. And that would be to remove the *concept* of typed data (TypedDataManager, the plugin type, interfaces, base classes, ...) and have entities, fields/fieldlists, primitives and references and so on that are all completely their own thing, in a fixed structure/hierarchy. That might have advantages for the known structure of entity -> field -> primitives/references, but there's also stuff that I don't enough about, like context stuff, config schema and future uses of typed data, like integrating it with the token system). I don't think that is possible at this point.

My suggestion would be to identify smaller, actionable points and deal with them in separate issues.
* I don't know what exactly that would be, but I think it should be possible to deal with getDefinition(), While not the interface, but the TypedData base class implements PluginInspectionInterface, which defines getPluginDefinition() and I have no clue what the difference between the two really is. and how one could possibly return something different from the other.
* Other methods like getString(), getName(), the context/parent stuff.
* validate(), which could also be useful to have outside of typed data? Also, looking at the patch context some config schema classes, they do seeem to implement validate() in a completely different way. Not sure if it's used like that or just incorrectly implemented and unused. Should possibly be used to more specific interfaces too.

I'd love to get some more feedback from @yched and @effulgentsia, who worked extensively with/around typed data stuff in the field context and from context/config schema people.

Issue tags:+sprint

Tagging for rocketship focus

I can't say I have an overall vision of TypedData API, its scope and current uses, the generic features it provides (validation, context...)
so unfortunately I can't provide a fully informed opinion... But:

There would IMHO only be one way to remove TypedDataInterface. And that would be to remove the *concept* of typed data (TypedDataManager, the plugin type, interfaces, base classes, ...) and have entities, fields/fieldlists, primitives and references and so on that are all completely their own thing, in a fixed structure/hierarchy

I would be very very +1 on that. The current situation where everything is, first and foremost, a piece of "typed data", and then also "something more specific" (an entity, a field within an entity, a property within a field...), leads to problematic limitations and absurd situations.

  • Absurd: "field type" classes are exposed/discovered as @FieldType plugins but used as @TypedData plugins.
    Currently all field types need to be exposed as FieldItem classes that are @FieldType plugins. But the Field/FieldItem objects themselves are only ever instantiated as TypedData objects, through the TypedData plugin manager. So @FieldType plugins need to exist as @TypedData plugins as well, this is done by using a plugin derivative that automatically adds all @FieldType plugins as @TypedData plugins.
    Result:
    - the plugin definitions are duplicated in two separate plugin managers, both with their own separate persistent + static cache. Memory clutter, cache invalidation fun.
    - FieldItem classes are exposed as @FieldType plugins with some plugin id, but are always instantiated as a different plugin type, with a different id. This is a confusing WTF for things like getPluginId() - "no, at runtime, the plugin id won't be the one defined in the annotation at the top of the file - and the pluginDefinition will be slightly different too".
    - Meanwhile, the list of @TypedData plugin definitions is an unstructured soup of things of very different nature and interfaces. Since real life use cases often need to tell them apart (Field UI needs only "the list of field types"...), plugin definitions need to add custom metadata entries for "I'm a 'field' kind of typed data" or "I'm a 'property' kind of typed data".
  • Limitating: The current situation means instantiation of any "typed data" object is forced into the same "typed data factory" (TypedDataManager::create()), and all objects need to share the same __construct() signature.
    There are several cases where we find Field / FieldItem classes have special needs though:
    - The $definition that gets injected should be a FieldDefinitionInterface object, not an array
    - Field / FieldItem need to be aware of their own language, and although I didn't really investigate how to pass that information yet, I kind of fear the typed data factory will make that a pain - might be FUD, not sure yet.
    [edit: patch for this at #2078625: Field/FieldItem value objects should carry their language - the constraints there mainly come from the "prototypes" optimization trick in TypedDataManager, and we'd probably want to keep that anyway, so no actual impact here]

On a high-level, no-implementation-details-so-talking-out-of-my-*ss architectural point of view, it would seem the generic features provided by TypedData would be better off split in different interfaces, that the different object types (entity, field, properties) would implement. The difficulty would probably be providing base implementations in base classes that can be inherited... Another place where we'd need traits...

Having spent quite a bit of time with TypedData lately I thought I would share my experiences as someone who's trying to consume the API.

This is sort of related to yched's comment above, but trying to introspect an entity using the appropriate plugin managers is a nightmare. The only sane way I could find to do it was to create an empty entity, and then iterate. I could never figure out how to get from a Field definition to its FieldItem definition. Nobody cares about the Field. It's a list, why is it impermeable? (I'm sure I'm screwing something up.) Trying to figure out the plugin managers is a PITA. I'm pretty sure I saw an issue that wants to remove the the metadata from the instances, which will kill my current approach. The thing is, that I understand the idea well enough, it's just very difficult to grok the implementation. Entity -> Field -> FieldItem -> Primitive seems simple enough (I'm sure that's not exactly right). But Entity, Field, and FieldItem all have ancestors to basic types(TypedData) on their own. What I'm thinking, or hoping, this issue is getting at is that there's not a real reason for each of the major types to descend all the way down. In practice, you're very much interested in knowing the difference between the types, more so than the similarities.

Part of me wants to like it, I remember the type system in entity api for 7. That was very easy to grok. It wasn't that much different, but there was a single point of discovery. An actual hierarchy rather than a net.

Yeah, I agree it's a good idea to split the whole typed data concept.

Not only "field type", but also 'entity type' are derivative typed data for TypedDataManager. But the two "types" do have their own Managers, i.e., FieldTypePluginManager and EntityManager. Personnaly speaking, TypedDataManager looks wierd to take any responsibilities here.

Limitating: The current situation means instantiation of any "typed data" object is forced into the same "typed data factory" (TypedDataManager::create()), and all objects need to share the same __construct() signature.

Hmm, "typed data" Entity is instantiated via its own storageController I think, i.e., Drupal::entityManager()->getStorageController($entity_type)->create($values). This is a special case, while other 'typed data' are instantiated via TypedDataManager.

In practice, you're very much interested in knowing the difference between the types, more so than the similarities.

Totally agree:)

Maybe move validating/constraint method, like 'getValidationConstraintManager' of TypedDataManager into some base "data plugin manager" for generic data managers(TypedDataManager, FieldTypePluginManager, EntityManager, and maybe other "data plugin managers") to inherit. TypedDataManger remains to only takes care of premitive data/ list Data, perhaps.

And Method 'getPropertyInstance' of TypedDataManger have different context to deal with when it comes to specific Type's property. If this method is spilt into different manager, I guess it will make code easy to read too:)

Edited my #41 above - the point about "Field / FieldItem need to be aware of their own language" cannot be counted against TypedDataManager's factory.

I can't say I have an overall vision of TypedData API, its scope and current uses, the generic features it provides (validation, context...)
so unfortunately I can't provide a fully informed opinion... But:

Except from the validation stuff (which we could probably deal with) I think the main reason we need something like typed data is that we keep a place for registering "data types" to which we can refer in data definitions. So we can make use of them when we describe condition parameters or block contexts.

I would be very very +1 on that. The current situation where everything is, first and foremost, a piece of "typed data", and then also "something more specific" (an entity, a field within an entity, a property within a field...), leads to problematic limitations and absurd situations.

True - there are quite some confusing points. So maybe, we can improve that situation with the folllowing:

Maybe move validating/constraint method, like 'getValidationConstraintManager' of TypedDataManager into some base "data plugin manager" for generic data managers(TypedDataManager, FieldTypePluginManager, EntityManager, and maybe other "data plugin managers") to inherit. TypedDataManger remains to only takes care of premitive data/ list Data, perhaps.

Yeah, I had that idea in mind as well. Could we inherit from FieldTypeManager from the TypedDataManager + make adjustments by overrides as necessary? That way, field items could be created via the field type manager? However then, what if someone tries to create a field type via the typed manager plugin manager? I guess we could re-route that to the appropriate manager via the derivative class.

That would help improving situation in regard to instantiation, but not really in regard to plugin identity. For that I think it would make most sense for that to be the highest-level plugin, i.e. the field type (or entity type). Once we've instantiation run through the field type manager this should be doable.

Then, I think the remaining PITA is:

Entities are still ComplexData and therefore ContextAware data, which means they still have stupid methods like getRoot(), getName(), setContext() that they do not and can not implement in a useful way.

We could solve this PITA we need to fix up this generic wording by making use of use-case specific wordings, i.e. replace getRoot() with getEntity() ... Again, this should be doable when we have per-data-type instantiation overrides. I don't think the generic ones are so problematic here, I think it's more ComplexDataInterface.

ComplexDataI is the harder one though. First off it's used by the validator to navigate through the tree of data. We could solved that by writing custom metadata adaptors for each use-case, but we need be able to generically drill down the tree in other situations also, e.g. for deriving token values or applying data selector of any block context UI system or Rules. So could we
- leave ComplexDataI in place for checking its capabilities, but strip it down and remove generically named stuff (e.g. everything imposing "property" terminology)
- have simple adapters, e.g. let derivative classes translate from generic typed data terminology to use-case specific terminology

We discussed this and #2002134: Move TypedData metadata introspection from data objects to definition objects during a hard-problems after-meeting in Prague. The whole notes can be found here, I just re-post the conclusions here:

  • Conclusion: We need to switch to an adapter approach to being able to e.g. translate getFieldDefinitions() of entities to a general getPropertyDefinitions() method, e.g. on the manager.
  • Conclusion We want to implement an adapter system instead. TypedDataInterface does not have to be implemented directly anymore, but instead you can implement an adapter that bridges the current available methods to other, possibly bettter named methods on using objects. E.g. getRoot() -> getEntity(). This will be done for Entities and Field items in a first step.
  • Conclusion We use the field type manager for instantiating field item objects, which internally makes used of the TypedDataManager to benefit from it’s optimized factory helper.
  • Conclusion In the first step, we leave FieldItemList and FieldItemProperties to use and extend from typed data directly, as that’s the first migration step anyway. Once we’ve done that we should have a clearer picture on what makes most sense and we can re-iterate on that question.

Furthermore related, the conversion to class-based field definitions over at #2047229: Make use of classes for entity field and data definitions moves data definitions to be classed based as well. This opens the questions whether we should support an adapter approach here as well.

For discovering typed data objects, we discussed having a helper method on the object that looks for known interfaces or classes (related: #2028097: Map data types to interfaces). Alternatively, we could keep an empty TypedDataInterface for discovery purposes, but we should not require that as it makes it more difficult to expose e.g. php objects of other libraries as typed data.

Title:Remove TypedDataInterfaceUse adapters for supporting typed data

So, based on #46 here is a suggested plan of attack:
1. Start moving to adapters for ComplexDataInterface first, i.e. #2002134: Move TypedData metadata introspection from data objects to definition objects.
2. Start with #2028097: Map data types to interfaces and add support for discovering typed data objects based on interfaces/classes.
3. Support adapters for TypedDataInterface + ListInterfaces also

As following, the TypedDataInterface probably won't go away - but e.g. move from your objects to adapters - I'm retitling this issue and updated the issue summary accordingly.

I'll start working on this as soon as the necessary ground-work for #2047229: Make use of classes for entity field and data definitions has been put in place.

Issue summary:View changes

updated issue summary

Issue summary:View changes
Status:Needs work» Postponed

Let's move on with this, once we've decoupled the metadata related methods from our objects #2002134: Move TypedData metadata introspection from data objects to definition objects.

Issue tags:+beta target

I think this will involve an API change to Entity Field API that will affect quite a few modules, so adding the "beta target" tag. Maybe I'm wrong though?

It's not necessarily affecting them, but given that we want to do this in order to remove methods from our primary data objects that don't suit there - yes those methods would go away.

So I think we need at least go though the list of methods and mark any methods that we want to go away later as deprecated before beta. But of course, it would be better to get this done before beta so I'm agreeing with at least a 'beta target'.

Status:Postponed» Needs work

Based on the recent improvements done over at #2002134: Move TypedData metadata introspection from data objects to definition objects I've given this issue quite a bit of thought recently.

Conclusion We want to implement an adapter system instead. TypedDataInterface does not have to be implemented directly anymore, but instead you can implement an adapter that bridges the current available methods to other, possibly bettter named methods on using objects. E.g. getRoot() -> getEntity(). This will be done for Entities and Field items in a first step.

I figured, typed data can already work with the suggested "adapter approach", where a class bridges between the typed-data view of the world and the domain-specific objects. The wrapping typed data objects act already as that "adapter class" and translate between the general and domain-specific APIs. However, we do not make use of this adapter approach for entities and fields right now, but decided to natively implement the typed data interfaces here what leads to the perceived problems.

So, let's re-iterate on what the problems are:

  1. TypedDataInterface & ComplexDataInterface force us to have the generic methods on our (entities & fields) domain objects, which are confusing.
  2. The data "entity:*" data types are problematic as it's not possible to create typed data objects for them via the factory, thus they violate the API. Moreover, it lacks differentiation between the entity object representing a certain, unique entity and typed data entity object, which should be able to hold various different entities, e.g. so you could set some entity object and validate it matches the definition (i.e. $t->setValue($entity) & $t-validate()).
  3. Fields and field items are instantiated primarily as typed data objects, whereas they should primarily be fields and field items.

Let's see how a solution could look like:

S1: Use wrapping typed data objects for implementing typed data for entities.
For 1. and 2. the natural solution would be to start doing a separate typed data entity object, which just wraps the real entity object and so acts as "adapter" between the generic typed data interfaces and the entity interface. This frees us from having typed data interfaces and methods on the entity object and solves typed data issues around the 'entity:*' data types.

#1868004: Improve the TypedData API usage of EntityNG completed typed-data integration for entities by doing it analogously to fields - by making entities being typed data itself. A good reason for doing that was also to fulfill the typed data API of fields and field items, i.e. their implementation of getParent() has to return the parent typed data object. By having the entity object being typed data we avoided having a confusing getParent() implementation on fields. What leads us to the problem of having confusing methods on our domain objects.

From #46:

Conclusion In the first step, we leave FieldItemList and FieldItemProperties to use and extend from typed data directly, as that’s the first migration step anyway. Once we’ve done that we should have a clearer picture on what makes most sense and we can re-iterate on that question.

I think this conclusion is fine, as it makes sense to build upon the existing API to implement field item properties, field items and field item lists instead of re-inventing the wheel. Doing otherwise would require us to duplicate a lots of the logic/code for fields now. But what exactly are the problems related to terminology that exist here?
- getName() is not necessarily the field name.
- Given S1, getParent() is not necessarily what one would expect
- getDefinition() on field items is confusing as one does not get the field definition, but the field item definition
- ..

So it boils down on typed data method names being too generically named, so that they are confused with something else in the domain context. Therefore, in order to make natively implementing typed data interfaces (as fields do) feasible I'd suggest doing:

S2: Name typed data interface methods unambiguously
For example:
TypedDataInterface

  • getDefinition() -> getDataDefinition()
  • getName() + getParent() -> getDataContext()
  • setContext() - remove (see below)
  • getPropertyPath() -> replace with a method on the manager working based on getDataContext() (it's hardly used anyway)
  • getRoot() -> replace as getPropertyPath(), (getEntity() can stay)

Conclusion We use the field type manager for instantiating field item objects, which internally makes used of the TypedDataManager to benefit from it’s optimized factory helper.

Yes, that's in particular important for field items being able to act as field item primarily, and as typed data secondly. For keeping a way to instantiate the typed data objects we'll need to do:

S3: Make typed data object instantiation more flexible

Right now, the typed data manager expects a certain typed-data object constructor to be on the data type class. For allowing objects like fields to implement their own constructor and for allowing DI I'd suggest to move to an instantiation method similar to the ContainerFactory, but by going with a method that is specifically named related to typed data - e.g. public static createTypedData($container, $definition, $name, $parent). That way we can use the same constructor as previously by default, while we implement custom construction via the field type manager for field item lists and field items.

So what would be downsides of doing S1-S3? One issue I can of think of right now is that $violation->getRoot() of entity validation violation would point to the wrapping typed data object, instead of poing to the entity itself. This relates to us/typed-data not knowing on whether an object implements the typed data API natively (as fields) or in the wrapping manner (as primitives, or with S1 entities). We could solve that by e.g. adding a flag to the data type annotation that tells us that, e.g. "wrapper: true|false". This would help us on other places where we have the same requirement (know in which form to pass the data on best) as well.

Then, there is the question on whether we should implement an adapter approach for field definitions. Right now, field definition objects implement the data definition interface, and are so native typed data definitions. The API as proposed at #2002134: Move TypedData metadata introspection from data objects to definition objects would allow us already to separate field definitions from the typed data definitions by implementing it as a separate FieldItemListDefinition class that wraps a regular field definition object to implement the typed data interface. However doing so does not require this issue, so I think it should be treated separately. It probably makes sense to do S3 + the instantiation changes for fields first though.

Generally speaking, I think S1, S2 and S3 can be implemented independently, thus could receive their own issues. We might want to do S2 before S1 though to ensure we do not run into confusing issues with $field->getParent() meanwhile.

I think S1-S3 would solve our problems and present a good way forward that's doable, as it works without re-writing the whole entity field implementation (what won't be feasible that late in the cycle anyway). Thoughts?

The list of "problems" in #51 only deals with value objects; But the same drawbacks are duplicated for "definition" objects. So I'd add:

4. the "definition" objects are primarily TypedData definition objects, instead of "just" being domain definition objects. That greatly complexifies FieldDefinition / Field / FieldInstance since:
- they have to extend DataDefinition, some methods of which make little sense for the domain
- they cannot be a self-contained collection of domain properties (field type, name, cardinality, settings...), but need to be split between a "definition" and an internal "item definition".

[edit: sorry, I posted that before getting to the end of #51. Basically you're saying :
- we could move to a FieldItemListDataDefinition + FieldItemDataDefinition, generated from the (domain) FieldDefinition object
- this would be made easier by #2002134: Move TypedData metadata introspection from data objects to definition objects
- this can be done independently of what is discussed above about data objects
Did I get that right ?]

StatusFileSize
new22.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Ok, trying to get this started, here's an initial patch that removes the ComplexDataInterface from ContentEntityInterface, which is the first and possibly easier part of S1, if I understood that right.

Added back whatever methods seemed useful directly to ContentEntityInterface, renamed property to field. That part looks a lot saner now I think :)

Everything else is obviously still very hackish, I extend the existing abstract typed data entity plugin class to implement ComplexDataInterface and wrap a content entity, not everything in there is implemented yet. Also a bunch of hacks at random places, but EntityFieldTest is passing with those changes now. There will obviously be lots of failures but that's ok ;)

I'm also fine with doing S2 first, but I wanted to see how the content entity API will look like when doing this and starting to understand the problems. For example, EntityFieldTest already nicely shows some of the drawbacks that @fago pointed out, like getRoot() returning the wrapper object and not the actual entity.

I also think that part of the problem of the definition objects was already addressed in the other issue, it's getDataDefinition() now I think and no longer getDefinition. No idea about the current state of the definition objects, though.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 53: typed-data-wrappers-53.patch, failed testing.

Status:Needs work» Needs review

53: typed-data-wrappers-53.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 53: typed-data-wrappers-53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new6.59 KB

Fixed a few more things, maybe this will be able to install?

Status:Needs review» Needs work

The last submitted patch, 58: typed-data-wrappers-58.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,754 pass(es), 564 fail(s), and 105 exception(s).
[ View ]
new4.87 KB

Fixing unit tests to get some real results. The crazy stuff that's going on in some unit tests is.. crazy ;)

Also removed a weird and wrong check in EntityNormalizer, there's no reason to ignore id fields? o.0

Status:Needs review» Needs work

The last submitted patch, 60: typed-data-wrappers-60.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new46.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,205 pass(es), 12 fail(s), and 7 exception(s).
[ View ]
new16.37 KB

Wow, hal entity serialization is fully of crazy stuff and hacks.

Found out why it had the !is_object() check in there... EntityReferenceItemNormalizer also passed in config entities, which only worked because it also used the included_fields option to only return the uuid. That then called something like $node_type->get('uuid'), which returned the UUID as a string, which then was filtered out again. So that somehow worked but resulted in incomplete definitions without UUID's for config entities I think.

For now, made that normalizer only apply to content entity references. Should probably be renamed as well, just like I renamed EntityNormalizer? There's an issue to add config entity serialization support, but we don't support them as they don't have validate() for example anyway...

Status:Needs review» Needs work

The last submitted patch, 62: typed-data-wrappers-62.patch, failed testing.

Still cannot look at code, but saw #2216569: Move Entity/TypedData normalization inline pass in my twitter feed. Might want to synchronize ?

According to this patch, setFieldValues() is only called in core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
In #2216569: Move Entity/TypedData normalization inline we discussed having EntityInterface::toArray(), which would basically be the same as getFieldValues().
If we rename getFieldValues here, we can rename ConfigEntityInterface::getExportProperties in another issue, and we'll finally have a unified way to get the values of an entity as an array!

StatusFileSize
new43.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,279 pass(es), 11 fail(s), and 7 exception(s).
[ View ]
new1.51 KB

Grml, spent way too much time debugging the Symfony Serializer because their normalizer cache is inconsistently used, so I incorrectly assumed my change could work. Not sure why there even are supports*() methods when the results is cached by class anyway, it would be much easier if it would just have methods to get the class and supported formats and check that.

Opened up https://github.com/symfony/symfony/pull/10457 to at least unify the use of the normalizer cache there.

Need a break now, will look into renaming the method tomorrow.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 66: typed-data-wrappers-66.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new44.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,431 pass(es).
[ View ]
new7.45 KB
new1.66 KB

Ok, hopefully fixed the remaining tests.

Removed setFieldValues() and renamed getFieldValues() to toArray().

Need some feedback on how to continue here... The patch proves that the adapter approach works, but does so by manually wrapping it every time it's necessary. should we have $entity->getDataWrapper() ? Should we maybe have an interface for that ?

To understand the serializer changes, see the attached patch between the output of entity/node/1 before and after the patch.

- Before, a broken node_type was in there, with an empty href and no uuid
- nid is now no longer in there, because we now check for the actual entity ID not just a field named "id".
- Looking at that, having the revision id in there is just as problematic, but it's not our problem to solve.

Related to config entity references: I think we should support to reference those by ID and provide a target id entity resolver. See what I did for to support that in the default_content module in https://github.com/md-systems/default_content/commit/e180dda7945be1b99a6....

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -491,9 +416,9 @@ public function getFieldDefinitions() {
    -  public function getPropertyValues() {
    +  public function toArray() {

    That rename is excellent, I very much like toArray() as it is immediately clear what it does.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -124,4 +124,69 @@ public function getFieldDefinition($name);
    +   * @param $field_name
    +   *   The name of the field to get; e.g., 'title' or 'name'.

    @param daty type is missing, also elsewhere.

  3. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityTypedDataWrapper.php
    @@ -0,0 +1,187 @@
    + * Note that the class only registers the plugin, and is actually never used.
    + * \Drupal\Core\Entity\Entity is available for use as base class.

    The class is never used, but it still contains methods? What's the point then? Please clarify.

  4. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityTypedDataWrapper.php
    @@ -0,0 +1,187 @@
    +class EntityTypedDataWrapper implements \IteratorAggregate, ComplexDataInterface {

    This reminds me of entity metadata wrappers in D7. Is that really what we want again? I guess I'm not up to speed with recent meta data improvements in core, but introducing wrappers again feels wrong. I assume that most entity API use cases now can be done very well without metadata inspection on the typed data level? Then special casing the typed data use cases with this wrapper might be the right thing.

The changes to the HAL serializer look fine, the original code is quite old and therefore might look weird.

3. & 4. The documentation and class name is bogus, I just did something to get started. It should very likely be called ...Adapter. Entity API introspection is equally capable as Typed Data, this just provides an Adapter for different API's/method names, which allows us to move methods on entities that make sense (getFields() instead of getProperties() and so on.

getProperties() -> toArray() is something that would make sense for ComplexDataInterface as well I think...

Issue tags:+KV Entity Storage

Postponed #2216569: Move Entity/TypedData normalization inline on this. Loving the toArray() name change!

Moved the hal cleanup to #2219795: Config entity references broken and other bugs, added tests and found even more bugs. Please review and help to get that in to keep the size of this down...

Filed #2219925: Rename ConfigEntityInterface::getExportProperties() to toArray(), whichever one of these gets in second should move toArray to EntityInterface

StatusFileSize
new34.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Re-rolled the patch now that the two issues went in.

Status:Needs review» Needs work

The last submitted patch, 76: typed-data-wrappers-76.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new34.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new569 bytes

Every time I'm too lazy to even to a basic check of the patch, this happens.

Status:Needs review» Needs work

The last submitted patch, 78: typed-data-wrappers-78.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new37.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,719 pass(es).
[ View ]
new2.97 KB

Fixing the unit tests.

Assigned:Unassigned» fago

Feedback plz!

StatusFileSize
new37.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,851 pass(es).
[ View ]

Reroll to handle the FieldableEntityStorageControllerBase -> ContentEntityStorageBase rename.

The patch proves that the adapter approach works, but does so by manually wrapping it every time it's necessary. should we have $entity->getDataWrapper() ?

It's hard to tell from the patch which of these wrappings are actually needed vs. temporary artifacts. For example:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -405,7 +328,8 @@ protected function getTranslatedField($name, $langcode) {
-        $field = \Drupal::typedDataManager()->getPropertyInstance($this, $name, $value);
+        $wrapper = new EntityTypedDataWrapper($this);
+        $field = \Drupal::typedDataManager()->getPropertyInstance($wrapper, $name, $value);
...
+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -913,7 +811,7 @@ public function __clone() {
-          $this->fields[$name][$langcode]->setContext($name, $this);
+          $this->fields[$name][$langcode]->setContext($name, new EntityTypedDataWrapper($this));

Is this only needed because the patch doesn't yet remove TypedData stuff from FieldItemList?

+++ b/core/lib/Drupal/Core/Field/FieldItemList.php
@@ -54,7 +55,7 @@ public function __construct(DataDefinitionInterface $definition, $name = NULL, T
-    return $this->getParent();
+    return $this->getParent() instanceof EntityTypedDataWrapper ? $this->getParent()->getEntity() : $this->getParent();

Why is this needed?

should we have $entity->getDataWrapper() ?

In my opinion, no, at least not a public one. A protected toTypedData() would be fine, if ContentEntityBase needs it as its implementation detail for how it chooses to implement validate(), but the whole point of adapters is that it's not up to the adaptee's interface to have any knowledge of them. Instead, what about adding an adapt(mixed $value) method to TypedDataManager, and then have a tagged service for each possible type of $value, with TypedDataManager finding the appropriate one to call (or calling all of them until the first one returns non-NULL), similar to how we implement normalizers and cache contexts.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -405,7 +328,8 @@ protected function getTranslatedField($name, $langcode) {
    +        $wrapper = new EntityTypedDataWrapper($this);
    +        $field = \Drupal::typedDataManager()->getPropertyInstance($wrapper, $name, $value);

    Should we start wrapping this logic in a place that makes sense already, e.g. on the field type manager?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -913,7 +811,7 @@ public function __clone() {
    +          $this->fields[$name][$langcode]->setContext($name, new EntityTypedDataWrapper($this));

    I think we should not hardcode the EntityTypedDataWrapper class, but use the 'entity' data type classes registered in the typed data manager instead.

    So you use the typed data manager to instantiate a typed data wrapper of an entity given its typed data type and $entity.

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -135,4 +135,58 @@ public function getFieldDefinitions();
    +   *   The value to set, or NULL to unset the field.
    ...
    +   *   If the specified field does not exist.

    It would be good to describe what kind of $values are supported here.

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -341,7 +342,7 @@ public function onBundleDelete($bundle) { }
    +      $items = \Drupal::typedDataManager()->create($instance, $values, $instance->getName(), new EntityTypedDataWrapper($entity));

    It sucks to have to instantiate the parent wrapper just for being able to have a reference on it :/

    We should be able to improve this when constructing fields via the field manager though. I could see us doing $data_type_class::createFromDataDefinition() for defining how you can construct your class via typed data, which then can re-route to the usual, data type dependent construction logic, e.g. in the field type manager.

  5. +++ /dev/null
    @@ -1,26 +0,0 @@
    -abstract class Entity {

    Yes, it's great to see that!

  6. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityTypedDataWrapper.php
    @@ -0,0 +1,187 @@
    + * Note that the class only registers the plugin, and is actually never used.

    Deprecated :)

  7. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityTypedDataWrapper.php
    @@ -0,0 +1,187 @@
    +class EntityTypedDataWrapper implements \IteratorAggregate, ComplexDataInterface {

    Maybe go with EntityWrapper again?

    Having TypedData in there doesn't read very well, and the interfaces should tell you.

  8. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityTypedDataWrapper.php
    @@ -0,0 +1,187 @@
    +      debug(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS));

    debug

  9. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityTypedDataWrapper.php
    @@ -0,0 +1,187 @@
    +  public function setValue($value, $notify = TRUE) {

    We'll need to document acceptable values for entities. In d7 we support entity objects and ids, what was very useful. I guess we should do that as well what allows us to clean up the "entity_reference" data type class then to work like the language_reference.

  10. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityReferenceItemNormalizer.php
    @@ -73,6 +73,10 @@ public function normalize($field_item, $format = NULL, array $context = array())
    +    if (empty($embedded['_links']['self'])) {

    Deprecated now since that got solved?

should we have $entity->getDataWrapper() ? Should we maybe have an interface for that ?

I agree with effulgentsia, we shouldn't. You shouldn't have much business in doing so, and if, you are still able to instantiate it via the TDM factory. This means, you have to know the data type to use at least, which we could supported detecting based upon interfaces, e.g. you declare a matching interface in the data type annotation which then the TDM can use to lookup the type / definition.
What about doing an optional interface for providing a data definition for an object, e.g. DataDefinitionProviderInterface or so? We won't be able to rely on it being there, so not sure it helps a lot. So maybe better, we could have equivalent functionality on the data type class, i.e. you have to know or be able to derive the data type again (what should be ok I think). Then, the TDM can let the data type class provide a decent data definition for your object + provide an public API for that:
TDM->getDataDefintion($type, $value) ?
So, instantiation of typed data wrappers via the TDM could work like that:
TDM->createInstance($type, $value), which uses TDM->getDataDefintion($type, $value) and then $class::createFromDataTypeDefinition(). Alternatively, we can keep having the public API TDM::create($definition, $value) when you already have the definition.

$class::getDataDefintion($value) I see mostly useful for cases like entities, where we can come up with a definition that contains more than just the basic 'entity' type for some $value. Not sure there are many uses besides that, but having it only do a DataDefinition::create($type) in most cases should be fine.

Instead, what about adding an adapt(mixed $value) method to TypedDataManager, and then have a tagged service for each possible type of $value, with TypedDataManager finding the appropriate one to call (or calling all of them until the first one returns non-NULL), similar to how we implement normalizers and cache contexts.

Interesting idea. So instead of denoting the interface in the annotation, you'd implement a method for checking it yourself? Sounds like this could be more efficient and/or at least simpler to write than doing that logic generically based on the metadata. Not sure whether having the interface in the metadata would have other use cases besides that also? (Cannot think of one right now).