Allow comments to be attached to entities using a string primary key

95% sure that this is not a core problem.

Comment requires that the host entity has an integer ID. To figure that out, it loads the base field definitions and uses those to verify that.

You either have a non-integer field definition for your ID or you didn't specify base field definitions.

See comment_form_field_ui_field_storage_add_form_alter() and _comment_entity_uses_integer_id().

Because comments have a reference to the commented entity with the entity_type/entity_id base fields, and that's string/integer. It's not possible to vary that by the reference and changing it to string would be a performance regression for all the ID entity types.

See #2205215: {comment} and {comment_entity_statistics} only support integer entity ids.

I'm sorry, but for 8.x, that's pretty much by design and won't change.

FYI Comment module is not the only thing with this limitation, much of core (and contrib) is built around integer-based primary keys.

Changing this to a feature request for 8.1.

@attiks Because 'comments' are not the ER but property (enable,disable,hidden), see the original conversion issue #731724: Convert comment settings into a field to make them work with CMI and non-node entities
The ER you keep in mind is from comment entity to parent entity does not fit in core now.

The only attempt to fix that was #1995944: Remove entity_id and entity_type from the comment table and replace with relationship tables but storing "entity_type+entity_id" to fetch comments bring a lot of overhead, I can't find the exact comment where it was measured but we spend a lot of time on that before.

MySQL will forgive you for putting ints into a varchar column, Postgres won't be so understanding.

If we do this, we accept that ID lookups aren't going to be as efficient as they'd be on an integer.

Million dollar question is how much slower

With #2107249: Don't assume that content entities have numeric IDs in EntityReferenceItem we could convert the entity_id base field to field instances, which would automatically have the right schema according to the entity type they're referencing.

I'm not sure what to do with the statistics: Convert the entity_id to a string (but what about Postgres), ... (any other suggestions?)?

I'll have a look in to this.

@Jelle_S this will bring to separate data tables for comments with int and string IDs, that means you move that primary ID into linked table - that's wrong.

Another trick here is a comment bundle (comment type) that defines fields and by that we need to keep this "commented entity id" in base table.

PS: conversion issue #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form

Here's what I got so far. Tests aren't green yet but I'm just uploading my progress thus far.

For some reason, in following code in the CommentManager, the field settings do not get updated after save, they just get overridden again and the entity reference points to a user in stead of an entity_test... I'm still baffled how this happens... Couldn't find an explanation so far:

/**
   * {@inheritdoc}
   */
  public function addEntityField($comment_type_id) {
    if (!FieldConfig::loadByName('comment', $comment_type_id, 'comment_entity')) {
      $comment_type = CommentType::load($comment_type_id);
      // Attaches the body field by default.
      $field = $this->entityManager->getStorage('field_config')->create(array(
        'label' => 'Commented entity',
        'bundle' => $comment_type_id,
        'required' => TRUE,
        'field_storage' => FieldStorageConfig::loadByName('comment', 'comment_entity'),
        'settings' => array(
          'target_type' => $comment_type->getTargetEntityTypeId(),
          'handler_settings' => array(),
          'handler' => 'default:' . $comment_type->getTargetEntityTypeId(),
        ),
      ));
      $field->save();

// If I dump the settings here, the target type is 'user' and the handler is 'default:user' in stead of 'entity_test' and 'default:entity_test'.

      // Assign widget settings for the 'default' form mode.
      entity_get_form_display('comment', $comment_type_id, 'default')
        ->removeComponent('comment_entity')
        ->save();

      // Assign display settings for the 'default' view mode.
      entity_get_display('comment', $comment_type_id, 'default')
        ->removeComponent('comment_entity')
        ->save();
    }
  }

#18 makes sense but breaks BC in many places, at least history and comment statistics should use the same table split.
Suppose that should be done in contrib and then migrate to core

1. +++ b/core/modules/comment/comment.install
   @@ -39,10 +39,10 @@ function comment_schema() {
   +        'type' => 'varchar_ascii',
   ...
   +        'length' => 255,
   

   wonders if we have limits on entity_id as string

2. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -41,6 +41,8 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +        debug($entity->getCommentedEntity()->access($operation, $account, TRUE));
   +        debug($entity->getCommentedEntity());
   

   --

3. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -85,7 +87,7 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
   -        'entity_id',
   +        'comment_entity',
   

   we use "commented_entity" all over module

4. +++ b/core/modules/comment/src/CommentManagerInterface.php
   @@ -52,6 +52,14 @@ public function getFields($entity_type_id);
   +   * Creates a comment_entity field.
   ...
   +  public function addEntityField($comment_type);
   
   +++ b/core/modules/comment/src/CommentTypeForm.php
   @@ -169,6 +169,7 @@ public function save(array $form, FormStateInterface $form_state) {
   +      $this->commentManager->addEntityField($comment_type->id());
   
   +++ b/core/modules/comment/src/Tests/CommentTestTrait.php
   @@ -58,6 +58,8 @@ public function addDefaultCommentField($entity_type, $bundle, $field_name = 'com
   +    // Add an entity field to the comment type.
   +    \Drupal::service('comment.manager')->addEntityField($comment_type_id);
   
   +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityCommentType.php
   @@ -22,6 +22,7 @@ class EntityCommentType extends EntityConfigBase {
   +    \Drupal::service('comment.manager')->addEntityField(reset($entity_ids));
   
   +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCommentTest.php
   @@ -43,6 +43,7 @@ protected function setUp() {
   +    \Drupal::service('comment.manager')->addEntityField('comment_no_subject');
   

   is there a reason to keep that in manager?
   otoh that looks like a trick...
   Like having comment interface getCommentedEntity() method that accesses a field module provided field...

I tested this patch in combination with #2107249: Don't assume that content entities have numeric IDs in EntityReferenceItem. Not sure if this patch needs it, but with it the relevant tests were green on my machine.

I was working on an older D8 checkout. Rerolled patch.

Small error in reroll. New patch.

Still won't be green, but should be a lot closer to it.

+++ b/core/modules/comment/src/CommentViewBuilder.php
@@ -67,6 +67,9 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+    if (!$entity->getCommentedEntity()) {
+      debug(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS));
+    }

still some debug code

New patch. I think this should be green... Go testbot!

Side note:
This patch in itself does not allow comments to be attached to entities using a string primary key. But it changes the link between the entity and the comment to an entity_reference field. So when #2107249: Don't assume that content entities have numeric IDs in EntityReferenceItem lands (which is RTBC right now) it will be possible to do so.

I fixed the exception, but there is still one fail in Drupal\views\Tests\Entity\ViewEntityDependenciesTest: it expects comment, node and user as module dependencies for a view, but gets only comment and user. I'm not quite sure if it's actually a bug, or normal behavior, since the new views relation is provided by entity_reference, which already is a dependency for comment. I don't know enough about the way configuration management or views deals with this, so I could really use someone who knows a lot about those modules to have a look and see if it's normal behavior or a bug. If it's normal behavior, all that's left is to change that test...

Even weirder:

When I import the view on a D8 install with the patch and I execute following code in devel/php I do get the correct dependencies:

dpm(\Drupal\views\Views::getView('test_field_get_entity')->calculateDependencies());

Result:

Array
(
    [module] => Array
        (
            [0] => comment
            [1] => node
            [2] => user
        )

)

Never mind. Turns out I was looking at the wrong view (sigh). The test_relationship_dependency view was still using the old "node" relationship (which doesn't exist anymore because it's been replaced by entity_reference). This patch should be green.

I'd not expected it to be tested in the first place :)

The existing tests still test all the functionality. This piece of code was untested, even before this patch. Everything about comment that was tested, is still tested with this patch. So to me, covering that bit of code with tests, seems out of scope for this issue. It was just discovered that it wasn't tested because of this patch, but it isn't the cause of it. Tentatively removing the Needs tests tag.

+++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
@@ -137,7 +137,7 @@ public function preRender(&$values) {
-        INNDER JOIN {" . $comment_entity_table . "} ce ON c.cid = ce.entity_id AND ce.deleted = 0
+        INNER JOIN {" . $comment_entity_table . "} ce ON c.cid = ce.entity_id AND ce.deleted = 0

better to file separate issue to add test for that

+++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
@@ -131,7 +132,13 @@ public function preRender(&$values) {
-      $result = $this->database->query("SELECT n.nid, COUNT(c.cid) as num_comments FROM {node} n INNER JOIN {comment_field_data} c ON n.nid = c.entity_id AND c.entity_type = 'node' AND c.default_langcode = 1
+      $field_storage = FieldStorageConfig::loadByName('comment', 'commented_node');
+      $table_mapping = \Drupal::entityManager()->getStorage('comment')->getTableMapping();
+      $comment_entity_table = $table_mapping->getDedicatedDataTableName($field_storage);
+      $target_id_column = $table_mapping->getFieldColumnName($field_storage, 'target_id');
+      $result = $this->database->query("SELECT n.nid, COUNT(c.cid) as num_comments FROM {node} n
+        INNER JOIN {" . $comment_entity_table . "} ce ON c.cid = ce.entity_id AND ce.deleted = 0
+        INNER JOIN {comment_field_data} c ON n.nid = c.$target_id_column AND c.entity_type = 'node' AND c.default_langcode = 1

looks we have no tests for this handler at all

Am I correct in saying that both should be separate issues since they didn't have any tests before this patch either? (Or should it be one separate issue for both tests?)

New patch with the entity_id cast, as described in #46.

Ugh, wrong classname...

Needs profiling
Isn't a bug, see comments above - by design
Also why didn't we profile changing the column in HEAD to a string and casting numeric ids?

@attiks I suggest to use one of scripts from https://www.drupal.org/contributor-tasks/profiling that will allow you to get more precised numbers

Rerolled patch #49

@attiks yeah sure!
I will check it tomorrow!!

Since there's been no reaction from deepakaryan1988, I'll take a stab at a reroll.

Should be just a straight reroll.

Leaving it assigned to me. I'll have a look at writing updates.

Fixed the test and added an update function.

@Jelle_S Thanks for patch.
I was quite buzy.

This is a feature so 8.1 and later
And because there's serious data model change this could go to 9.x

Anyway we need to solve the issue with attaching comments to entities that have non-integer PKs

Here is my first go at trying to get this re-rolled. I tested it with my existing comment types and they all got switched to use entity reference.

The following updates are pending:

comment module : 
  8302 -   Update existing comments to use the new entity reference fields. 

Do you wish to run all pending updates? (y/n): y
Performing comment_update_8302                                              [ok]
Performing comment_update_8302                                              [ok]
Performing comment_update_8302                                              [ok]
Performing comment_update_8302                                              [ok]
Performing comment_update_8302                                              [ok]
Performing comment_update_8302                                              [ok]
Performing comment_update_8302                                              [ok]
Cache rebuild complete.                                                     [ok]
Finished performing updates.                                                [ok]

There were unfortunately a few adjustments that had to be made to the earlier patch:

1) For some reason updateEntityType was never called so when calling comment_update_8302 it would always say "base table or view not found" as the commented_update_8302 tabe was never created. Not sure if this was a change in 8.3.x. To solve this I looked at other hook_update_n invocations and did the following:

+/**
+ * Update existing comments to use the new entity reference fields.
+ */
+function comment_update_8302(&$sandbox) {
+  if (!isset($sandbox['total'])) {
+    $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+    $entity_type = $definition_update_manager->getEntityType('comment');
+    $definition_update_manager->updateEntityType($entity_type);

2) I converted all array()'s to short array syntax.

3) There were a few psr4 namespaces removed since last patch that were needed so I added them back mostly just:

use Drupal\field\Entity\FieldStorageConfig;

4) Slightly updated rdf module patch lines

5) I couldn't the relevant hal + migrate_drupal files to patch

6) I think there are going to be quite a few test failures as were on my local but wanted to post progress.

One minor correction to views/filter/UserUid.php. Retriggering.

This patch should fix the views.comment_recent schema issues and ForumIndexStorage missing $nid which was my error. ^_^

Fix for the "Undefined variable: entity_test" for CommentCacheTagsTest.php is now called:

'commented_entity_test' => $this->entityTestCamelid->id(),

I can handle the remaining code quality issues in next patch but unsure about bulk of rest of issues.

I do know there are some changes to be made in both HAL + Rest module to switch entity_id to commented_entity_test.

Any guidance on what else I missed would be appreciated ^_^

Okay I thought I would post status here and get any opinions. I was talking about this issue with a few drupal developers on and offline and the general consensus was I should be moving the comments + this patch functionality to a new contrib module likely called "external_comments".

Unfortunately I need this workflow to provide comments / ratings to external entities via CKAN / SOLR and expose them restfully as a required case. The worry with this patch would be the potential need to update for every core release and general maintenance issues.

I'll still be working on updating this patch but thought I would mention intentions to create a new contrib with this extended functionality and obviously pointing to the modules existence to only provide primary key as string storage.

Thoughts? :)

I think moving all of
https://git.drupalcode.org/project/drupal/-/blob/38d18a4f1f172a12374228c...
into
https://git.drupalcode.org/project/drupal/-/blob/38d18a4f1f172a12374228c...

would basically fix this?