Problem

The entity reference field can not reference configuration entities because it can only store integer IDs and it explicitly prohibits users from configuring the field to reference config entities.

Proposal

Change the target_id property to be either int or string, depending on the configured entity type reference.

Original report

Problem

  • UUIDs have been added to all entities, but reference fields (such as taxonomy term fields) still reference by serial ID only.

Goal

  • Add a UUID column to all reference fields.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Field system
moshe weitzman’s picture

Add a UUID column to all reference fields.

Are you suggesting to keep existing ref to integer and add uuid as additional information?

sun’s picture

Yeah, an additional uuid column on reference fields (field values). I thought this was originally suggested in #1637370-14: Add UUID support to core entity types, but can't find it in there, so it probably was in another issue.

fago’s picture

Do we really need this? We could map ids when import/exporting entities anyway as deploy does it in d7 now?

moshe weitzman’s picture

Status: Active » Closed (works as designed)

I guess that we don't strictly need this. Please reopen if someone disagrees.

cbeier’s picture

Status: Closed (works as designed) » Active

I was a little confused that the UUIDs are only used in the "main entities" and not even in the references themselves. In the current situation is the entity in their unit uniquely identifiable (even across different systems). But this advantage is lost, when the item is related to other tables/entities (or it must be mapped separately between the uuid from the "main entity" and their internal id).

The biggest advantage of the UUID integration (in my opinion) would be, if the main identifier of all items/entities/nodes is a UUID. And these UUID is also used for the internal references. The result would fewer intermediate steps (e.g. for ID mapping) for use cases like data synchronisation.

mitchell’s picture

Component: entity system » entity_reference.module
larowlan’s picture

Assigned: Unassigned » larowlan

going to give this a crack early next week

webchick’s picture

Assigned: larowlan » Unassigned
Priority: Normal » Major

Given the impact that xjm found at #1735118-217: Convert Field API to CMI, this qualifies as at least major.

webchick’s picture

Assigned: Unassigned » larowlan

Oopsie.

yched’s picture

So, the problem found with #1735118: Convert Field API to CMI is that the field instance definition, deployed in a CMI file, can contain field values in the 'default_value' entry. For entity_reference or taxo fields, that currently means a local, numeric id, and the reference breaks when the config is deployed.

There's a more general issue that, even if we had only UUIDs in our CMI files, this question of "how do we deploy cross-references between Config and Content, i.e things that are not deployed (if deployed at all) through the same mechanisms" is AFAIK quite blurry at the moment. What do we do when the referenced UUID is absent on the target environment ? What's the nicest way to break, and what tools do we need on the CMI import flow to implement that nicest way ?
Not sure if there's an existing issue about that.

Then there's the issue of making sure our CMI files contain UUIDs rather than local ids, and that's a question for each individual ConfigEntity to solve. "Highly pluggable" subsystems, like Field API or Views, have one additional tricky bit, though, since they are not responsible for a large part of the config they store.

So, for the specific case of field default values :

Those are currently stored in the exact same "canonical" format that's used for field values loaded and saved in an entity, according to the "field schema" of the field type - the format that's produced by the widget in submitted form values :
array('column_1' => 'some value', 'column_2' => 3, ...)

So if a "reference" field type like taxonomy, entity_reference (image and file handle default values their own custom way, even though they store a numeric file id too) store local numeric ids instead of UUIDs, in the current mechanism, that's what will get stored and deployed in the field instance CMI file.

So this either means:
- that reference field types need to change how they store their reference ids,
- or, that we need an additional step for field types to reshape their "default values" when they get saved to or read from the instance definition.

Xano’s picture

There's a more general issue that, even if we had only UUIDs in our CMI files, this question of "how do we deploy cross-references between Config and Content, i.e things that are not deployed (if deployed at all) through the same mechanisms" is AFAIK quite blurry at the moment. What do we do when the referenced UUID is absent on the target environment ? What's the nicest way to break, and what tools do we need on the CMI import flow to implement that nicest way ?
Not sure if there's an existing issue about that.

What happens now if we delete an entity that is referenced from an entity reference field? Can we mimic that behavior for when entities are unavailable during install?

fago’s picture

- or, that we need an additional step for field types to reshape their "default values" when they get saved to or read from the instance definition.

Yep - that would be the solution which would be inline with the currend handling of UUIDs aka map to UUIDs when needed only.

larowlan’s picture

Assigned: larowlan » Unassigned

This is seriously involved, we don't really support entity_load_multiple_by_properties by uuid so we need to go right down to the storage controller.
With the time given, I reckon this is beyond me.
Will keep tinkering but anyone else who wants it jump in.

Xano’s picture

At this moment, is there anything that blocks entity references from using UUIDs?

larowlan’s picture

Xano, configured instance of blocks that represent custom_block derivatives reference the content entity by UUID. Ie Block instance (configured/placed block - a config entity) references the custom block content entity by UUID. This was the main reason I took on that module. Serial IDS in config entities is bad news for deployability.

Xano’s picture

We probably misunderstood one another. What I'd like to know if there are any technical barriers left that prevent us from referencing entities by UUID rather than serial ID, so I can go ahead and roll a patch.

larowlan’s picture

I started down that path but there is no real ability at the storage level to entity_load_multiple_by_uuid (that I could see), so I guess that's the first barrier.
And db perf issues for using a string key instead of a serial.

Xano’s picture

Performance can be addressed by not using UUIDs, but to change the schema depending on what type of entity is referenced: integer serial ID for content entities and varchar name for config entities. This is, however, less uniform, but will not be a regression from the current situation.

larowlan’s picture

Yeah I think we're only (mainly) talking about content entities here, especially for default values in fields where we get serial ids bleeding into config entities, limiting deployability.

Xano’s picture

I'm not sure what you mean. Is there an approach you are in favor of?

larowlan’s picture

Nevermind me :)

Xano’s picture

Can someone who knows more than me about database performance and field API recommend either 1) using UUIDs or 2) using IDs for content entities and names for config entities?

Xano’s picture

Assigned: Unassigned » Xano
Issue tags: -staging, -UUID

Working on a patch that uses the dynamic schema approach.

Xano’s picture

Title: Add UUIDs to reference fields » Config entity reference
Assigned: Xano » Unassigned
Status: Active » Needs review
Issue tags: +Needs tests, +staging, +UUID
FileSize
4.45 KB

Those tags were removed accidentally.

This patch is backwards compatible and simply changes the column type for config entities to a varchar. I'm having PHP 5.4 issues locally (#2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0). I will fix those before writing tests.

Xano’s picture

#25: drupal_1757452_00.patch queued for re-testing.

Xano’s picture

tstoeckler’s picture

+++ b/core/modules/entity_reference/entity_reference.install
@@ -14,9 +14,6 @@ function entity_reference_field_schema($field) {
-        'type' => 'int',
-        'unsigned' => TRUE,
-        'not null' => TRUE,

@@ -29,19 +26,33 @@ function entity_reference_field_schema($field) {
+  if (isset($definition['config_prefix'])) {
+    $schema['columns']['target_id'] += array(
+      'type' => 'varchar',
+      'length' => 255,
+    );
+  }
+  else {
+    $schema['columns']['target_id'] += array(
+      'type' => 'int',
+      'unsigned' => TRUE,
+      'not null' => TRUE,
+    );

Since we're already changing this, this should *always* be a varchar. ConfigEntities are not the exception by having string IDs, ContentEntities are in fact the exception in that they have serial/int IDs, while that is being enforced nowhere in the system. The current code is quite simply a bug.

If we're worried about performance, we can document that id() must return integers in ContentEntityInterface and then check for that here, but not the other way around.

+++ b/core/modules/entity_reference/entity_reference.install
@@ -29,19 +26,33 @@ function entity_reference_field_schema($field) {
+    if (is_subclass_of($entity_manager->getControllerClass($field['settings']['target_type'], 'storage'), 'Drupal\Core\Entity\DatabaseStorageController')) {
...
-    );
+      $schema['foreign keys'][$base_table] = array(
+        'table' => $base_table,
+        'columns' => array('target_id' => $id_column),

Hmm.. This should get a @todo at least to clean this up. Even using the same storage controller, different entity types could be saved in different databases, so adding the foreign keys might not always make sense. On the other hand, I could write a BetterDatabaseStorageController that does not subclass the original one and still save my entity type in the same database. Neither of those are very likely, and it's not terrible to add the foreign keys in any case, I guess, but a @todo would be cool.

Did you actually try this, does this actually work? The amount of code is ridiculously small. I also like adding the options() method to EntityManager

Xano’s picture

Assigned: Unassigned » Xano

Did you actually try this, does this actually work? The amount of code is ridiculously small. I also like adding the options() method to EntityManager

Not yet. I haven't been able to address the PHP 5.4 issues I was experiencing, but I just started to write tests anyway a few minutes ago. I'll just let the testbot run those for me.

Hmm.. This should get a @todo at least to clean this up. Even using the same storage controller, different entity types could be saved in different databases, so adding the foreign keys might not always make sense. On the other hand, I could write a BetterDatabaseStorageController that does not subclass the original one and still save my entity type in the same database. Neither of those are very likely, and it's not terrible to add the foreign keys in any case, I guess, but a @todo would be cool.

What's the use of adding foreign keys if we can't even be sure that they point to an existing table? I'd hate to have code in there that only serves one single use case out of many.

tstoeckler’s picture

Yeah, it's one use-case out of many, but it's the 90% one :-)
I discussed with @Xano that we would suggest dropping the foreign keys entirely, and letting contrib solve that. We weren't aware of anything actually using those anyway, so...

yched’s picture

Panels / ctools D7 relies on the foreign keys present in the field schema to build possible "relationships"

Xano’s picture

Status: Needs work » Needs review
FileSize
12.48 KB

This patch removes foreign key support, and simply uses a VARCHAR(255) for storing referenced entities' IDs.

There are some test failures, but I've been staring at them for too long to see what goes wrong.

Assigned: Xano » Unassigned
Status: Needs review » Needs work

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

Xano’s picture

FileSize
12.47 KB

Re-rolled.

Status: Needs review » Needs work

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

Xano’s picture

I cannot reproduce the syntax error locally. Hopefully someone else can take a look at it and the test failures.

amateescu’s picture

I also maintain D7 custom code that relies on the foreign keys definition to build 'relationships', so I would be pretty much against removing this feature unless we have an alternative.

Looking at the patch, the functionality cannot possibly work right now because \Drupal\Core\Entity\Plugin\DataType\EntityReferenceItem::getPropertyDefinitions() is not updated, so we clearly need tests..

Xano’s picture

Assigned: Unassigned » Xano

I also maintain D7 custom code that relies on the foreign keys definition to build 'relationships', so I would be pretty much against removing this feature unless we have an alternative.

The problem is–and this existed long before this issue–that we can never be a 100% sure whether entities are stored in the database or not. As @tstoeckler pointed out on IRC, a database storage controller can be written that does not extend DatabaseStorageController, and there will be no foreign keys. True, we're talking about the 20% here, but that doesn't mean we should maintain wonky edge-case code for the 80%. This will bite us later on.

Looking at the patch, the functionality cannot possibly work right now because \Drupal\Core\Entity\Plugin\DataType\EntityReferenceItem::getPropertyDefinitions() is not updated, so we clearly need tests..

Well spotted. Thanks for pointing this out. I'll reroll the patch.

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
13.35 KB

Fixed the whitespace errors and converted the field item target_id property to a string.

The reason why EntityReferenceItemTest fails is because the target_id is not saved, or at least it is emptied upon saving the entity the field is attached to. When inspecting the property definitions on the entity, the reference target ID is correctly set up as a string field, so I don't know why storage fails.

amateescu’s picture

The problem is–and this existed long before this issue–that we can never be a 100% sure whether entities are stored in the database or not. As @tstoeckler pointed out on IRC, a database storage controller can be written that does not extend DatabaseStorageController, and there will be no foreign keys. True, we're talking about the 20% here, but that doesn't mean we should maintain wonky edge-case code for the 80%. This will bite us later on.

This wasn't really a problem in D7 because the usage of alternative storage engines was very very low, but I agree that it can be a potential issue for D8. However, since the foreign keys information is not 'officially' used anywhere, I feel that the responsabilty of how and when to use it is downstream, in contrib and custom code that actually interact with it.

So.. can we please not affect the 80% by being overprotective? :)

Status: Needs review » Needs work

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

yched’s picture

Well, if "reference"-like fields (file, image, taxo) somehow settled on EntityRefItem (or subclasses), I guess there could be a smarter way to deduce relationships than looking for foreign keys in a db schema, which is kind of backwards ?

amateescu’s picture

Yes, I guess we can look into their settings definition..

Xano’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Entity system, -staging, -UUID, -Field system

#39: drupal_1757452_39.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +Entity system, +staging, +UUID, +Field system

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

Xano’s picture

I don't know why it says that. My localhost runs the file just fine.

tim.plunkett’s picture

$entity->get($field_name)[0]
That's not valid in PHP 5.3

Xano’s picture

Status: Needs work » Needs review
FileSize
13.35 KB

Ah, thanks. Must have been a copypaste error and I'm running 5.4 locally.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Entity system, -staging, -UUID, -Field system

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

Xano’s picture

Status: Needs work » Needs review

#48: drupal_1757452_48.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +Entity system, +staging, +UUID, +Field system

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

klonos’s picture

...the issue summary needs to be updated (this issue originally started as an effort to add UUIDs to reference fields) and the issue title is too vague to understand what we're doing here.

Xano’s picture

FileSize
14.36 KB

The config entity reference test still fails. Haven't been able to figure out why.

amateescu’s picture

Title: Config entity reference » Support config entities in entity reference fields
Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs issue summary update, -Entity system, -staging, -UUID, -Field system +sprint, +Entity Field API
FileSize
23.3 KB
24.94 KB

The config entity reference test still fails. Haven't been able to figure out why.

Because referencing a config entity wasn't actually working :P

As discussed in Prague, we need to vary the target_id property (int or string) by referenced entity type. I think this should do it.

amateescu’s picture

Issue summary: View changes

.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -599,4 +599,19 @@ public function getAllBundleInfo() {
+  function options() {

This should be public. Also the function name is kind of odd. Don't we have some generic entity form controller where this could also live?

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -599,4 +599,19 @@ public function getAllBundleInfo() {
+  function options() {

Can't we call this getEntityTypeLabels() or something? options() is not what I expected. Also, public needed. Also bonus points for using array_map() instead

Otherwise, this looks great (way easier to review if you diff locally with -w)

Status: Needs review » Needs work

The last submitted patch, 1757452-54.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
26.93 KB

Renamed that method to getEntityTypeLabels(), but I think an array_map() would just make it less readable..

Xano’s picture

I'm not sure the method should be a getter, as we're not actually getting a value from the object (something that can be set as well), but we retrieve a dynamically assembled list.

amateescu’s picture

Just entityTypeLabels() then?

Xano’s picture

I think that would indeed be a more accurate description of what the method does.

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReferenceItem.php
@@ -44,20 +44,35 @@ class EntityReferenceItem extends FieldItemBase {
+      if ($target_type_storage instanceof DatabaseStorageController) {

We should probably check for ConfigEntityInterface and ContentEntityInterface instead. I believe this was also discussed and agreed upon earlier this year, as we require entities to implement either of those interfaces, but storage is pluggable and does not say anything about entity IDs' data types.

amateescu’s picture

FileSize
6.63 KB
26.46 KB

Yep, I think that makes sense, I guess I was just lazy in the previous patch :/

Status: Needs review » Needs work

The last submitted patch, 1757452-62.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
26.36 KB

Rerolled.

dawehner’s picture

Tested it by choosing views as config entity. Sadly views can't build a view of views, because then you could reference a view using a view of views.

Okay I went with the default selecting method.
While creating a new node I should reference a view, though I tried "content", "Content" but none of those worked even
there is a Content view serving "admin/content".

Note: The autocompletion also failed.

Additional this patch should maybe inject the entity manager.

amateescu’s picture

That's an interesting bug you found there, but it's related to config entity query, not ER :) This code should return the 'Content' view.. and it doesn't work. I'll open a separate issue for it.

$query = \Drupal::entityQuery('view');
$query->condition('label', 'C', 'CONTAINS');
$result = $query->execute();
debug($result);
Additional this patch should maybe inject the entity manager.

The problem with this is that the selection plugin manager uses the "non-conventional" ReflectionFactory instead of ContainerFactory. I'll open another issue to decouple selection plugins from field definitions :)

Berdir’s picture

  1. +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
    @@ -36,22 +36,28 @@ class ConfigEntityReferenceItemBase extends EntityReferenceItem implements Confi
    +      // Only add the revision ID property if content entity target types.
    +      $target_type_info = \Drupal::entityManager()->getDefinition($target_type);
    +      if (is_subclass_of($target_type_info['class'], '\Drupal\Core\Entity\ContentEntityInterface')) {
    +        static::$propertyDefinitions[$key]['revision_id'] = array(
    

    Would it make more sense to check for a revision table/key in the definition?

  2. +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
    @@ -88,7 +94,7 @@ public static function schema(FieldInterface $field) {
         $target_id = $this->get('target_id')->getValue();
    -    if (!empty($target_id) && is_numeric($target_id)) {
    +    if (!empty($target_id) && (is_numeric($target_id) || is_string($target_id))) {
           return FALSE;
         }
    

    What else could it be other than int or string? :) I guess the numeric check was when we still had autocreate in target_id ?

    We could just write this like this:

    if (!empty($this->target_id)) {
      return FALSE;
    }
    

    ?

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -599,4 +599,19 @@ public function getAllBundleInfo() {
+  public function entityTypeLabels() {

Shouldn't that be getEntityTypeLabels()? The other methods on the manager use the get.. pattern.

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReferenceItem.php
@@ -44,19 +43,35 @@ class EntityReferenceItem extends FieldItemBase {
+      if (is_subclass_of($target_type_info['class'], '\Drupal\Core\Entity\ContentEntityInterface')) {
+        static::$propertyDefinitions[$key]['target_id'] = array(
+          'type' => 'integer',

There shouldn't be a requirement for content entities to have numeric IDs?

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

Xano’s picture

Shouldn't that be getEntityTypeLabels()? The other methods on the manager use the get.. pattern.

That would make it look like a real getter, for which a setter is available as well. Instead, it computes the labels, so we opted not to use "get" as part of the method name.

fago’s picture

I'm not sure the method should be a getter, as we're not actually getting a value from the object (something that can be set as well), but we retrieve a dynamically assembled list.

hm, generally methods should do something. not sure about assembleEntityTypeLabels() - get would work for me, but I do not want to start bikesheding on that ;-)

fago’s picture

That would make it look like a real getter, for which a setter is available as well. Instead, it computes the labels, so we opted not to use "get" as part of the method name.

A getter does not imply there has to be a setter.

amateescu’s picture

FileSize
3.22 KB
26.39 KB

After a quick bikeshed here and in IRC we decided to go with getEntityTypeLabels(). Also agreed to open a separate issue for the false requirement of content entities to have numeric IDs.

amateescu’s picture

amateescu’s picture

Also opened #2107309: Case (in)sensitivity for config entity query for the config entity query problem.

Berdir’s picture

Status: Needs review » Needs work

There's a problem with the validation of autocomplete inputs.

That defaults to $label ($id) in the output, but the validate then won't accept that because AutocompleteWidget::elementValidate() has a hardcoded assumption about id being integer:

      // Take "label (entity id)', match the id from parenthesis.
      if (preg_match("/.+\((\d+)\)/", $element['#value'], $matches)) {
        $value = $matches[1];
      }

Failing that, it tries to do an EFQ for e.g. "Administrator (administrator) on the label and that doesn't match either.

chx’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
28.88 KB
amateescu’s picture

FileSize
6.51 KB
32.36 KB

Thanks @chx, I added a @todo so we can get away with assuming that content entity IDs are numeric for now. Also, fixed up $value not being assigned and added an integration test for the whole thing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, manually tested this and it's working fine. I've created a field that references a field definition and it works fine (Apart from not displaying it because it doesn't have an access controller or admin permission, but that's not the fault of this issue). A role reference was displayed just fine.

This has been reviewed by me and various others. Let's get this in, then we can do cool stuff with e.g. $node->type, $term->vid and so on.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
32.18 KB

Here you go :)

amateescu’s picture

FileSize
32.11 KB

And another one.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

jibran’s picture

Title: Support config entities in entity reference fields » Change notice: Support config entities in entity reference fields
Status: Fixed » Active
Issue tags: +Needs change record

This deserves change notice IMO.

Berdir’s picture

Change compared to what? A HEAD to HEAD chang notice is imho not necessary and 7.x users already expect that they can reference all entity types. In a way, this just implemented a feature that users already expect to work :)

And the only change notice about adding entityreference is https://drupal.org/node/1796546.

amateescu’s picture

Title: Change notice: Support config entities in entity reference fields » Support config entities in entity reference fields
Status: Active » Fixed
Issue tags: -Needs change record, -sprint

I feel the same way. I'd gladly write a notice if there was anything else to write than the title :)

webchick’s picture

webchick’s picture

Issue summary: View changes

Updated issue summary with the current solution.

Status: Fixed » Closed (fixed)

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