Make the comment entity_id be a reference field

1. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -223,10 +223,12 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setRequired(TRUE);
   

   The field is still required? Or are all ER fields required by default.

2. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -312,6 +314,19 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['entity_id']->setSettings(array(
   +      'target_type' => $target_type,
   +      'default_value' => 0,
   +    ));
   +    return $fields;
   +  }
   

   Awesome!

If the answer to (1) is 'all ER fields are required', then this is RTBC.
Great work!

Yeah, I think it should still be required, not sure why @blueminds removed it :)

+ list($target_type, ) = explode('__', $bundle);

My gut feeling tells me there should be a function somewhere to extract the target type from a bundle. No?

Yes, but that's not a problem that we are introducing here, this is something for #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form.

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -312,6 +315,19 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    list($target_type, ) = explode('__', $bundle);

this needs the third argument (2) as per CommentBundleEnhancer::enhance in case user names field field__snazzy via the UI

From an ER point of view, this looks very nice. I'm just wondering if/how we can find other places which might try to access the value of the field with ->value instead of ->target_id.

Also, would this be a good time to update the field name? get('entity_id')->target_id looks... unfortunate :)

Yeah, that's a common problem that we have elsewhere, like $node->uid, not sure about changing that here, would mean a considerable bigger change.

I'm pretty sure that there is no direct access to entity_id left, should all use the methods.

Also, welcome back :)

Thanks :)

Ok then, let's move forward with what we can for now.

Overall great!!!

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -223,10 +223,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+        'default_value' => 0,

@@ -312,6 +315,19 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      'default_value' => 0,

any reason for this default value?

1. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -223,10 +223,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setSettings(array(
   +        'default_value' => 0,
   +      ));
   
   @@ -312,6 +315,19 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['entity_id']->setSettings(array(
   +      'target_type' => $target_type,
   +      'default_value' => 0,
   +    ));
   

   Not marking needs work for that, but in case this gets a re-roll this should be moved to setSetting(). setSettings() should never be used as it removes the default settings by the field item class. See #2236903: setSettings() on field definitions can unintentionally clear default values.

2. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -312,6 +315,19 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    list($target_type, ) = explode('__', $bundle, 2);
   

   Super minor, but omitting the ", " would work as well, i.e. : list($target_type) = ...

#18 needs work 'entity_id' could be string!

No, right now the schema is hardcoded to an integer, so it can't be a string. That's not related to this issue. Not sure why we do need a default_value though?

Awesome, thanks for the quick turnaround!

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -216,10 +216,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setSettings(array(
+        'default_value' => 0,
+      ));

There's still a default value and a setSettings() here. Can you quickly fix that as well? That would be great, thanks.

Updated patch with change said in #23. Please review.

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -220,9 +220,7 @@
+      ->setSetting('target_type', 'comment');

I don't think 'comment' makes sense here. I.e. comments on comments. I think that line can just be removed.

Correct, there is only a target type once we know the bundle.

@tstockler: Is schema() of entity references already capable to handle that? This might break the schema generation?

Heh, I thought about that as well. No, it currently can't. should be an easy fix, though. It only gets the entity type as it uses the 'integer' schema type for content entities.

So should we fix that here?

@Berdir, @tstoeckler:

btw, I have made the change that you said in #25 by looking at the existing code just above than it in same file(pasting below).

    $fields['pid'] = FieldDefinition::create('entity_reference')
      ->setLabel(t('Parent ID'))
      ->setDescription(t('The parent comment ID if this is a reply to a comment.'))
      ->setSetting('target_type', 'comment');

Yes, but that has nothing to do with this field, so it shouldn't be added for this.

We decided that we won't touch schema() here, so just needs an update to remove that line. All patches were green, so that's good.

Ok. Removed that line now.

Yup, looks great. Thanks!

+1 to RTBC

Hm. I might be wrong, but this smells like something that could do with some profiling.

No need, as long as you don't actually access the referenced entity, there is only a very minor overhead and this is in line with every other similar field (like the node author and node type), we've just been holding out on this because we weren't able to do the by-bundle definition earlier.

And in return, this gives us proper validation of this field and better integration with REST or similar things that rely on UUID's for referenced entities.

Ok, right on. Thanks for the explanation. The patch surprisingly involves far fewer changes than I would've guessed. :)

Committed and pushed to 8.x. Thanks!

Filed follow-up #2245001: Remove unneeded CommentManagerInterface::getParentEntityUri()