Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amitaibu’s picture

Title: #1839064: Implement the new entity field API for the Entity-Reference field type » Implement the new entity field API for the Entity-Reference field type
amitaibu’s picture

Assigned: Unassigned » amitaibu

Work is done in branch - yched\er-fieldItem-1847588

fago’s picture

Issue tags: +Entity Field API

tagging

amitaibu’s picture

I'm seeing an error whenever I entity_create('entity_test', array())->save() caus
ing my tests to fail

Anyone familiar with this?

fago’s picture

What's the actual error? I could imagine some fields are required on the db level?

amitaibu’s picture

> What's the actual error?

Unsupported operand types

I was able to reproduce the error it several times on a clean installation (however one time on another computer it didn't work), via hook_init:

module_enable(array('entity_test'));
$entity = entity_create('entity_test', array('name' => 'foo'));
$entity->save();
amitaibu’s picture

The error in #6 was due to EntityTestStorageController::baseFieldDefinitions() having a wrong name (entityreference instead of entity_reference).
Now that we moved the ER plugin under entity-reference module should we make entity-test dependent on ER?

amitaibu’s picture

@fago,

I see that in DatabaseStorageControllerNG::mapToStorageRecord() we have $record->$name = $entity->$name->value;, however ER has the "target_id" and "entity" properties, it doesn't have "value", so it will fail when entity_test is enabled.
Question are:
1) How to overcome that ->value hardcoding?
2) what to do with "user_id" in entity-test -- should entity_test module rely on ER?

fago’s picture

1) How to overcome that ->value hardcoding?

Having $entity->user_id->target_id in the API is weird in general though. If we use target_id instead of value for references, I think we should rename user_id to "user", e.g. have $entity->user->target_id. That would be more inline with the naming of fields anyway.

To fix, I see multiple ways:
a) Just customize it in the entity storage controller and map it "manually" there.
b) While not being so nice any more, we could fix it by using the value of the first not computed property by default?
c) A cleaner solution would be to support a proper naming pattern in the schema columns, e.g. "field__column" or "field:column" (if allowed in the schema?).

I must say I'd like c) most, e.g. rename the user_id field to user and the user_id column to user:target_id or user__target_id ?

2) what to do with "user_id" in entity-test -- should entity_test module rely on ER?

I think so, yes.

amitaibu’s picture

chx from IRC - the short version, took me some time to understand what he meant ;) :

yes remove from schema, turn it into configurable field

fago’s picture

yes remove from schema, turn it into configurable field

This test cases is actually about testing non-configurable fields, so converting it isn't a good idea.

amitaibu’s picture

Status: Active » Needs review
FileSize
17.97 KB
157.51 KB
  • Moved EntityReferenceItem under Entity-reference module
  • Added test, based on taxonomy's test
  • Remove "value" key hardcoding from DatabaseStorageControllerNG::mapToStorageRecord()

Patch is against 8.x, and interdiff against the main ER patch.

tim.plunkett’s picture

Sorry if it's too early for this sort of review...

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+          'entity type' => $target_type,
...
+        'settings' => array('id source' => 'target_id'),

Probably out scope, but due to annotations we've moved to more keys with underscores instead of spaces

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -298,7 +298,8 @@ protected function invokeHook($hook, EntityInterface $entity) {
-      $record->$name = $entity->$name->value;
+      $key = key($entity->$name->offsetGet(0)->getProperties());
+      $record->$name = $entity->$name->{$key};

Ewwww :(

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+ * Definition of Drupal\entity_reference\Type\EntityReferenceItem.

Here and elsewhere, "Contains \Drupal\..."

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+use InvalidArgumentException;

Don't use top level classes, just \ them

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+ * Defines the 'entityre_ference_field' entity field item.

typo entityre_ference

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+  static $propertyDefinitions;

Needs visibility

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+   * Implements ComplexDataInterface::getPropertyDefinitions().

Missing the fully qualified namespace, here and every method

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+    if (!isset(self::$propertyDefinitions[$target_type])) {

Here and everywhere, I believe it should static::, not 100% sure

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+        // The entity object is computed out of the entity id.

s/id/ID

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.phpundefined
@@ -0,0 +1,103 @@
+ * Tests the new entity API for the entity-reference field type.

I've seen entity_reference or entity reference, but not entity-reference

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.phpundefined
@@ -0,0 +1,103 @@
+    $node1 = $this->drupalCreateNode();
...
+    $node2 = $this->drupalCreateNode();

$node_1, $node_2

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined
@@ -168,8 +168,8 @@ public function baseFieldDefinitions() {
-      'type' => 'entityreference_field',
...
+      'type' => 'entity_reference_field',

Sure, I guess :)

Status: Needs review » Needs work

The last submitted patch, er-fieldItem-1847588-12.patch, failed testing.

amitaibu’s picture

Thanks Tim!

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -298,7 +298,8 @@ protected function invokeHook($hook, EntityInterface $entity) {
-      $record->$name = $entity->$name->value;
+      $key = key($entity->$name->offsetGet(0)->getProperties());
+      $record->$name = $entity->$name->{$key};

Ewwww :(

I know :) Didn't know what where was the best place to to put it. Any suggestions, while I fix the other tests?

amitaibu’s picture

Status: Needs work » Needs review
FileSize
13.54 KB
22.57 KB
162.12 KB
+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+  static $propertyDefinitions;

Needs visibility

Can you explain what you mean?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/Type/EntityReferenceItem.phpundefined
@@ -0,0 +1,82 @@
+    if (!isset(self::$propertyDefinitions[$target_type])) {

Here and everywhere, I believe it should static::, not 100% sure

I've just moved the original class under Entity reference module - currently it's using self.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.phpundefined
@@ -0,0 +1,103 @@
+    $node1 = $this->drupalCreateNode();
...
+    $node2 = $this->drupalCreateNode();

$node_1, $node_2

Tests are copied and adapted from TaxonomyTermReferenceItemTest which uses the $term1. I actually think $node1 is nicer than $node_1, but if you feel strongly about it, I can change it :)

  • Patch against 8.x
  • Interdiff is against the original ER patch
  • Interdiff against pervious patch
tim.plunkett’s picture

The moved code stuff can stay as is then, that just wasn't clear it was moved.

By visibility I mean public/protected.

Status: Needs review » Needs work

The last submitted patch, er-fieldItem-1847588-16.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
162.05 KB

Oh, removed wrong file just before created the patch :/

Status: Needs review » Needs work

The last submitted patch, er-fieldItem-1847588-19.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
15.17 KB
153.91 KB

Changing target_id => value seems to be way more complicated than I expected. To keep this patch small, aviod scopre creep, and arguably "value" is better than "target_id" -- I'll keep it as value.

amitaibu’s picture

Missed another test -- entity-test/add doesn't work now. Damn, it's an harder than expected kind of patch ;)

amitaibu’s picture

Ok, enough for today. Let's see in the morning if there are still failing tests.

Status: Needs review » Needs work

The last submitted patch, er-fieldItem-1847588-23.patch, failed testing.

amitaibu’s picture

FileSize
70.53 KB

Nope, seems we need to change to target_id after all.

Current problem is that after this code:

$entities = array_values(entity_load_multiple_by_properties('entity_test', array('name' => 'foo')));
$entities[0]->user_id->target_id;

I get wsod, because the loaded values are incorrect
image.png

amitaibu’s picture

Status: Needs work » Needs review
FileSize
27.57 KB
166.31 KB

Found the issue from #26, but fixed as hack DatabaseStorageControllerNG::mapFromStorageRecords()

foreach ($record as $name => $value) {
  // @todo: How to generalize it?
  $key = $name == 'user_id' ? 'target_id' : 'value';
  $entity->{$name}[LANGUAGE_DEFAULT][0][$key] = $value;
}
$records[$id] = $entity;

Not sure what's the proper fix.

Uploading to test with testbot (takes to long on my local)

Status: Needs review » Needs work

The last submitted patch, er-fieldItem-1847588-27.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -154,7 +154,9 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
       foreach ($record as $name => $value) {
-        $entity->{$name}[LANGUAGE_DEFAULT][0]['value'] = $value;
+        // @todo: How to generalize it?
+        $key = $name == 'user_id' ? 'target_id' : 'value';
+        $entity->{$name}[LANGUAGE_DEFAULT][0][$key] = $value;

@@ -298,7 +300,8 @@ protected function invokeHook($hook, EntityInterface $entity) {
     foreach ($this->entityInfo['schema_fields_sql']['base_table'] as $name) {
-      $record->$name = $entity->$name->value;
+      $key = key($entity->$name->offsetGet(0)->getProperties());
+      $record->$name = $entity->$name->{$key};

@fago and I already had a similar discussion in regarts to date fields in the comments NG issue.

There, only the entity -> storage conversion was a problem, not both as in here.

Somehow, we need to make it more flexible, but I'm not quite sure yet.

Right now, this essentially means that contrib can't define data types that don't have a value property. Which is bad.

Maybe we can somehow use the property data.

Note that this will conflict with #1833334: EntityNG: integrate a dynamic property data table handling and #1869250: Various EntityNG and TypedData API improvements has changes related to these parts as well I think.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
index a60e65e..0000000
--- a/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.php

--- a/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.php
+++ /dev/nullundefined

+++ /dev/nullundefined
+++ /dev/nullundefined
@@ -1,82 +0,0 @@

@@ -1,82 +0,0 @@
-<?php
-
-/**
- * @file
- * Definition of Drupal\Core\Entity\Field\Type\EntityReferenceItem.
- */
-
-namespace Drupal\Core\Entity\Field\Type;

I disagree that we should be doing this.

The entity reference data type will also be used for e.g. the uid property on nodes/comments, which would make entity_reference a dependency for node.module, do we really want that?

I don't have a problem with keeping this in Drupal\Core and entityreference.module would just be responsible for exposing it as a configurable field in the UI.

fago’s picture

// @todo: How to generalize it?
$key = $name == 'user_id' ? 'target_id' : 'value';
$entity->{$name}[LANGUAGE_DEFAULT][0][$key] = $value;

For setting the value we can omit the $key, as the field-items implement their own logic to set the items from a primitive. This would be already fixed with #1869250: Various EntityNG and TypedData API improvements.

+ $key = key($entity->$name->offsetGet(0)->getProperties());

Well, this could be
+ $key = key($entity->{$name}[0]->getProperties());

Not nice, but better as the previous hard-coding to 'value'.

@berdir: yep we should come-up with a general mapping the storage controller uses, as written above:

c) A cleaner solution would be to support a proper naming pattern in the schema columns, e.g. "field__column" or "field:column" (if allowed in the schema?).

amitaibu’s picture

Status: Needs work » Needs review
FileSize
33.51 KB
171.3 KB

Kept the original field API in place as per #29
Fixed tests. I hope I got them all... All those fails are a result of the hardcoded "value" key.

fago’s picture

Status: Needs review » Needs work

Good work!

I don't have a problem with keeping this in Drupal\Core and entityreference.module would just be responsible for exposing it as a configurable field in the UI.

True. Having dependencies on various modules just for various always-needed storage-fields seems weird, so I like this approach.

I just found some minor issues:

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.php
@@ -2,19 +2,19 @@
- * Defines the 'entityreference_field' entity field item.
+ * Defines the 'entityre_reference_field' entity field item.

entity_reference_field ?

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.php
@@ -28,36 +28,35 @@ class EntityReferenceItem extends FieldItemBase {
-    // Definitions vary by entity type, so key them by entity type.
-    $entity_type = $this->definition['settings']['entity type'];
+    $target_type = $this->definition['settings']['target_type'];

Can we keep that comment?

+++ b/core/modules/entity_reference/entity_reference.module
@@ -29,11 +29,29 @@ function entity_reference_field_info() {
+      $property_info['definitions'][$field_name]['settings']['target_type'] = $field['settings']['target_type'];

This could need a comment why we need to implement the hook on our own.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUUIDTest.php
@@ -81,7 +81,9 @@ function testCRUD() {
+          // @todo: Use the property data to extract the key.
+          $key = $property == 'user_id' ? 'target_id' : 'value';
+          $this->assertEqual($entity_duplicate->{$property}->{$key}, $entity->{$property}->{$key});

Well we could just use
$duplicate->{$field}->getValue() == $entity->{$field}->getValue() ?

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -209,9 +209,10 @@ function translation_entity_prepare_translation(EntityInterface $entity, Languag
-      // @todo The value part should not be needed. Remove it as soon as things
-      //   do not break.
-      $target_translation->$property_name->value = $source_translation->$property_name->value;
+      // @todo The "key" part should not be needed. Remove it as soon as things
+      // do not break.
+      $key = key($entity->{$property_name}[0]->getProperties());
+      $target_translation->$property_name->{$key} = $source_translation->$property_name->{$key};

While not directly related, in that case
$target_translation->$field = $source_translation->$field; should do it.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
171.16 KB
3.48 KB
33.36 KB

Addressed fago's comments.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.phpundefined
@@ -11,7 +11,7 @@
+ * Defines the 'entityre_reference' entity field item.

Still says entityre_reference?

Status: Needs review » Needs work

The last submitted patch, er-fieldItem-1847588-33.patch, failed testing.

amitaibu’s picture

Oh, missed that there's a typo there :)
I'll re-roll.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
171.14 KB
33.36 KB

Status: Needs review » Needs work

The last submitted patch, er-fieldItem-1847588-37.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
171.33 KB
33.55 KB

While not directly related, in that case
$target_translation->$field = $source_translation->$field; should do it.

This actually caused the test in #37 to fail, so re-added it.

amitaibu’s picture

Status: Needs review » Closed (duplicate)

Ok, test are back green, and I've addressed fago's comments, so merging it to ER's main branch.