Right now, entity types are not exposed as proper typed data API types. There is just a type 'entity', which gets you a typed data object *wrapping* the actual entity object. But furthermore, $entity is an instance of the ComplexDataInterface of the typed data API and with #1778178: Convert comments to the new Entity Field API also an instanced of the ContextAwareInterface.

First, I'd consider this a DX is wtf, it's implementing the typed data just the half way, while all the derived fields are implementing it the full way, i.e. you can call $field->getType() or $field->getDefinition() what you cannot do for an entity.

Second, this special separation makes things more complex, e.g. it will cause problems when implementing #1696648: [META] Untie content entity validation from form validation based upon TypedData validation (as the entity itself does not implement the TypedDataInterface - ouch).

Third, the parent relationship of typed data APIs is broken for entities. E.g. when you have a referenced entity and you use the TypedData API to derive a property of the entity from the TypedData object (EntityWrapper), it's parent will be another object (the entity object) - ouch. Also, right now $typed_data->getParent() cannot assume the parent is typed data, it can just assume it's complex data or a list. We should fix that.

Once we've done, we can let the TypedData ListInterface and ComplexDataInterface extend the TypedDataInterface also - we do not need to support implementing the API half-way any more ;-)

I'm thinking about options to implement this best such that it results in no WTFs, will come back with my thoughts.

CommentFileSizeAuthor
#86 1868004-improve-typeddata-entityapi-86.patch77.37 KBdixon_
#86 1868004-improve-typeddata-entityapi-86-interdiff.txt1.46 KBdixon_
#81 d8_typed_entity-1868004-81.patch75.5 KBBerdir
#81 d8_typed_entity-1868004-81-interdiff.txt6.7 KBBerdir
#79 d8_typed_entity-1868004-79.patch82.2 KBBerdir
#79 d8_typed_entity-1868004-79-interdiff.txt778 bytesBerdir
#76 d8_typed_entity-1868004-76.patch81.44 KBBerdir
#76 d8_typed_entity-1868004-76-interdiff.txt630 bytesBerdir
#74 d8_typed_entity.interdiff.txt8.53 KBfago
#74 d8_typed_entity.patch82.83 KBfago
#68 d8_typed_entity.patch71.28 KBfago
#68 d8_typed_entity.interdiff.txt530 bytesfago
#63 d8_typed_entity.interdiff.txt1005 bytesfago
#63 d8_typed_entity.patch70.77 KBfago
#61 d8_typed_entity.patch70.67 KBfago
#61 d8_typed_entity.interdiff.txt12.48 KBfago
#58 typed-entity-ng-1868004-58.patch63.76 KBBerdir
#55 typed_data.patch.interdiff.txt8.07 KBfago
#55 d8_typed_entity.patch63.82 KBfago
#51 1868004-51.patch59.95 KBfubhy
#46 d8_expose_-1868004-46.patch68.31 KBdas-peter
#46 interdiff-1868004-44-46-do-not-test.diff815 bytesdas-peter
#44 d8_expose_-1868004-44.patch67.52 KBdas-peter
#44 interdiff-1868004-31-44-do-not-test.diff1.37 KBdas-peter
#42 d8_expose_-1868004-31-reroll.patch67.4 KBdas-peter
#38 d8_expose.patch67.37 KBfago
#38 d8_expose.interdiff.txt1.14 KBfago
#37 d8_expose.patch66.22 KBfago
#37 d8_expose.interdiff.txt2.62 KBfago
#34 d8_expose.patch66.1 KBfago
#34 d8_expose.interdiff.txt17.54 KBfago
#28 d8-expose-entity-typeddata-1868004-28.patch67.78 KBdas-peter
#28 interdiff-1868004-26-28-do-not-test.diff701 bytesdas-peter
#26 d8-expose-entity-typeddata-1868004-26.patch67.4 KBdas-peter
#26 interdiff-1868004-24-26-do-not-test.diff6.03 KBdas-peter
#24 d8-expose-entity-typeddata-1868004-24.patch62.38 KBdas-peter
#24 interdiff-1868004-22-24-do-not-test.diff5.68 KBdas-peter
#22 d8_expose.interdiff.txt30.91 KBfago
#22 d8_expose.patch56.7 KBfago
#18 entity-typed-data-1868004-18.patch41.78 KBBerdir
#18 entity-typed-data-1868004-18-interdiff.txt589 bytesBerdir
#16 entity-typed-data-1868004-16.patch41.05 KBBerdir
#12 d8_expose.patch39.85 KBfago
#10 d8_expose.patch.txt25.36 KBfago
#6 typed-data-deriver.patch4.22 KBEclipseGc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

ok, I think the following should work out:

  • Make EntityInterface extend TypedDataInterface also. An entity is always the root of the data tree.
  • Have registered typed data types like "node", "comment" and return that from the getType() method.
  • If you have an entity reference the typed data object wrapping the entity must actually be a different object, as it's not the root of tree but a property in another tree, so it will have a different 'type', e.g. "entity_reference".
  • To point to the referenced entity/tree I think we should introduce a new interface DataReferenceInterface or so. (Delegating interface methods would not result in cleanly decoupled trees. We cannot rely on the plain value being the object either as this does not make sense for other reference cases where the object do not implement the typed data API natively, e.g. language.)
  • For implementing the TypedDataInterface with entities we need to have a) getValue() setValue() functions that can set the entity object "value" and b) support (re-)constructing an object via typed_data()->createInstance($type, $value). For that to make sense I think $value should be the complete array of property values, including translations.
  • For creating entity objects via typed_data()->createInstance($type, $value) we would have to adapt the construct or make creating typed data objects more flexible, e.g. allow specifying a factory class.
klausi’s picture

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

fago’s picture

What it fixes is that the following works:

$entity instanceof TypedDataInterface;
$entity->getType() == 'node';
$entity->getDefinition(); $entity->validate(); // etc. works as for any other typed data.
$entity->user_id->getParent() instanceof TypedDataInterface;
$node = typed_data()->create('node', $values);

whereas the following would stop to work:

// The entity reference would not be complex data any more,
// so you cannot do:
$entity->get('user_id')->get('entity')->get('field');
// while the following continues to work (=delegation is removed)
$entity->get('user_id')->get('entity')->getValue()->get('field');

// And you cannot run in the issue that the getParent() is inconsistent:
$entity_reference = $entity->get('user_id')->get('entity'); 
$entity_reference->get('field')->getParent() !== $entity_reference // The parent is the entity object as of now

Instead, you could do something like the following (while using TypedData API only):

  $entity->get('user_id')->get('entity')->resolveReference()->get('field');
  // What in the case of an entity reference equals to
  $entity->user_id->entity->field;

We need resolveReference() as we cannot rely on the value being a TypedData object of the referenced value, e.g. for language references it wouldn't.

fago’s picture

Issue tags: +Entity Field API, +typed data

Adding tags.

fago’s picture

EclipseGc’s picture

FileSize
4.22 KB

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

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

Eclipse

fago’s picture

+++ b/core/lib/Drupal/Core/TypedData/TypedDataFactory.php
@@ -57,6 +65,11 @@ public function createInstance($plugin_id, array $configuration, $name = NULL, $
+    // If constraints exist in the definition, pass them to the configuration.
+    if (isset($type_definition['constraints']) && !empty($type_definition['constraints'])) {
+      $configuration['constraints'] += $type_definition['constraints'];

No need to do that, that's taken care of for you when you get the constraints. So you always can differentiate between type constraints and data definition constraints. Check the e-mail example - it works and has type constraints.

fago’s picture

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

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

ok, started working on this. Attached patch starts with implementing #1 and partially also #8. It's not finished yet but should show where it goes.

Todo:
* Finish abstract type implementation
* Finally make $entity implement the typed data interface
* Convert tests dealing with type => 'entity'
* Make sure all entity types and bundle combinations are registered
* Write tests for the new API

On what the value of an entity should be? I think it should be plain array representation of all the values of an entity (including translations?) That way you can create a valid entity object by using the typed data factory + passing in all necessary values and such this also serves as correct entity factory.

fago’s picture

Status: Active » Needs work
FileSize
25.36 KB
podarok’s picture

#10 looks good
any progress here?

fago’s picture

FileSize
39.85 KB

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

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

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

fago’s picture

Issue tags: +sprint

As we've got a patch ready in #1869562: Avoid instantiating EntityNG field value objects by default, it's time to move on with this next.

moshe weitzman’s picture

Priority: Normal » Major

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

fago’s picture

Once #1869562: Avoid instantiating EntityNG field value objects by default gets in entities already implement the typed data interfaces but only with stubs, so we've to re-roll this and implement those stubs + add some tests for using them.

Berdir’s picture

Status: Needs work » Needs review
FileSize
41.05 KB

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

Status: Needs review » Needs work

The last submitted patch, entity-typed-data-1868004-16.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
589 bytes
41.78 KB

This should at least fix most of those exceptions.

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

Status: Needs review » Needs work

The last submitted patch, entity-typed-data-1868004-18.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Entity.phpundefined
@@ -0,0 +1,158 @@
+ * example 'entity:user' or 'entity:node:article'. Entity types that make use of
+ * bundles cannot be instantiated without bundles either, i.e. entity:node is
+ * an abstract type as well as it requires a bundle for instantiation.

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

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReference.phpundefined
@@ -0,0 +1,113 @@
+ * Contains \Drupal\Core\Entity\Field\Type\EntityWrapper.

Still says EntityWrapper

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReference.phpundefined
@@ -0,0 +1,113 @@
+   * Implements \Drupal\Core\TypedData\DataReferenceInterface::getTargetDefinition().

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

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReference.phpundefined
@@ -0,0 +1,113 @@
+
+  /**
+   * Overrides \Drupal\Core\TypedData\TypedData::getValue().
+   */
+  public function getValue() {
+    // If we have a valid reference, return the entity object, otherwise NULL.
+    $target = $this->getTarget();
+    return !$target->isempty() ? $target : NULL;

+++ b/core/lib/Drupal/Core/TypedData/DataReferenceBase.phpundefined
@@ -0,0 +1,80 @@
+  /**
+   * Overrides TypedData::getValue().
+   */
+  public function getValue() {
+    return $this->getTarget()->getValue();

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

+++ b/core/lib/Drupal/Core/TypedData/DataReferenceBase.phpundefined
@@ -0,0 +1,80 @@
+    if (!isset($this->target)) {
+      $this->target = typed_data()->create($this->getTargetDefinition(), $this->getSource()->getValue());

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

+++ b/core/lib/Drupal/Core/TypedData/DataReferenceBase.phpundefined
@@ -0,0 +1,80 @@
+    return (string) $this->getTargetIdentifier();

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

+++ b/core/lib/Drupal/Core/TypedData/DataReferenceInterface.phpundefined
@@ -0,0 +1,38 @@
+   * @return mixed
+   *   The identifier of the referenced data.
+   */
+  public function getTargetIdentifier();

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

+++ b/core/lib/Drupal/Core/TypedData/Type/Language.phpundefined
@@ -17,13 +17,6 @@
 class Language extends TypedData {

+++ b/core/lib/Drupal/Core/TypedData/Type/LanguageReference.phpundefined
@@ -0,0 +1,41 @@
+class LanguageReference extends DataReferenceBase {

What's the difference between those two?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -469,21 +469,15 @@ public function testDataStructureInterfaces() {
+    // @todo: Make the Entity class implement the TypedDataInterface.
+    return;

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

fago’s picture

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

Ops, that should not have been part of the patch.

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

Yeah, I think that's reasonable.

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

I think we should fix the code to throw a typed data MissingContextException in that case.

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

I'm not aware of any usage of it yet, but I'd use it whenever a string representation of the object is needed, e.g. when referring to it in an exception message. Given that use-case, I think the suggestion is good.

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

It depends on the target whether it supports isEmpty, e.g. language does not as we are not implementing the ComplexDataInterface for language objects yet.

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

Replied there!

fago’s picture

Status: Needs work » Needs review
FileSize
56.7 KB
30.91 KB

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

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

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

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

Status: Needs review » Needs work

The last submitted patch, d8_expose.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
5.68 KB
62.38 KB

Tried to fix some of the failures.

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

function comment_prepare_author(Comment $comment) {
  // The account has been pre-loaded by CommentRenderController::buildContent().
  $account = $comment->uid->entity;
  if (empty($account->uid)) {
  ...

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

Status: Needs review » Needs work

The last submitted patch, d8-expose-entity-typeddata-1868004-24.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
6.03 KB
67.4 KB

Next bunch of fixes:

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

Status: Needs review » Needs work

The last submitted patch, d8-expose-entity-typeddata-1868004-26.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
701 bytes
67.78 KB

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

Status: Needs review » Needs work
Issue tags: -sprint, -Entity Field API, -typed data

The last submitted patch, d8-expose-entity-typeddata-1868004-28.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Entity Field API, +typed data
fago’s picture

Title: Expose entity types as proper TypedData API types » Improve the TypedData API usage of EntityNG

So we've got #2002134: Move TypedData metadata introspection from data objects to definition objects now, which solves our metadata problem for entities without bundles (nodes without a type). So we embrace that instead of the Abstract Type notion.

Together with #2002138: Use an adapter for supporting typed data on ContentEntities I guess we can re-title the issue to just improve TypedData API usage, i.e. clean entity references up and just skp the AbstractType stuff for now as #2002134: Move TypedData metadata introspection from data objects to definition objects will take care of that.

Berdir’s picture

Priority: Major » Normal
Status: Needs review » Needs work
Issue tags: -sprint +Typed sanity

I *think* this means that this no longer a major, validation blocking task and needs work, right? Removing sprint tag for now then, to keep the focus on the important issues, of which we have a few right now, including quite a few that are RTBC.

fago’s picture

Component: base system » typed data system
Assigned: Unassigned » fago
Issue tags: +sprint

I agree, this does not block validation any more. Re-assigning sprint though as I'm focussing on getting this done now.

fago’s picture

Assigned: fago » Unassigned
Status: Needs work » Needs review
FileSize
17.54 KB
66.1 KB

ok, worked over it and removed that (weird) abstract typed data stuff. Instead we'll do #2002134: Move TypedData metadata introspection from data objects to definition objects - see #31.

I introduced IdentifiableInterface with this patch and make EntityInterface and the typed Language object extend it. That allows for a proper DataReferenceBase implementation and is something that scotch (and Rules) would need as well - be able to get the id of some data.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -409,9 +409,10 @@ public function getNGEntity() {
+    if ($this->bundle != $this->entityType) {
+      return 'entity:' . $this->entityType . ':' . $this->bundle;
+    }
+    return 'entity:' . $this->entityType;

Is the ":" special, e.g. does the system actually understand that entity:node:article is a "subtype" of entity:node and is that documented somewhere?

Also, non-NG entities don't have bundle and to be sure, should we use the bundle() method here?

Nice to see the Abstract stuff gone because I didn't understand that from reading the patch initially (I do now, after our discussions).

fubhy’s picture

Can we assume that if a typed data object is "identifiable" it is also loadable and incorporate #1983100: Provide a LoadableInterface for Typed Data objects in the same interface?

fago’s picture

FileSize
2.62 KB
66.22 KB

Can we assume that if a typed data object is "identifiable" it is also loadable and incorporate #1983100: Provide a LoadableInterface for Typed Data objects in the same interface?

I'm not sure that everything we can identify is loadable for us also, so it might better be optional. Howsoever, it's not needed for this issue, while we needed the IDs. So we could deal with loadable in a follow-up I think.

Is the ":" special, e.g. does the system actually understand that entity:node:article is a "subtype" of entity:node and is that documented somewhere?

This syntax stems from the usual plugin derivatives syntax, so - but it's not documented anywhere and can be seen as an implementation internal I think. It's not documented or defined to be sub-types in this patch.

I agree we'll need the possibility for sub-type checks, but it should work inline with #1867880: Bring data type plugins closer to their PHP classes and work on PHP classes/interfaces as far as possible. We cannot check relationships between classes as those have to be swappable though. However, with #1867880: Bring data type plugins closer to their PHP classes we probably need to add an optional interface key to the type definition so you can e.g. specify NodeInterface and EntityInterface. Then, we can check relationships based upon the relationships of the interfaces. Consequently, I don't think we should depend on the type syntax for that.

Also, non-NG entities don't have bundle and to be sure, should we use the bundle() method here?

ok, updated that. Also re-rolled patch, fixed merge conflicts and adapted previous usages of typed_data() to use \Drupal::TypedData().

fago’s picture

FileSize
1.14 KB
67.37 KB

Added previously missing doc-updates.

moshe weitzman’s picture

#38: d8_expose.patch queued for re-testing.

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

The last submitted patch, d8_expose.patch, failed testing.

yched’s picture

#2012662: Constraints on 'target_id' / 'tid' properties break autocomplete if applied modified EntityWrapper, I guess that's why this doesn't apply anymore.

das-peter’s picture

Status: Needs work » Needs review
FileSize
67.4 KB

Here's a re-roll. Let's see what fails ;)

Status: Needs review » Needs work

The last submitted patch, d8_expose_-1868004-31-reroll.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
67.52 KB

Following tests pass on my machine now:

  • Drupal\comment\Tests\CommentInterfaceTest
  • ExpandDrupal\rdf\Tests\CommentAttributesTest
  • ExpandDrupal\system\Tests\Entity\EntityFieldTest

ExpandDrupal\menu\Tests\MenuTest always runs in a timeout, but I think that's my machine ;)

I'm not sure about this adjustment:

+      // Enusre we set no BCEntity.
+      if (isset($value)) {
+        $value = $value->getNGEntity();
+      }

While I'm sure we need to "sanitize" the entity instance, I'm not sure if Drupal\Core\Entity\Field\Type\EntityReference should sanitize to BCEntity (if applicable) or the NGEntity instead. Suggestions?

Status: Needs review » Needs work

The last submitted patch, d8_expose_-1868004-44.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
815 bytes
68.31 KB

Fixed caching issue in the menu test. No this should be green again *cross fingers*.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityDeriver.phpundefined
@@ -0,0 +1,67 @@
+    foreach (entity_get_info() as $entity_type => $info) {
+      $this->derivatives[$entity_type] = array(
+        'label' => $info['label'],
...
+      foreach (entity_get_bundles($entity_type) as $bundle => $bundle_info) {
+        $this->derivatives[$entity_type . ':' . $bundle] = array(
+          'label' => $bundle_info['label'],

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReference.phpundefined
@@ -0,0 +1,126 @@
+  public function getString() {
+    if ($entity = $this->getValue()) {
+      return $entity->label();

I just care that type & bundle are displayed in correct language. Is there a test for that? probably out of scope...

Berdir’s picture

Nice, I like where this is going!

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -424,9 +424,10 @@ public function getNGEntity() {
+    if ($this->bundle() != $this->entityType()) {
+      return 'entity:' . $this->entityType() . ':' . $this->bundle();
+    }
+    return 'entity:' . $this->entityType();

We still have the (in other places also existing problem) that bundle == entity_type does not mean that the entity isn't bundleable. We'd need the interface for that that we discussed in Portland. Not a blocker I think as we have no better way atm.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityDeriver.phpundefined
@@ -0,0 +1,67 @@
+   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinition().
...
+   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinitions().

@inheritdoc.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityDeriver.phpundefined
@@ -0,0 +1,67 @@
+      // Incorporate the bundles as entity:$entity_type:$bundle, if any.
+      foreach (entity_get_bundles($entity_type) as $bundle => $bundle_info) {

entity_get_bundles() ensures that there's always a bundle, by default one with the same name as the entity type. So to be consistent with the above check, this should only do this if count($bundles) == 1 and $bundle != $entity_type.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReference.phpundefined
@@ -0,0 +1,126 @@
+use InvalidArgumentException;

Probably just moved but let's inline this while we do so.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReference.phpundefined
@@ -0,0 +1,126 @@
+    if (isset($this->definition['constraints']['Bundle']) && is_string($this->definition['constraints']['Bundle'])) {
+      $definition['type'] .= ':' . $this->definition['constraints']['Bundle'];

Same here, I don't think this is consistent with the first check? Always adding the bundle would be another way to make it consistent and right now probably reflect reality?

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReference.phpundefined
@@ -0,0 +1,126 @@
+   * {@inheritdoc}
+   *
+   * @return \Drupal\Core\Entity\EntityInterface|null|false

The current coding standards define that @inheritdoc must only be used if it's the only thing that's defined, there's an issue to change it, but it might make sense to leave it out to avoid any discussions. Especially since it's missing the description.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReference.phpundefined
@@ -0,0 +1,126 @@
+   * {@inheritdoc}
+   *
+   * Both the entity ID and the entity object may be passed as value.

Same. Maybe move this to an inline comment for now? Also wondering if I didn't already mention this above.

+++ b/core/lib/Drupal/Core/TypedData/DataDefinitionException.phpundefined
@@ -0,0 +1,17 @@
+use Exception;

Inline as \Exception.

+++ b/core/lib/Drupal/Core/TypedData/DataReferenceInterface.phpundefined
@@ -0,0 +1,39 @@
+   * @return int|string|null

I'm thinking if a target identifier could be anything else... but probably not :)

+++ b/core/lib/Drupal/Core/TypedData/Type/Language.phpundefined
@@ -17,15 +17,8 @@
-class Language extends TypedData {
+class Language extends TypedData implements IdentifiableInterface {

Still don't completely grok what this class exactly represents :) What happens if language becomes a config entity, which there's still hope? Can we then kill the special casing of languages and just treat them as entity/entity reference?

+++ b/core/modules/comment/comment.moduleundefined
@@ -1516,7 +1516,7 @@ function comment_preprocess_block(&$variables) {
-  if (!$account) {
+  if (empty($account->uid->value)) {

Interesting, why is this change necessary here? My UserInterface patch adds a isAnonymous() (or was it isAuthenticated and then ! of that). Would make more sense here I guess.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Type/TaxonomyTermReferenceItem.phpundefined
@@ -32,9 +32,12 @@ public function getPropertyDefinitions() {
       static::$propertyDefinitions['tid'] = array(
         'type' => 'integer',
         'label' => t('Referenced taxonomy term id.'),
+        'constraints' => array(
+          'Range' => array('min' => 0),
+        ),
       );
       static::$propertyDefinitions['entity'] = array(
-        'type' => 'entity',
+        'type' => 'entity_reference',
         'constraints' => array(

The patch that makes this extend from entity reference landed, so this will need a re-roll and this is no longer necessary.

Berdir’s picture

Status: Needs review » Needs work
fubhy’s picture

Assigned: Unassigned » fubhy
fubhy’s picture

Assigned: fubhy » Unassigned
Status: Needs work » Needs review
FileSize
59.95 KB

Tried to re-roll this. Not sure if it's 100% correct ;).

Status: Needs review » Needs work

The last submitted patch, 1868004-51.patch, failed testing.

Berdir’s picture

Priority: Normal » Major

Promoting to major as discussed with @xjm.

@fago: Can you look at this?

fago’s picture

Assigned: Unassigned » fago

yep, looking at this one.

Language as config entities got in, but I don't think this influences language_reference - as we have $node->langcode independently of language module.

fago’s picture

Assigned: fago » Unassigned
Status: Needs work » Needs review
FileSize
63.82 KB
8.07 KB

Re-rolled and fixed for latest changes.

I also had to fix another hunk in the entity-relationship-query code. I think that could need some overhaul to check for types by interfaces, but that's clearly out of scope for this issue.

fago’s picture

Note: Pushed code to branch typed-entity-ng of the entity sandbox.

Status: Needs review » Needs work

The last submitted patch, d8_typed_entity.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
63.76 KB

Re-roll, the annotations stuff went in. Let's see how well this works.

Status: Needs review » Needs work

The last submitted patch, typed-entity-ng-1868004-58.patch, failed testing.

Berdir’s picture

Issue tags: +Needs tests

I just noticed that what I did can not work, the annotation on entity won't get picked up. So we have to add that in using another way, as we don't want to move Entity into the datatype plugin directory I assume?

It also means that we are obviously missing tests on the fact that this actually works and is exposed correctly.

fago’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
12.48 KB
70.67 KB

While we discussed adding the StaticDiscoveryDecorator in IRC this poses the problem of having typed-data know/depend on the entity system - which is the wrong way round. So I looked up what we are already doing for field types: We just have an unused class for registering the plugin. While that's not very nice it technically solves everything just fine, so why not.

Thus, I implemented the same for exposing entity types and aligned both implementations, i.e. moved the field item deriver to the usual Deriver sub-path as well. Also, as those classes are never used and instantiated I documented it that way and made them abstract.

I've also added test-coverage for checking that data types get correctly exposed. References are already test as part of EntityFieldTest.

Status: Needs review » Needs work

The last submitted patch, d8_typed_entity.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
70.77 KB
1005 bytes

This should be green.

Status: Needs review » Needs work

The last submitted patch, d8_typed_entity.patch, failed testing.

Berdir’s picture

I had the same failures in #1939994: Complete conversion of nodes to the new Entity Field API, check if it can create the translation correctly in the step above the first fails. In my case the problem was that there was the language() cache and setting a new language didn't update it correctly. That is AFAIK the only place in core where we change the language of a node (maybe of an entity in general).

fago’s picture

That's interesting as those tests were green for me with #63. Let's double-check.

fago’s picture

Status: Needs work » Needs review

#63: d8_typed_entity.patch queued for re-testing.

fago’s picture

oh dear, there are two TranslationTest classes.. - I checked the wrong one.

So yes, it was the language issue again. I figured the problem is the BCdecorator not notifying of changes.

Status: Needs review » Needs work
Issue tags: -sprint, -Entity Field API, -typed data, -Typed sanity

The last submitted patch, d8_typed_entity.patch, failed testing.

fago’s picture

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

#68: d8_typed_entity.patch queued for re-testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReference.phpundefined
@@ -0,0 +1,133 @@
+      if ($this->id === FALSE) {
+        $this->target = FALSE;
+      }
+      else {
+        // If we have a valid reference, return the entity object which is typed
+        // data itself.
+        $this->target = entity_load($this->definition['constraints']['EntityType'], $this->id);

Does https://drupal.org/node/2032185 affect this?

Is it possible that this logic also addresses the problems I had in File and Entity, where I had to add special handling for uid 0?

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReferenceItem.phpundefined
@@ -40,11 +43,11 @@ class EntityReferenceItem extends FieldItemBase {
+    // Definitions vary by settings, so key them accordingly.
+    $key = implode(',', $this->definition['settings']);

This looks scary, should this use a hash or so instead?

+++ b/core/lib/Drupal/Core/TypedData/DataReferenceInterface.phpundefined
@@ -0,0 +1,39 @@
+   * @return \Drupal\Core\TypedData\TypedDataInterface|null|false
+   *   The referenced typed data object, or NULL if the reference is unset, or
+   *   FALSE if the reference is invalid; i.e., the referenced is stale.

Same as above, entity_load() no longer returns FALSE. Does this need to be updated (aka are there other instances that could?)

+++ b/core/modules/comment/comment.moduleundefined
@@ -1482,7 +1482,7 @@ function comment_preprocess_block(&$variables) {
   $account = $comment->uid->entity;
-  if (!$account) {
+  if (empty($account->uid->value)) {

I guess this answers my question that this in fact returns an entity for id 0. Great.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -527,22 +512,39 @@ public function getContainedStrings(TypedDataInterface $wrapper, $depth, array &
+    entity_test_create_bundle('bundle');
+    $types = \Drupal::typedData()->getDefinitions();
+    $this->assertTrue($types['entity:entity_test:bundle']['class'], 'Entity bundle data type registed.');

Trying to understand where the cached definitions are cleared, where does that happen?

Still not completely sure that the type with type and bundle is correct, when you e.g. have an

Berdir’s picture

#68: d8_typed_entity.patch queued for re-testing.

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

The last submitted patch, d8_typed_entity.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
82.83 KB
8.53 KB

Trying to understand where the cached definitions are cleared, where does that happen?

I think it's the field system that clears caches when bundles change, ie. field_entity_bundle_create(). It should probably happen at the entity system level, but that's another issue.

This looks scary, should this use a hash or so instead?

It shuold be fine as the only two supported settings are strings, however I improved it to explicitly add in the individual settings. While being there, I noted that inheriting classes did not key correctly (as they inherit they support the same setting), so I applied the same key-ing method to all those.

Does https://drupal.org/node/2032185 affect this?

I'd say so - thanks for the pointer. As there is no FALSE return statement anymore I don't think we have to differentiate between those two states here any more and can simplify the code flow + simplify things for the caller. -> Updated it to just support NULL.

Status: Needs review » Needs work

The last submitted patch, d8_typed_entity.patch, failed testing.

Berdir’s picture

Interesting, don't quite understand why this was not necessary before, I guess the language got loaded from the langcode field even if it was empty or something like that?

Fix makes sense to me, addTranslation() checks if the language is locked and if so, does not allow to add a translation.

plach’s picture

Status: Needs work » Needs review

Yep, #76 looks definitely needed.

Status: Needs review » Needs work

The last submitted patch, d8_typed_entity-1868004-76.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
778 bytes
82.2 KB

The things we have to do to get this to green.

The number of bugs this uncovers should be enough to commit it :)

You can't translate entities with und as language, I don't really know *how* it worked, but it relied on the broken behavior in the default language definition that I fixed in the previous comment...

Berdir’s picture

This is an important improvement that implements the empty methods that we added earlier on to the Entity class.

The API changes are not that big, it exposes entity types property as typed data, which is more or less required for #1906810: Require type hints for automatic entity upcasting (which is critical), the handling of entity/data references makes a lot more sense than the current EntityWrapper implementation and it exposes and fixes a number of bugs that are in HEAD right now, which were made explicit here.

It has been worked and iterated on for quite a long time and I think is ready. I only did some re-rolls and fixed a few tests.

Berdir’s picture

@effulgentsia noticed that this patch adds the EntityTranslation class again that we removed in #1810370: Entity Translation API improvements due to a merge conflict, as it was never in the interdiffs.

Removing this file again.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

This patch looks awesome. My only concern is the following instanceof check on a non-interface:

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
@@ -140,13 +141,12 @@ function addField($field, $type, $langcode) {
+            if (!$column && isset($propertyDefinitions[$relationship_specifier]) && $entity->{$field['field_name']}->get('entity') instanceof EntityReference) {

But #2028097: Map data types to interfaces to make typed data discoverable is already open for addressing that across all of TypedData. Therefore, RTBC.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

fago’s picture

Changes look good to me, thanks for catching those issues.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I know these are very minor but... if it had just been the spelling mistakes I would have fixed during commit...

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.phpundefined
@@ -0,0 +1,28 @@
+ * Note that the class only register the plugin, and is actually never used.

registers

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReference.phpundefined
@@ -0,0 +1,128 @@
+class EntityReference extends DataReferenceBase {
...
+  public function setValue($value, $notify = TRUE) {
+    unset($this->target);
+    unset($this->id);
+
+    // Both the entity ID and the entity object may be passed as value.
+    if (!isset($value) || $value instanceof EntityInterface) {
+      // Enusre we set no BCEntity.
+      if (isset($value)) {
+        $value = $value->getNGEntity();
+      }
+      $this->target = $value;
+    }

This logic looks interesting... how is it possible for $value to not be set? I.e. why would we pass null into $value? Talking to @berdir on irc it appears that this would to unset the reference. I think that this could do with a better comment to explain that setValue can also result in unsetting a value... plus there is a spelling mistake Enusre... also I think that that comment should be more in the positive like Ensure we have the NGEntity

dixon_’s picture

Fixed remarks from Alex's review.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, improvements look good.

fago’s picture

It's not something special that you unset it by passing NULL as all the typed data objects work that way, but howsoever having it mentioned in a comment makes sense.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5fc86b0 and pushed to 8.x. Thanks!

Berdir’s picture

Issue tags: -sprint

Untagging.

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