{comment} and {comment_entity_statistics} only support integer entity ids

Yeah! we can move \Drupal::state()->get('comment.maintain_entity_statistics') on per field/instance basis with settings.
The only question here are we going to reuse current comment field by simply extending its schema and definition?

Hm. So if you're going to use the field data table storage, then you should just update the entity field and save it. Messing with the field database tables directly is not allowed :)

Which essentially means that all the comment statistics stuff would be doing is calculate the statistics, does it even still need to be a service or could it simply be a method on Comment?

@berdir (un)posting a comment should cause a statistics calculation and affects commented entity so it is a field, the same time we have a comment field that actually displays this statistics (currently in node links) but supposed via own formatter #1920044: Move comment field settings that relate to rendering to formatter options

No you can not and yes, it is overkill. But if you use field storage, $entity->save() is the only API you're allowed to use.

Not updating the entity is one of the reasons this was never put into a field, and why it was moved out of the {node} table in 2003/4 or so.

Also the comment statistics aren't data attached to the field - it's denormalization/state, not field values as such.

If we had optimized entity updates, so that storage only gets written for fields that were explicitly changed, then this wouldn't be a problem (or at least less of a problem), but we don't have that yet.

I'm not really sure how we ended up at this issue, what's it blocking?

There's nothing blocked on it. Once #2101183: Move {comment_entity_statistics} to proper service is in we can implement calculated field a-la #2201051: Convert path.module form alters to a field widget

Right calculated field suits this better. So is the only issue #2195915: Cannot save text filter config using PostgreSQL if Comment is enabled [blocks installation!] then?

See #2195915: Cannot save text filter config using PostgreSQL if Comment is enabled [blocks installation!], which explains the need for per entity-type tables due to entity ID as string being supported now, which do not have to be field storage. As discussed, this would only solve half the problem anyway as we still have the main comments table.

Another thing is the entity cache, so what we should do at least is invalidate that cache when we change it. Right now I'm using the load_uncached() hook for this there.

I think uncached is OK for this sort of information - or at least it's a trade-off between cache invalidation and loading each time.

Or another option would be to not try to put the information onto the entity at all, and only access it when needed, which in the majority of cases is during rendering anyway.

@catch Also I'd like to point what we discussed with @berdir - statistics like history needs a way to "load multiple" for node listings at least, currently only configurable fields have this ability.
Cache invalidation for entity is needed here, otherwise we should move history/comment_stats into some "out-of-bound"/"post render" stage because:
1) history is a per user data, so should not be cached
2) comment statistics should be invalidated when comment added/removed - so actually commented entity is changing, this is a serious improvement for forum and probably front page too

@catch Also I'd like to point what we discussed with @berdir - statistics like history needs a way to "load multiple" for node listings at least, currently only configurable fields have this ability.

hook_entity_load() is just as multiple as configurable fields are, they receive the same set of entities at the same time.

comment statistics should be invalidated when comment added/removed - so actually commented entity is changing, this is a serious improvement for forum and probably front page too

The commented entity is only changing because we use hook_entity_load(). hook_entity_prepare_view() would attach the information for rendering, but not as part of the entity proper.

+1 to 2, but suppose better to remove 'entity' from table name so {comment_node_statistics}
PS Updated summary

Tagged as critical because we can't ship beta with things how they are

The prioritization criteria have changed. Downgrading accordingly. See https://drupal.org/core/issue-priority#critical-bugs

I think we need to take a conscious decision on this either way as to whether it's fixable or not at all in 8.x and whether we can change it safely before. Bumping back up until that's done.

Also, this sounds more like a bug to me, since elsewhere the entity system supports non-integer IDs.

Note that the decision here will likely affect what Views can be built, among other things. See #1986606: Convert the comments administration screen to a view. In that issue, a separate Views bug was preventing comments on non-nodes from being listed, but once resolved, we can potentially create a view that lists all comments for moderation -- not just on nodes. However, that isn't a feature of D7 (see also #1040786: Include entities in taxonomy_index), so maybe we just let it go.

I am guessing a denormalization table of some manner is probably still important to have, though?

I don't think that is related to this issue at all. I wanted to comment in that issue for a while that views should be doing the same as the current code is doing. a custom field formatter that does batch-loading of all referenced entities and displays it as a link, instead of using a relationship. We're already doing exactly that for the files view, see EntityLabel.

If we move things out into {comment_node} and {comment_user}, we no longer have the cross-entity-type, denormalized {comment} base table to query against. Same problem as Taxonomy has. It's not about the relationship or field formatting for results for each entity (which was the previous bug), it's about the listing of the entities to begin with in a performant query. But maybe best to move discussion over there. :)

To clarify #33 using the files example: There is one {file_managed} table. We don't have {file_node}, {file_user}, etc.

Yeah, you're right, sorry :)

file_usage.id is a string, though, but we're not doing joins on that...

As I said in #19 the history table has the same issue!

In ideal world, comments should be attached to "field instance record" not the entity itself.
But currently we duplicate this data into comment table (entity_type, field_name, entity_id)

So here's only a few solutions:
1) convert entity_id field to string
2) split comment table into comment_[entity_type] tables - preferred
3) add intermediate table to store entity_type, field_name, entity_id (as string) and point comment here
4) merge "entity_type:entity_id" into one field

PS: related #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form does not affects this

If we split into separate tables, I don't think we'd want the serial IDs for the comments to be split (i.e. duplicated), also Comment::load($cid) should still work.

So that seems to me that we'd need a {comment} table still, with the autoincrement ID and target entity type - if only to have unique numeric IDs and support Comment::load($cid);.

We discussed this issue with @larowlan, @dixon_, @alexpott, @effulgentsia, and myself, and decided that option 1 (simply not supporting string IDs for comment in core) is the best option at this point in the release cycle. If for some reason someone needs to support comments on an entity type with a string ID (@effulgentsia supplied the example of a remote entity), then they can swap out the service.

For this patch, we should:

• Throw an exception when a comment field instance is created for a bundle of an entity type with a string ID.
• form alter the field add form to not list comment fields in the dropdown as an option when it has a string ID.

I'll take a look at this and fix the last tests there.

I've been a bit on and off this today, but this should fix a couple of tests.

There were a few hard coded assumptions around entity IDs being integers that I had to work around in the tests that I'm not very happy with.
Also, when an entity type with string IDs have field tables generated I ran into problems because there are assumptions around the revision_id data type as well.

I'm thinking that we should break out this new test entity type into a separate set of simpler tests...

That approach looks good and does the job!

1. +++ b/core/modules/comment/comment.module
   @@ -264,6 +264,15 @@ function comment_field_config_delete(FieldConfigInterface $field) {
   +    $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);
   +    $entity_type_id_key = $entity_type->getKey('id');
   +    $field_definitions = \Drupal::entityManager()->getBaseFieldDefinitions($entity_type->id());
   +    $entity_type_id_definition = $field_definitions[$entity_type_id_key];
   

   This exact code is repeated twice - can we add a helper? _comment_entity_uses_integer_id() will do.

2. +++ b/core/modules/comment/comment.module
   @@ -264,6 +264,15 @@ function comment_field_config_delete(FieldConfigInterface $field) {
   @@ -812,6 +821,15 @@ function comment_form_field_ui_field_overview_form_alter(&$form, $form_state) {
   
   @@ -812,6 +821,15 @@ function comment_form_field_ui_field_overview_form_alter(&$form, $form_state) {
      if ($form['#entity_type'] == 'comment' && $request->attributes->has('commented_entity_type')) {
        $form['#title'] = \Drupal::service('comment.manager')->getFieldUIPageTitle($request->attributes->get('commented_entity_type'), $request->attributes->get('field_name'));
      }
   +  $entity_type_id = $form['#entity_type'];
   +  $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);
   +  $entity_type_id_key = $entity_type->getKey('id');
   +  $field_definitions = \Drupal::entityManager()->getBaseFieldDefinitions($entity_type->id());
   +  $entity_type_id_definition = $field_definitions[$entity_type_id_key];
   +  if ($entity_type_id_definition->getType() != 'integer') {
   +    // You cannot use comment fields on entity types with non-integer IDs.
   +    unset($form['fields']['_add_new_field']['type']['#options']['comment']);
   +  }
   

   Is it possible to do this check where the option is added, rather than removing it later?

Is it possible to do this check where the option is added, rather than removing it later?

Fields are included in the "New field" dropdown list on the field overview page for a content type if they do not declare a no_ui property in their @FieldType annotations.

foreach ($field_types as $name => $field_type) {
  // Skip field types which should not be added via user interface.
  if (empty($field_type['no_ui'])) {
    $field_type_options[$name] = $field_type['label'];
  }
}

I don't know of an intervention point before comment_form_field_ui_field_overview_form_alter to unset this value.

I addressed the duplicated code from 50:1 in this patch.

I addressed the duplicated code from 50:1 in this patch.

Awesome.

I don't know of an intervention point before comment_form_field_ui_field_overview_form_alter to unset this value.

Fair enough.

I think the function belongs elsewhere, there is similar need in entity reference item's schema method.
Perhaps in entity module or on one of the entity storage classes?

The entity module is about to be removed soon I think. @fago have plans for working on that I've heard.

And considering this is already procedural code I'm fine with keeping that code in _comment_entity_uses_integer_id() as @catch already suggested.

Committed e05d622 and pushed to 8.x. Thanks!

+++ b/core/modules/comment/comment.module
@@ -264,8 +264,13 @@ function comment_field_config_delete(FieldConfigInterface $field) {
+      throw new \UnexpectedValueException(t('You cannot attach a comment field to an entity with a non-integer ID field.'));

Exception messages should not use t() or end with a full stop. Fixed during commit.

Interesting...

Approach #2 could have been implemented with #1995944: Remove entity_id and entity_type from the comment table and replace with relationship tables .

oh well, I think going with Approach #3 was better in terms of getting Drupal 8 out the door. But I think it's important to evaluate database design and determine the unintended consequences of whatever design we implement.

Thanks!

I created a follow up, because now we allow people to define comment types for all entities, but we hide it if the field is using a string id: #2496913: Don't expose entity types with string ids as a target option when creating comment types