Convert entity bundle base fields to entity reference

klausi opened a new pull request for this issue.

Looks good to me.

This currently just works because EntityReferenceItem still has the magic methods overridden and redirects ->value to ->target_id.

I'd rather not rely on that and prefer to update existing code references, at least comment should have tons of ->field_id->value calls all over the place.

It would be easier if #2016679: Expand Entity Type interfaces to provide methods, protect the properties would have been committed already for comments... :(

Discussed a bit with @Berdir on IRC and he convinced me that the subtle API change introduced here is not really ok. So we can either wait for #2028025: Expand CommentInterface to provide methods and do the switch properly or just leave Comment out of this patch.

Yep, I was in a bit in a hurry, let me explain my thoughts a bit for my previous post.

Thanks to the following and similar methods in EntityReferenceItem, you might think that this is no API change when it in fact is:

  /**
   * {@inheritdoc}
   */
  public function __get($name) {
    $name = ($name == 'value') ? 'target_id' : $name;
    return parent::__get($name);
  }

That's why no changes are necessary and it still works.

However, this code was added before we introduced getMainPropertyName and generic code (e.g. the database storage controller) needed a way to get the value of a field in a generic way. We have that now, and I'd prefer to remove those hacks, in order to have less unexpected behavior (If you'd call getPropertyDefinitions(), it will tell you that there is ->target_id and ->entity, but not ->value, so why does it work?) and it might give you the (wrong) idea that there is always a ->value, which there is not.

So, what about this:

1. I'd like to avoid a huge conflict with #2028025: Expand CommentInterface to provide methods and that will also minimize the necessary changes (another reason why those methods are useful), so let's postpone changing comment on that issue. If that issue gets in first, we'll update it here, otherwise, we update the code there once this is in.

2. Open a follow-up issue to try and remove those magic magic methods from EntityReferenceItem, make sure that calling code uses target_id.

3. Look at the remaining two cases and check if there are cases that we need to update. I think I already changed most $term->vid to $term->bundle(), I found a single use with a short search), I can't find a single case for custom_block, I think that already uses ->bundle() everywhere. Which shows that the API change impact exists but is minimal as code should either use the generic bundle() method when working with a generic entity or the entity type specific method if available, like $node->getType() and they have nothing to be worried about.

I agree with this plan :)

klausi pushed some commits to the pull request.

Thanks, yes, looks like there's fewer things to update than I expected. Back to RTBC then, @amateescu agreed in IRC.

This looks like a fairly minor API change; are there any existing change records we'd update?

Edit: I guess probably the one for #2112239: Convert base field and property definitions for a start.

Ah, based on @berdir's comment on that issue, no change record action needed until #2002134: Move TypedData metadata introspection from data objects to definition objects lands?

The API change is that the changed fields are now an entity reference, which means their actual properties that you should be using are ->target_id and ->entity (yes, $term->vid->entity == $vocabulary). This mostly affects code that acts in a generic way with entities, for example the serialization stuff changes, a term exposed over REST now has different properties and there's an additional explicit reference from the term to the vocabulary.

Due to the the mapping of ->value to ->target_id, this is not yet visible, but the referenced issue will make it so. (That is #2188075: Remove magic getter of EntityReferenceItem, you referenced the wrong one)

2181593_0.patch no longer applies.

error: patch failed: core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php:208
error: core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php: patch does not apply

Here we go, Re-rolling the patch from #8.

I think comment needs more work because #2149859: Convert the 'field_id' base field on comment entities to an entity reference field was closed as duplicate of the issue

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -431,8 +431,8 @@ public static function baseFieldDefinitions($entity_type) {
-    // @todo Convert to aa entity_reference field in
-    // https://drupal.org/node/2149859.
+    // @todo Convert to an entity_reference field in
+    // https://drupal.org/node/2028025
     $fields['field_id'] = FieldDefinition::create('string')

The related issue does not supposed to change this!
The actual issue was closed as duplicate of that one.

Here is a patch with suggestions from #17.
@andypost thank you :)

Fix broken test, now vocabulary should exist to create term!
+1 to RTBC

Based on @Berdir's comment in #13 above, this is good to go change record-wise.

Committed 0b2c622 and pushed to 8.x. Thanks!

Reopened #2149859: Convert the 'field_id' base field on comment entities to an entity reference field because #2002158: Convert form validation of comments to entity validation reverts following hunk

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -431,11 +431,10 @@ public static function baseFieldDefinitions($entity_type) {
-    // @todo Convert to aa entity_reference field in
-    // https://drupal.org/node/2149859.
-    $fields['field_id'] = FieldDefinition::create('string')
+    $fields['field_id'] = FieldDefinition::create('entity_reference')
       ->setLabel(t('Field ID'))
-      ->setDescription(t('The comment field id.'));
+      ->setDescription(t('The comment field id.'))
+      ->setSetting('target_type', 'field_entity');

Comment 'field_id' is 'node__comment' but actual field->id() is 'node.comment'