Problem/Motivation

Drupal\Core\Entity\Plugin\DataType\EntityReferenceItem::getPropertyDefinitions() hardcodes the assumption that content entities have numeric IDs:

      static::$propertyDefinitions[$key]['target_id'] = array(
        // @todo: Lookup the entity type's ID data type and use it here.
        'type' => 'integer',
        'label' => t('Entity ID'),
        'constraints' => array(
          'Range' => array('min' => 0),
        ),
      );

Proposed resolution

Suggested by @fago in #1757452-68: Support config entities in entity reference fields:

I'd suggest taking the "id" entity key and looking up the definition of the first field item property (or go with the main property once introduced by #2015687: Convert field type to FieldType plugin for taxonomy module ).

Remaining tasks

Write a patch.

User interface changes

None.

API changes

None.

#1757452: Support config entities in entity reference fields

CommentFileSizeAuthor
#45 interdiff.txt1.44 KBamateescu
#45 2107249-45.patch10.04 KBamateescu
#43 interdiff-proper-data-type.txt1.94 KBamateescu
#43 interdiff.txt1.46 KBamateescu
#43 2107249-43.patch9.97 KBamateescu
#37 entityreference_non_numeric_ids_2107249-37-tests-only.patch2.71 KBjelle_s
#37 entityreference_non_numeric_ids_2107249-37.patch10.01 KBjelle_s
#37 interdiff-31-37.txt969 bytesjelle_s
#32 entityreference_non_numeric_ids_2107249-32-tests-only.patch2.48 KBjelle_s
#2 drupal_2107249_2.patch5.52 KBxano
#6 drupal_2107249_6.patch3.5 KBxano
#11 drupal_2107249-11.patch3.17 KBjibran
#14 2107249-14-entity-reference-target-id.patch8.29 KBtstoeckler
#17 2107249-14-entity-reference-target-id.patch1.53 KBtstoeckler
#19 2107249-19-entity-reference-target-id.patch1.26 KBtstoeckler
#20 2107249-20-entity-reference-target-id.patch4.62 KBtstoeckler
#22 2107249-20-22-interdiff.txt660 byteststoeckler
#22 2107249-22-entity-reference-target-id.patch4.82 KBtstoeckler
#24 test.patch5 KBtstoeckler
#26 entityreference_non_numeric_ids_2107249-27.patch4.83 KBjelle_s
#28 entityreference_non_numeric_ids_2107249-28.patch10.16 KBjelle_s
#31 entityreference_non_numeric_ids_2107249-31-tests-only.patch2.34 KBjelle_s
#31 interdiff-28-31.txt1.68 KBjelle_s
#31 entityreference_non_numeric_ids_2107249-31.patch9.78 KBjelle_s

Comments

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Assigned: Unassigned » xano
xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Assigned: xano » Unassigned
Status: Active » Needs review
StatusFileSize
new drupal_2107249_2.patch5.52 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

2: drupal_2107249_2.patch queued for re-testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new drupal_2107249_6.patch3.5 KB

Status: Needs work » Needs review

jibran queued 6: drupal_2107249_6.patch for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

See #2345813: Primitive TypedData fields are not cast properly for yet another reason we need this.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new drupal_2107249-11.patch3.17 KB

reroll

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Priority: Normal » Major

This is major. This makes it impossible to use content entities with non-integer IDs as targets of entity references. That's a pretty standard use-case.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2107249-14-entity-reference-target-id.patch8.29 KB

Let's see if this applies, will roll the patch into it if it does.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2107249-14-entity-reference-target-id.patch1.53 KB

Oops, less custom crap this time. Let's see if this works.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2107249-19-entity-reference-target-id.patch1.26 KB

Ok, let's try this again.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new 2107249-20-entity-reference-target-id.patch4.62 KB

OK, that was not enough. The schema needs to be updated as well, of course.

Also brings back the test from above. The code changed so much in the meantime that I recreated it, so no interdiff.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2107249-20-22-interdiff.txt660 bytes
new 2107249-22-entity-reference-target-id.patch4.82 KB

Bah. Should be green.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new test.patch5 KB

Testbot, what's wrong?! :'-(

Status: Needs review » Needs work

The last submitted patch, 24: test.patch, failed testing.

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
StatusFileSize
new entityreference_non_numeric_ids_2107249-27.patch4.83 KB

Reroll of #24.

Status: Needs review » Needs work

The last submitted patch, 26: entityreference_non_numeric_ids_2107249-27.patch, failed testing.

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new entityreference_non_numeric_ids_2107249-28.patch10.16 KB

After a lot of digging through code, this patch should be green...

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
amateescu’s picture

amateescu commented
Component: entity system » entity_reference.module
Category: Task » Bug report
Status: Needs review » Needs work

The patch looks pretty good, nice work!

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -121,13 +120,25 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
-      $columns = array(
-        'target_id' => array(
-          'description' => 'The ID of the target entity.',
-          'type' => 'int',
-          'unsigned' => TRUE,
-        ),
-      );
+      $properties = static::propertyDefinitions($field_definition)['target_id'];
+      if ($properties->getDataType() === 'integer') {
+        $columns = array(
+          'target_id' => array(
+            'description' => 'The ID of the target entity.',
+            'type' => 'int',
+            'unsigned' => TRUE,
+          ),
+        );
+      }
+      else {
+        $columns = array(
+          'target_id' => array(
+            'description' => 'The ID of the target entity.',
+            'type' => 'varchar',
+            'length' => 255,
+          ),
+        );
+      }

    I think it would be cleaner if we move the $properties = ... call above the first if() and then add the $properties->getDataType() === 'integer' condition to it, and have a single 'else' case for a varchar target_id. 

  2. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceItemTest.php
index fb34794..3dedb82 100644
--- a/core/modules/views/tests/src/Unit/EntityViewsDataTest.php

    Are the changes to this phpunit test actually testing something related to this patch?

Also, can you please upload a test-only patch with just the test changes to show that we currently cannot reference content entities with string ids? Because that's actually a bug, IMO :)

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
StatusFileSize
new entityreference_non_numeric_ids_2107249-31.patch9.78 KB
new interdiff-28-31.txt1.68 KB
new entityreference_non_numeric_ids_2107249-31-tests-only.patch2.34 KB

The changes to the EntityViewsDataTest were just necessary to make it pass again, because of these additions:

+      $id_definition = \Drupal::entityManager()->getBaseFieldDefinitions($settings['target_type'])[$target_type_info->getKey('id')];
+      $target_id_definition = DataDefinition::create($id_definition->getType())

there were a couple of things that needed to be mocked. Nothing new is being tested there.

In EntityReferenceItemTest the additions do test the new behavior. I uploaded a tests only patch as well.

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new entityreference_non_numeric_ids_2107249-32-tests-only.patch2.48 KB

Sorry, tests only patch was wrong. Correct one this time.

The last submitted patch, 31: entityreference_non_numeric_ids_2107249-31-tests-only.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs review » Needs work 
+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceItemTest.php
@@ -153,6 +169,15 @@ public function testContentEntityReferenceItem() {
+    $entity = EntityTest::create();
+    $entity->field_test_entity_test_string_id->value = $this->entityStringId->id();
+    $entity->save();

Since this doesn't fail, maybe we need to add some more assertions after the save() call? Something like asserting that 'target_id' has the correct assigned value.

Also, you should use ->target_id instead of ->value, because the entity reference field type does not have a 'value' property.

attiks’s picture

attiks commented

#34 You're right, the save never fails, it just writes 0 into the database. Which sounds like a - separate - bug as well.

We have to load the entity again and check if our test values is actually set.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Then the test is doing something wrong. When I tried this on an actual site (in the belief that we had already fixed this at some point) I got SQL exceptions because it was trying to save 'some_id' into a SQL int column.

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-31-37.txt969 bytes
new entityreference_non_numeric_ids_2107249-37.patch10.01 KB
new entityreference_non_numeric_ids_2107249-37-tests-only.patch2.71 KB

New patch with the added assertions.

Status: Needs review » Needs work

The last submitted patch, 37: entityreference_non_numeric_ids_2107249-37-tests-only.patch, failed testing.

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review

So the tests only patch failed as it should and the complete patch passed the tests.

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

It would be nice to also have a confirmation from @berdir or @yched, but the latest patch looks great to me. It's nice to see that typed data / entity field api got to a point where this change is so small and easy to grasp :)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Invalid datetime format: 1366 Incorrect integer value: 'weWP47OX'

Yay! :-)

I guess I can't RTBC this, but it looks great now.

Awesome work @Jelle_S, thanks a ton!!!

Edit: x-post, yay @amateescu

yched’s picture

yched commented

I‘m away from my coding env, so not too easy to look at the patch in context :-)

Something looks weird though : unless I'm misreading, we‘re taking the *field* type and settings of the id field we're targeting, and use them as a *TD property* type and settings ? That's wrong, those live in different spaces ? It might happen to work with the two specific cases we're interested about (int and string), but generally speaking that code is mixing apples and oranges ?

Rather than aiming for generic support whatever the type of 'id', I think we'd be better off explicitly coding for the two cases we actually support - especially since the schema() implementation can only support those two cases anyway ?

amateescu’s picture

amateescu commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2107249-43.patch9.97 KB
new interdiff.txt1.46 KB
new interdiff-proper-data-type.txt1.94 KB

Heh, of course I missed something :/

After writing the code for supporting any data type for 'id' (in the second interdiff attached), I think I agree that just hard-coding the integer data type for integer field types like we do in schema() is a bit more straightforward.

Status: Needs review » Needs work

The last submitted patch, 43: 2107249-43.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2107249-45.patch10.04 KB
new interdiff.txt1.44 KB

#sigh

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Wow, @yched that is totally correct. Sorry for missing that. Everytime I think I understand Field API and Typed Data integration ... ... it's a trap! :-/

Thanks @amateescu, yes the explicit check makes a lot of sense. I hope we can do better in D9, but for now the explicitness makes it very clear what is going instead of the mainPropertyName() stuff in the other interdiff.

Now that the propertyDefinitions() stuff was rewritten my only contribution to this patch is the two lines in schema() (the tests were written by @Xano and fixed by @Jelle_S) and I think this has gone sufficiently gone back and forth for me to RTBC now.

Thanks again @yched for setting me straight!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed cb451af and pushed to 8.0.x. Thanks!

  • alexpott committed cb451af on 8.0.x 
    Issue #2107249 by Jelle_S, tstoeckler, amateescu, Xano, jibran, yched:...

Status: Fixed » Closed (fixed)

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