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().

Patch summary :

- ContentEntityInterface no more extends ComplexDataInterface (i.e is no more a TypedData object)
- A first set of methods are removed from ContentEntityBase:
getDataDefinition()
getValue()
setValue()
getString()
applyDefaultValue()
isEmpty()
getConstraints()
getName()
getRoot()
getPropertyPath()
getParent()
setContext()
- Most of those go into a new TypedData\EntityAdapter class, that extends TypedData (or we just rely on the default implementation in TypedData)
- A second set of methods remain in ContentEntityBase, and are added to ContentEntityInterface directly, in some cases with more specific docs / param names :
get($field_name)
set($field_name, $value, $notify = TRUE)
getFields($include_computed = FALSE)
onChange()
validate()

- EntityInterface::getTypedData() is added to obtain the TypedData\EntityAdapter for an entity

- Adds TypedDataInterface::createInstance() static factory, and a default implementation in TypedData
TypedDataManager::createInstance() now uses that instead of new $class();

- moves TypedData\ReadOnlyException to TypedData\Exception\ReadOnlyException
renames/move TypedData\MissingContextException to TypedData\Exception\MissingDataException
removes DataDefinitionException (was actually not used anywhere)

Remaining tasks

None

User interface changes

None

API changes

Since TypedDataInterface is not on ContentEntityInterface any more, most of its methods got removed from entities, while some got replacements - e.g. getProperties() got replaced by getFields(). For any usage of a removed method, one now has to fetch the typed data adapter object for the entity and invoke the method on it like the following:

$definition = $entity->getDataDefinition();
// becomes
$definition = $entity->getTypedData()->getDataDefinition();

This is for example needed by the FieldItemList class which has to work with the parent typed data object - however those usages are mostly obsolete once/if we'd fix #2227227: FieldTypePluginManager cannot instantiate FieldType plugins, good thing TypedDataManager can instantiate just about anything and improve the field item list's implementation to be field-related primarily.

In detail the API changes are:
- ContentEntityInterface no more extends ComplexDataInterface (i.e is no more a TypedData object)
- A first set of methods are removed from ContentEntityBase:
Removed:

  • getDataDefinition()
  • getValue()
  • setValue()
  • getString()
  • applyDefaultValue()
  • isEmpty()
  • getConstraints()
  • getName()
  • getRoot()
  • getPropertyPath()
  • getParent()
  • setContext()

Re-moved but re-added to ContentEntityInterface methods with a new name or docs are:

  • get($field_name)
  • set($field_name, $value, $notify = TRUE)
  • getFields($include_computed = FALSE)
  • onChange()
  • validate()

Further TypedData-related smaller API changes are:

  • TypedDataInterface::createInstance() is added, however most implementations extend TypedData and do not need to be changed.
  • The TypedData exceptions change a bit:
    • TypedData\ReadOnlyException -> TypedData\Exception\ReadOnlyException (moves into new sub-namespace Exception)
    • TypedData\Exception\MissingDataException gets added
    • The unused DataDefinitionException is removed

#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.

CommentFileSizeAuthor
#137 d8_typed_adapter.interdiff.txt5.26 KBfago
#137 d8_typed_adapter.patch71.7 KBfago
#134 interdiff.txt4.29 KByched
#134 2002138-typed_data_adapters-134.patch72.16 KByched
#132 interdiff.txt2.31 KByched
#132 2002138-typed_data_adapters-131.patch69.97 KByched
#130 interdiff.txt17.37 KByched
#130 2002138-typed_data_adapters-130.patch69.77 KByched
#129 interdiff.txt7.59 KByched
#129 2002138-typed_data_adapters-129.patch68.85 KByched
#128 d8_typed_adapter.interdiff.txt16.13 KBfago
#128 d8_typed_adapter.patch68.71 KBfago
#126 d8_typed_data_adapter-2002138-126-interdiff.txt662 bytesBerdir
#126 d8_typed_data_adapter-2002138-126.patch56.98 KBBerdir
#124 d8_typed_data_adapter-2002138-124-interdiff.txt687 bytesBerdir
#124 d8_typed_data_adapter-2002138-124.patch56.33 KBBerdir
#118 d8_typed_data_adapter-2002138-118-interdiff.txt5.87 KBBerdir
#118 d8_typed_data_adapter-2002138-118.patch56.32 KBBerdir
#112 d8_typed_data_adapter-2002138-112.patch55.39 KBBerdir
#109 d8_typed_data_adapter.patch56.5 KBfago
#109 d8_typed_data_adapter.interdiff.txt12.02 KBfago
#107 typed_data.patch56.43 KBfago
#103 d8_typed_data_adapter.patch47.5 KBfago
#102 2138-typed-entity-102.patch18.57 KBJose Reyero
#98 typed-data-2002138-98-interdiff.txt902 bytesBerdir
#98 typed-data-2002138-98.patch32.94 KBBerdir
#95 typed-data-2002138-95.patch32.11 KBxjm
#93 typed-data-2002138-93.patch33.18 KBxjm
#91 d8_typed_data_adapter.patch33.21 KBandypost
#91 interdiff.txt675 bytesandypost
#88 d8_typed_data_adapter.patch33.28 KBfago
#82 typed-data-wrappers-82.patch37.73 KBmsonnabaum
#80 typed-data-wrappers-80-interdiff.txt2.97 KBBerdir
#80 typed-data-wrappers-80.patch37.78 KBBerdir
#78 typed-data-wrappers-78-interdiff.txt569 bytesBerdir
#78 typed-data-wrappers-78.patch34.8 KBBerdir
#76 typed-data-wrappers-76.patch34.81 KBBerdir
#69 rest_diff.txt1.66 KBBerdir
#69 typed-data-wrappers-69-interdiff.txt7.45 KBBerdir
#69 typed-data-wrappers-69.patch44.55 KBBerdir
#66 typed-data-wrappers-66-interdiff.txt1.51 KBBerdir
#66 typed-data-wrappers-66.patch43.29 KBBerdir
#62 typed-data-wrappers-62-interdiff.txt16.37 KBBerdir
#62 typed-data-wrappers-62.patch46.28 KBBerdir
#60 typed-data-wrappers-60-interdiff.txt4.87 KBBerdir
#60 typed-data-wrappers-60.patch31.64 KBBerdir
#58 typed-data-wrappers-58-interdiff.txt6.59 KBBerdir
#58 typed-data-wrappers-58.patch26.87 KBBerdir
#53 typed-data-wrappers-53.patch22.72 KBBerdir
#33 2002138-remove-typeddatainterface-32.patch50.06 KBBerdir
#33 2002138-remove-typeddatainterface-32-interdiff.txt511 bytesBerdir
#30 2002138-remove-typeddatainterface-29.patch49.98 KBBerdir
#21 2002138-remove-typeddatainterface-test4.patch123.51 KBdixon_
#21 2002138-remove-typeddatainterface-test4-interdiff.txt54.56 KBdixon_
#18 2002138-remove-typeddatainterface-17.patch30.54 KBdixon_
#18 2002138-remove-typeddatainterface-17-interdiff.txt8.57 KBdixon_
#10 2002138-remove-typeddatainterface-10.patch21.99 KBdixon_
#4 2002138-remove-typeddatainterface-do-not-test.patch7.28 KBdixon_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

tagging

Berdir’s picture

Priority: Normal » Major

Promoting to major as discussed with @xjm.

dixon_’s picture

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?

dixon_’s picture

Status: Active » Needs work
FileSize
7.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.

fago’s picture

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.

dixon_’s picture

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.

Berdir’s picture

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

fago’s picture

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).

dixon_’s picture

Assigned: Unassigned » dixon_

I have a patch coming up soon.

dixon_’s picture

Status: Needs work » Needs review
FileSize
21.99 KB

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.

dixon_’s picture

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.

dixon_’s picture

Status: Needs work » Needs review

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

Berdir’s picture

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

fago’s picture

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 ?

dixon_’s picture

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 to make typed data discoverable, 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.

fago’s picture

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.
dixon_’s picture

Status: Needs work » Needs review
FileSize
8.57 KB
30.54 KB

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

Berdir’s picture

+++ 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.

dixon_’s picture

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: Typed config incorrectly implements Typed Data interfaces

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

jibran’s picture

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

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

Berdir’s picture

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()

webchick’s picture

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)?

webchick’s picture

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.

yched’s picture

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

alexpott’s picture

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

Berdir’s picture

Status: Needs work » Needs review

Re-roll.

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

Berdir’s picture

Assigned: alexpott » Unassigned
FileSize
49.98 KB

Patch would help.

Status: Needs review » Needs work

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

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
511 bytes
50.06 KB
smiletrl’s picture

+++ 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.

smiletrl’s picture

Not sure shout this getDefinition()., but maybe

$definition = $data->getDefinition();

convert to something like:

$definition = \Drupal:typedData()->getDefinition($data->getPluginId());
msonnabaum’s picture

Status: Needs review » Needs work

core/lib/Drupal/Core/TypedData/TypedDataManager.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:

   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.

jibran’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Issue tags: +sprint

Tagging for rocketship focus

yched’s picture

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...

twistor’s picture

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.

smiletrl’s picture

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:)

yched’s picture

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

fago’s picture

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

fago’s picture

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 to make typed data discoverable). 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.

fago’s picture

Title: Remove TypedDataInterface » Use 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 to make typed data discoverable 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.

fago’s picture

Issue summary: View changes

updated issue summary

fago’s picture

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.

effulgentsia’s picture

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?

fago’s picture

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'.

fago’s picture

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?

yched’s picture

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 ?]

Berdir’s picture

FileSize
22.72 KB

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.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
26.87 KB
6.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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
31.64 KB
4.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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
46.28 KB
16.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.

yched’s picture

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

tim.plunkett’s picture

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!

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
44.55 KB
7.45 KB
1.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....

klausi’s picture

  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.

Berdir’s picture

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...

tim.plunkett’s picture

Issue tags: +KV Entity Storage

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

Berdir’s picture

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...

tim.plunkett’s picture

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

tim.plunkett’s picture

Berdir’s picture

FileSize
34.81 KB

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
34.8 KB
569 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
37.78 KB
2.97 KB

Fixing the unit tests.

Berdir’s picture

Assigned: Unassigned » fago

Feedback plz!

msonnabaum’s picture

FileSize
37.73 KB

Reroll to handle the FieldableEntityStorageControllerBase -> ContentEntityStorageBase rename.

effulgentsia’s picture

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?

effulgentsia’s picture

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.

fago’s picture

  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).

fago’s picture

I've created issues based on the plan layed out in #51, whereas I think

S1: Use wrapping typed data objects for implementing typed data for entities.

is the job of this issue to establish.

Created issues:
#2268031: Name typed data interface methods unambiguously
#2268009: Make typed data object instantiation more flexible
#2268049: Decouple field definitions from typed data definitions

In regards to beta, I think we should have:
- Either fixed this one or mark to-be-removed methods as deprecated, such that are not confusing any more.
- #2268031: Name typed data interface methods unambiguously
- #2268009: Make typed data object instantiation more flexible should be able to go in after beta I think?
- However, #2268049: Decouple field definitions from typed data definitions needs it and is an API change visible to people. Not sure whether it would be applicable to mark to-be-removed methods as deprecated and mark them for removal + remove them after beta?

fago’s picture

Status: Needs review » Needs work

This actually needs work.

fago’s picture

Assigned: fago » Unassigned
Status: Needs work » Needs review
FileSize
33.28 KB

Quickly re-rolled the patch, it seems to be broken now though.
Also, unassigning myself while not actively working on it.

Status: Needs review » Needs work

The last submitted patch, 88: d8_typed_data_adapter.patch, failed testing.

xjm’s picture

Issue tags: +beta deadline

Discussed with @effulgentsia, @fago, @alexpott, @berdir. It's a potential that parts of this could go in after the beta, but we need to figure out which parts those are before it. ;)

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, 91: d8_typed_data_adapter.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
33.18 KB

Another reroll. It's probably still broken.

Status: Needs review » Needs work

The last submitted patch, 93: typed-data-2002138-93.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
32.11 KB

Sigh.

Status: Needs review » Needs work

The last submitted patch, 95: typed-data-2002138-95.patch, failed testing.

xjm’s picture

There we go. :) The full exception is:

InvalidArgumentException: Property langcode is unknown. in Drupal\Core\TypedData\TypedDataManager->getPropertyInstance() (line 281 of core/lib/Drupal/Core/TypedData/TypedDataManager.php).

Berdir’s picture

Status: Needs work » Needs review
FileSize
32.94 KB
902 bytes

This should fix that, we added that check a while ago while fixing a bug when calling that for config entities, will need a better check...

Didn't check anything else.

Status: Needs review » Needs work

The last submitted patch, 98: typed-data-2002138-98.patch, failed testing.

Jose Reyero’s picture

Sorry to interrupt, but this issue just caught my attention.

Really, though I've read very good ideas on the first posts of the issue, like simplifying and breaking down the interfaces, it seems that those are long forgotten and we have ended up with some wrapper as the last resource. Well, I'd rather have interfaces fixed (and IMO it's not TypedDataInterface that needs to be dropped -simplified yes-, but ComplexDataInterface whose same says it all.)

Or maybe just the idea that the container of typed data structures (Entity) must be a typed data itself?

Anyway this is better than nothing, so down to the patch, which is at least some improvement.

+ * Contains \Drupal\Core\Entity\Plugin\DataType\EntityTypedDataWrapper.
+/**
+ * Defines the base plugin for deriving data types for entity types.
+ *
+ * Note that the class only registers the plugin, and is actually never used.
+ * \Drupal\Core\Entity\Entity is available for use as base class.
+ *
+ * @DataType(
+ *   id = "entity",
+ *   label = @Translation("Entity"),
+ *   description = @Translation("All kind of entities, e.g. nodes, comments or users."),
+ *   derivative = "\Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver",
+ *   definition_class = "\Drupal\Core\Entity\TypedData\EntityDataDefinition"
+ * )
+ */
+class EntityTypedDataWrapper implements \IteratorAggregate, ComplexDataInterface {

Why does the wrapper need to be a (never used) plugin?.

I think 'entity' could very well be a valid data type and we could save the 'Wrapper' name. Otherwise, if you just want it to be some 'wrapper' (though TypedData objects are all wrappers, aren't they?), then it doesn't need to be a new plugin.

+  /**
+   * {@inheritdoc}
+   */
+  public function setPropertyValues($values) {
+    // TODO: Implement setPropertyValues() method.
+  }

Understandable, many TODOs in the patch yet, but I hope we are not committing any code with more TODOs...

-        if (!in_array('Drupal\Core\TypedData\TypedDataInterface', class_implements($entity_type_class))) {
+        if (!in_array('Drupal\Core\Entity\ContentEntityInterface', class_implements($entity_type_class))) {

I know it was already there, but since we are changing that line, can't we use 'instanceof' instead?

   public function getEntity() {
-    return $this->getParent();
+    return $this->getParent() instanceof EntityTypedDataWrapper ? $this->getParent()->getEntity() : $this->getParent();
   }

What kind of logic is this? Is this a wrapper or is this an entity? We should know I mean..
Or maybe the method doesn't belong here?, just wondering...

Berdir’s picture

Or maybe just the idea that the container of typed data structures (Entity) must be a typed data itself?

That is *exactly* what this issue aims to do. But then we need a way to interact with those non-longer-typed-data-objects as typed data, which is what the wrapper approach is for.

The initial ideas around "just remove the interfaces" never made sense to me, we do need a generic way to interact with this data. Context is making more and more use of this, as one example, as will rules, generic tokens, search api and many other places like those.

The actual patch is just a proof of concept that needs more work, it's far from complete/working and I'm aware it's full of hacks. No, it will not be committed like that ;)

- The docblock on the entity class is outdated and was there before where it really wasn't used. Now it is.

- instanceof only works on class instances, what we have there is a class name as a string,

- the instanceof check is exactly the kind of stuff where I got stuck ;)

Jose Reyero’s picture

FileSize
18.57 KB

Posting an incomplete patch just in case it is any use, no time to finish it any time soon -also not familiar enough with Entity/Field APIs.

Based on @Berdir's 98, the idea is as follows:

- TypedData itself is a wrapper or an adapter so no need for an extra 'adapter' concept.

- Just as Integer wraps an int value, this wraps a value that is an entity. Thus the class/plugin name should be 'Entity' (or maybe ContentEntity)

These are the relevant bits of the code:

class Entity extends TypedData implements \IteratorAggregate, ComplexDataInterface {
   // There are only a few methods we need to implement/override.
}
class FieldItemList extends ItemList implements FieldItemListInterface {
  /**
   * {@inheritdoc}
   */
  public function getEntity() {
    return $this->getParent()->getValue();
  }
}
/**
 * @file
 * Contains \Drupal\Core\Entity\ContentEntityBase.
 */

...

use Drupal\Core\Entity\Plugin\DataType\Entity as EntityTypedDataWrapper;

....

What I find really confusing is the 'EntityReference' plug-in. That's not a data type at all and it should be better renamed (Maybe to EntityId if the value is an entity id) or dropped because DataType/Entity could very well implement the DataReferenceInterface. But really the thing is as confusing as having a 'String' and a 'StringReference' data types to hold the same kind of value.

Then there's EntityDeriver, I have no idea what we need it for if we already have an 'Entity' data type.

Anyway, seeing it again, I think @Berdir's patch in #98 is very well aimed, just needs to clarify when it uses TypedData/Entity and when it is a Drupal/Core/Entity instead. But it really cleans up a lot of the entity interfaces.

(Not marking this patch 'for review' because it's just a few pieces of example code, won't pass any test)

fago’s picture

Status: Needs work » Needs review
FileSize
47.5 KB

As discussed with berdir, I've been working on this and finally came back again to it today. I've no green patch yet but it's a start and should be enough for architecturial reviews.

I'd agree with Jose that we should not call it EntityTypedDataWrapper but follow the typed data naming scheme. So I'd go with an "Entity" class as well, which is the "Entity" data type - it's below the data type namespace anyway. That matches what we do in other places already, e.g. we have a NodeType entity class and a NodeType condition class.

The instantiation of that "Entity" data type has to work via the regular TDM factory - so I changed the Entity data type class accordingly. For creating the class from a given entity object, I've just added another static create method: createFromEntity() what creates the right data definition and instantiates the typed object as usual.

Then, I think we should add a getTypedData() method to the entity interface what makes it easy to get a typed data object from an entity and work from that. For not that's mostly needed by context and validator stuff, but it makes the conversion way easier.

Coming back to naming, I think we should to refer to typed data objects wrapping entities as $typed_entity as this goes along the naming of the Typed Data API, where "data" is the generic concept. Thus, in case of the data being an entity it makes sense to use the name $typed_entity to me. I agree it's not very nice, but that is as the name "Typed data" is not very nice - still it's the name so imo we should work with that and avoid inventing new names.

Attached patch seems to basically work, tests are not green yet. I had some troubles with entity serialization in form workflows when the static cache in Entity::getTypedData is enabled. Not sure what's going wrong here, so I quickly disabled the static cache for now.

What's left is:
- fix the static cache and tests
- update the entity_reference implementation
- reviews and get it in

fago’s picture

What I find really confusing is the 'EntityReference' plug-in. That's not a data type at all and it should be better renamed (Maybe to EntityId if the value is an entity id) or dropped because DataType/Entity could very well implement the DataReferenceInterface. But really the thing is as confusing as having a 'String' and a 'StringReference' data types to hold the same kind of value.

Well, the point of the data reference is that it can give you the reference data and/or its definition, while it's not defined / hard-coded how the reference works. Yes, often the reference is id-based but that's not necessarily the case, it might be some combination or a backward reference via a computed field.

StringReference does not make much sense anyway, however yeah - at a first glance I agree that "entity_reference" does not seem to be a separate type but could be just "entity". I've been thinking quite a bit about this though and I came to no good solution that handles both the entity and a referenced entity in a single plugin - separating it into two plugins just makes things way more stream-lined code-wise. Moreover, it's not only that as there is quite a big semantic difference between changing a referenced entity and a (stand-a-lone) entity variable - so the reference cannot be the same object as the regular typed data entity object.

Status: Needs review » Needs work

The last submitted patch, 103: d8_typed_data_adapter.patch, failed testing.

fago’s picture

Assigned: Unassigned » fago
fago’s picture

Assigned: fago » Unassigned
Status: Needs work » Needs review
FileSize
56.43 KB

I've been working on it at #2330525: Test helper issue for use adapters for supporting typed data to get it green - so here's a first green patch, without a static typed entity property.
todo:
- reviews :)
- Typed data related parts of ContentEntityBaseUnitTest need to be moved to a new unit test though.
- fix the static typed entity property to work

Notes:
- Validation constraints DX of the entity type and bundle constraint became worse as it's working on both the entity_reference and entity data types, but latter is passed wrap while the former one isn't. Not sure this is an issue as it's quite special to the EntityType and Bundle constraints to be used with both.

Berdir’s picture

Happy to see this green again :) Feedback/Questions below.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1015,11 +890,11 @@ public function referencedEntities() {
             foreach ($field_item->getProperties(TRUE) as $property) {
    -          if ($property instanceof EntityReference && $entity = $property->getTarget()) {
    +          if ($property instanceof EntityReference && $entity = $property->getValue()) {
                 $referenced_entities[] = $entity;
    

    What's the difference between getTarget() and getValue()? Don't remember if this was already here before (added by me) or not :)

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -547,4 +554,23 @@ public function toArray() {
    +      //$this->typedData = $class::createFromEntity($this);
    +      return $class::createFromEntity($this);
    

    Looks like the static cache is currently disabled? Ah yes, you said that.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -359,6 +359,19 @@ public function setOriginalId($id);
    +  public function getTypedData();
    

    What exactly do you get when you do this for a config entity?

  4. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
    @@ -21,6 +31,144 @@
    +    // @todo: Add support for config entities.
    +    throw new MissingDataException(String::format('Unable to get property @name as no entity has been provided.', array('@name' => $property_name)));
    

    I see, this happens ;)

  5. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
    @@ -21,6 +31,144 @@
    +   * {@inheritdoc}
    +   */
    +  public function getProperties($include_computed = FALSE) {
    +    if (isset($this->entity)) {
    +      return $this->entity->getFields($include_computed);
    +    }
    +    throw new MissingDataException(String::format('Unable to get properties as no entity has been provided.'));
    +  }
    

    This also needs a check then on the interface, otherwise we get a fatal error?

    Instaed of checks everywhere, we could also have two classes, a base class and one for content entities, the base class would just unconditionally throw a NotImplementedException or something?

  6. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
    @@ -21,6 +31,144 @@
    +  public function isEmpty() {
    +    return !isset($this->entity);
    +  }
    +
    

    This is different from how isEmpty() on the entity itself worked but I guess it makes sense, an entity is never really going to be empty as soon as you have a single default value.

  7. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReference.php
    @@ -82,22 +82,17 @@ public function getTargetIdentifier() {
    -  public function getValue() {
    -    // Entities are already typed data, so just return that.
    -    return $this->getTarget();
    -  }
    

    Ah, getTarget() returns the typed data entity, getValue() the actual entity object.

  8. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/BundleConstraintValidator.php
    @@ -16,10 +17,15 @@
    +    // @todo: Work over how metadata unwraps typed data or now.
    +    $entity = $typed_entity instanceof TypedDataInterface ? $typed_entity->getValue() : $typed_entity;
    +    if (!in_array($entity->bundle(), $constraint->getBundleOption())) {
           $this->context->addViolation($constraint->message, array('%bundle' => implode(', ', $constraint->getBundleOption())));
         }
    

    That @todo sense makes not. :)

  9. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
    @@ -89,9 +89,13 @@ public function getDataType() {
    -      // Append the bundle only if we know it for sure.
    +      // Append the bundle only if we know it for sure and it is not the default
    +      // bundle.
    

    Not sure if "default bundle" is the right term here. Maybe fallback bundle? nobundle bundle? :p

  10. +++ b/core/lib/Drupal/Core/TypedData/ListInterface.php
    @@ -54,6 +54,9 @@ public function onChange($delta);
    +   * @throws \Drupal\Core\TypedData\MissingDataException
    +   *   If the complex data structure is unset and no item can be created.
    +   *
        * @return \Drupal\Core\TypedData\TypedDataInterface
    

    @throws should be below @return I think? Same for others in this interface.

  11. +++ b/core/lib/Drupal/Core/TypedData/TypedData.php
    @@ -44,6 +44,13 @@
    +   * {@inheritdoc}
    +   */
    +  public static function createInstance($definition, $name = NULL, TypedDataInterface $parent = NULL) {
    +    return new static($definition, $name, $parent);
    +  }
    

    Oh, so now we're also allowing to inject stuff here? There's an option issue for this somewhere, which we can then close? Then we should add the DependencySerializationTrait to TypedData.

  12. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -73,6 +73,10 @@ public function normalize($field_item, $format = NULL, array $context = array())
    +    // @todo Config entities currently can not be serialized, skip them.
    +    if (empty($embedded['_links']['self'])) {
    +      return array();
    +    }
    

    I think I added this, if you look up in this mehtod, there's now an interface check for this and it falls back to the parent, so you should be able to remove this.

fago’s picture

What's the difference between getTarget() and getValue()? Don't remember if this was already here before (added by me) or not :)

getValue() contains the value of the data reference, while target gives you a typed data object of the referenced data. That's the pre-existing API of typed data references, but before this patch the value of an entity reference was typed data just as getTarget(), after this patch the value is the entity object while the target is the typed entity.

Ah, getTarget() returns the typed data entity, getValue() the actual entity object.

exactly :)

What exactly do you get when you do this for a config entity?

A typed data object wrapping the entity as usual. But given the current lack of config entity typed data support, the typed data object won't be able to provide any metadata about properties. Still, we can easily bridge the existing typed-config metadata though (and make sure it's proper typed data if it isn't yet) in a follow-up and finally have proper typed data support for config entities.

I see, this happens ;)

Yep. The error message is not fully correct though, fixed that anlong with 6.

This is different from how isEmpty() on the entity itself worked but I guess it makes sense, an entity is never really going to be empty as soon as you have a single default value.

Yep, I think it would be unexpected to have it be "empty" if there is an entity without any values - so that should be better.

That @todo sense makes not. :)

Corrected it is now.

Not sure if "default bundle" is the right term here. Maybe fallback bundle? nobundle bundle? :p

Right - problem here is that we have neither words for it or even standardized/documented the behaviour. Reminds me of: #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles. Do you agree with my conclusions there?

@throws should be below @return I think? Same for others in this interface.

Right, fixed.

Oh, so now we're also allowing to inject stuff here? There's an option issue for this somewhere, which we can then close? Then we should add the DependencySerializationTrait to TypedData.

Yep, I think so. It was required to instantiate Entity in a previous version of the patch, but meanwhile it works with the standard constructor. Thus, we could extract that part into another issue? -> #2268009: Make typed data object instantiation more flexible

I think I added this, if you look up in this mehtod, there's now an interface check for this and it falls back to the parent, so you should be able to remove this.

ok, let's try without it.

chx’s picture

I would recommend the word proxy . As in AccountProxy or http://en.wikipedia.org/wiki/Proxy_pattern

effulgentsia’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
55.39 KB

Re-rolled. Only conflicted in UserUniqueValidator with a git rebase, looks like the change there is no longer needed.

Status: Needs review » Needs work

The last submitted patch, 112: d8_typed_data_adapter-2002138-112.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 102: 2138-typed-entity-102.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityTypeConstraintValidator.php
@@ -23,7 +23,9 @@ public function validate($typed_entity, Constraint $constraint) {
     }
-    // @todo: Work over how metadata unwraps typed data or now.
+    // The constraint is used on both entity references and typed data entity
+    // objects. While entities are not automatically unwrapped, entity
+    // references are.
     $entity = $typed_entity instanceof TypedDataInterface ? $typed_entity->getValue() : $typed_entity;

@effulgentsia: This is the most problematic part about this issue fago and I think. This adds complexity to validators.

Wim Leers’s picture

Status: Needs review » Needs work

Berdir asked me to review this. Please know that I don't know Typed Data at all, so I reviewed this "from the surface", i.e. mostly the Entity API changes.

Lots of minor/nitpicky things, hope that's okay.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -152,4 +152,66 @@ public function getFieldDefinitions();
    +   * Gets a field item list.
    ...
    +   * Sets a field value.
    

    I understand where these terms come from, but it's confusing that they don't match.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -152,4 +152,66 @@ public function getFieldDefinitions();
    +   * @param $field_name
    ...
    +   * @param $field_name
    ...
    +   * @param $field_name
    

    string

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -152,4 +152,66 @@ public function getFieldDefinitions();
    +   *   The name of the property to set; e.g., 'title' or 'name'.
    

    s/property/field/

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -152,4 +152,66 @@ public function getFieldDefinitions();
    +   * @param $value
    

    mixed|null

  5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -152,4 +152,66 @@ public function getFieldDefinitions();
    +   * React to changes to a child field.
    

    s/React/Reacts/

    Child field? We don't call it a "child" field anywhere else?

    I guess "child" was added here to stress that this is not called when a field in a referenced entity is changed. But I think that's self-evident, and if it's not, I suggest including a line about that in the docblock, but not calling this "child field".

  6. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -152,4 +152,66 @@ public function getFieldDefinitions();
    +  public function validate();
     }
    

    Missing newline.

  7. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -8,6 +8,7 @@
    +use Drupal\Core\Field\PrepareCacheInterface;
    

    Blast from the past :) This is no more! Must be a remnant :)

  8. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -25,7 +23,9 @@
    +  use DependencySerializationTrait {
    +    __sleep as traitSleep;
    +  }
    

    Wow, never seen this syntax before!

  9. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -547,4 +554,23 @@ public function toArray() {
    +      //$this->typedData = $class::createFromEntity($this);
    

    Dead code that can be removed? Or…?

  10. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -547,4 +554,23 @@ public function toArray() {
    +  }
     }
    

    Missing newline.

  11. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -359,6 +359,19 @@ public function setOriginalId($id);
    +   * Returns a typed data object for the entity object.
    

    s/the/this/?

  12. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -359,6 +359,19 @@ public function setOriginalId($id);
    +   * The returned typed data objects wraps the entity objects and allows dealing
    +   * with entities based on the generic typed data API.
    

    Why all the plural "objects"?

  13. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
    @@ -7,11 +7,21 @@
    + * based upon the typed data API.
    

    s/typed data API/Typed Data API/?

  14. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
    @@ -7,11 +7,21 @@
    + * In addition to the "entity" data type, there is derivative which exposes
    + * "entity:$entity_type" and "entity:$entity_type:$bundle" data types.
    

    And these provide… what exactly?

  15. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
    @@ -21,6 +31,151 @@
    +      // @todo: Add support for config entities.
    ...
    +      // @todo: Add support for config entities.
    ...
    +      // @todo: Add support for config entities.
    

    Follow-up or still in this issue?

  16. +++ b/core/lib/Drupal/Core/TypedData/TypedDataInterface.php
    @@ -19,6 +19,26 @@
    +   * @param string $name
    

    string|null

  17. +++ b/core/lib/Drupal/Core/TypedData/TypedDataInterface.php
    @@ -19,6 +19,26 @@
    +   * @param \Drupal\Core\TypedData\TypedDataInterface $parent
    

    Typehint should be updated to allow null.

Berdir’s picture

Status: Needs work » Needs review
FileSize
56.32 KB
5.87 KB

1. They don't match because they are different. get() returns FieldItemList objects, set() supports any possible value, from NULL over scalars, a single field item array to multiple field item arrays and even FIeldItemList objects.
2. Fixed.
3. Fixed.
4. mixed already includes null, so the only reason would be to explicitly expose that, and I'd rather not because that behavior is weird and only needed for serialization crazyness.
5. Hm, I'm actually not sure it is Reacts. In a way, this is similar to a hook, where we afaik do not use the third person. But other onSomething() methods use Reacts, so Reacts it is. Removed child.
6. Fixed.
7. Yeah, that totally blasted in ;) Removed.
9. This is supposed to be enabled I think (statically cache the typed data object) but apparently broke things. Let's see what's still breaking.
10. Fixed.
11/12. Cleaned that up.
13. Yep.
14. These exist as typed data types. This is for example used for context, see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Annotatio... for examples.
15. What you can see here is a living example of a "wishful thinking" @todo :) No, not in here.
16. Fixed.
17. Fixed.

Thanks!

Status: Needs review » Needs work

The last submitted patch, 118: d8_typed_data_adapter-2002138-118.patch, failed testing.

fago’s picture

Thanks for the review and the fixes, changes look good to me.

9. This is supposed to be enabled I think (statically cache the typed data object) but apparently broke things. Let's see what's still breaking.

Yep, still breakage. We'll have to fix that I guess, because else it will needlessly create a lot of wrapper objects when creating fields I think.

xjm’s picture

Tagging as a priority for a pre-AMS beta sprint. Also removing redundant beta target tag; it's beta deadline. :)

chx’s picture

I would recommend the word proxy at least in the issue title . As in AccountProxy or http://en.wikipedia.org/wiki/Proxy_pattern (neither the word proxy nor adaper appears in the patch, though)

andyceo’s picture

+1 for Proxy word.

Berdir’s picture

Status: Needs work » Needs review
FileSize
56.33 KB
687 bytes

Re-roll, this fixes some tests, let's see how many.

Status: Needs review » Needs work

The last submitted patch, 124: d8_typed_data_adapter-2002138-124.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
56.98 KB
662 bytes

Wow, all of them, apparently :) That one is new and should be fixed now too.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -844,6 +844,7 @@ public function __clone() {
     if (!$this->translationInitialize) {
+      $this->typedData = NULL;

Good catch! berdir++

fago’s picture

I do no think the term proxy fits. Proxies implement the interfaces of the proxied objects such that the caller does not have to know - this is not the case here.

Attached patch fixes some small quirks and added unit test coverage for the new class / moved the old coverage from previous methods on the entity.

yched’s picture

Minor edits seen with @fago

yched’s picture

Seen with @fago as well : renamed TypedData/Entity to TypedData/EntityAdapter, and rename a couple variables accordingly.

The last submitted patch, 129: 2002138-typed_data_adapters-129.patch, failed testing.

yched’s picture

Added issue links to the dangling @todos ("support Config entities as TypedData") - note: those are about something that doesn't work in HEAD either. The patch here just adds explicit @todos about that.

The last submitted patch, 130: 2002138-typed_data_adapters-130.patch, failed testing.

yched’s picture

Fixes :
- TypedDate\Entity -> TypedDate\EntityAdapter in strings that PhpStorm didn't find
- TaxonomyFormatter using $items->getParent() instead of getEntity()

The last submitted patch, 132: 2002138-typed_data_adapters-131.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 134: 2002138-typed_data_adapters-134.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
71.7 KB
5.26 KB

Thanks. I've undone the two sneaked in changes. This contains improved comments to the validation issue, pointing to #2346373: Data reference validation constraints are applied wrong for the fix.

yched’s picture

Issue summary: View changes
fago’s picture

Issue summary: View changes
yched’s picture

Followup (not introduced by this patch, but needs this patch to get in first) : #2346433: $entity iterators omit computed fields by default

fago’s picture

Issue summary: View changes
fago’s picture

Added change record draft: https://www.drupal.org/node/2346441

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

webchick’s picture

Status: Reviewed & tested by the community » Fixed
[-300 lines of code]
+    return $this->getTypedData()->validate();

WELL! That certainly is MUCH nicer DX! :D

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/BundleConstraintValidator.php
@@ -16,11 +17,27 @@
+  public function validate($entity_adapter, Constraint $constraint) {
+    if (!isset($entity_adapter)) {
+      return;
+    }

I had a question here, which is why this fails silently when the function is supposed to validate something. Alex said that this makes it so it doesn't flag an error on a blank field.

Apart from that I can't see anything to complain about. Let's do it!

Committed and pushed to 8.x. Thanks!

  • webchick committed 6caeaf0 on 8.0.x
    Issue #2002138 by yched, Jose Reyero, xjm, andypost, fago, msonnabaum,...
fago’s picture

WOHOO! :-)

yched’s picture

Title: Use adapters for supporting typed data » Use an adapter for supporting typed data on ContentEntities

Renaming to reflect what the final approach actually was :-)

fago’s picture

Figured we did not re-add Traversable on the interface: #2346635: Content entities are iteratable but do not implement Traversable

yched’s picture

Also, discussed with @fago a bit and posted a couple adjustements as a followup issue in #2346643: Adjust Entity::getTypedData()

yched’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.