Updated: Comment #N
Tagged as critical because we can't ship beta with things how they are

Problem/Motivation

Entity ids no longer have to be numeric.
{comment} stores entity ids as integers.
Comment stores entity statistics in {comment_entity_statistics} table but entity_id in there is assumed to be integer.
comment_entity_load handles loading up data from {comment_entity_statistics} table and adding to the CommentItem field item when attached to a node (allowing $node->commentfield->comment_count etc. This is outside the normal field storage stack.
#2195915: Cannot save text filter config using PostgreSQL if Comment is enabled [blocks installation!] manifests this problem

Proposed resolution

Approach 1

Drop {comment_entity_statistics} and store the statistics in the field tables.
Comment's various entity hooks ensure that data is in sync (write/updates), delegated to CommentStatisticsInterface service.
comment_entity_load() is removed, field api handles loading for us.
Make entity id field in {comment} a varchar

Approach 2

Make separate tables for each entity type, comment_node, comment_user, comment_node_statistics, comment_user_statistics
Remove the entity_type field/column
Make schemas generate automagically.

Approach 3

Comment fields aren't supported for non-integer entity ids

Remaining tasks

Decide on best approach
Patch
Test
Review
etc

User interface changes

None

API changes

Not sure yet

Files: 
CommentFileSizeAuthor
#51 interdiff.txt3.06 KBjessebeach
#51 comment-string-ids-2205215-51.patch7.73 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,889 pass(es).
[ View ]
#46 comment-string-ids-2205215.07a7f8c.patch7.32 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,850 pass(es).
[ View ]
#46 interdiff.txt1.12 KBlarowlan
#44 2205215-comment-string-ids-44.patch15.74 KBdixon_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,196 pass(es), 5 fail(s), and 3 exception(s).
[ View ]
#39 comment-string-ids-2205215.16c2c23.patch7.27 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,707 pass(es), 7 fail(s), and 16 exception(s).
[ View ]
#8 remove-comment-statistics-2205215.8.patch22.74 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,818 pass(es), 242 fail(s), and 20 exception(s).
[ View ]

Comments

larowlan’s picture

andypost’s picture

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?

larowlan’s picture

Yes and yes

larowlan’s picture

Status:Active» Needs review
StatusFileSize
new22.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 60,923 pass(es), 361 fail(s), and 47 exception(s).
[ View ]
new15.02 KB

This will blow up because tracker/forum et-al hard-wired to use that table.
Lets see how it goes for comment module though.

This is based off of #2101183: Move {comment_entity_statistics} to proper service
do-not-test-patch is this issue only
main patch combines both.

Status:Needs review» Needs work

The last submitted patch, 4: remove-comment-statistics-2205215.1.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
Issue tags:-undefined+Entity Field API
StatusFileSize
new1.23 KB
new22.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove-comment-statistics-2205215.2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new16.97 KB

We don't need merge anymore, as record always exists, its just update now.
Not sure what to do with langcode...

Again patch includes #2101183: Move {comment_entity_statistics} to proper service, do-not-test patch is diff against that.

Status:Needs review» Needs work

The last submitted patch, 6: remove-comment-statistics-2205215.2.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new22.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,818 pass(es), 242 fail(s), and 20 exception(s).
[ View ]
Berdir’s picture

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?

andypost’s picture

@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

Status:Needs review» Needs work

The last submitted patch, 8: remove-comment-statistics-2205215.8.patch, failed testing.

larowlan’s picture

@berdir can I save a single field? if so yes that is the answer.
otherwise I can do the whole entity again, but seems overkill.

Berdir’s picture

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.

catch’s picture

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?

andypost’s picture

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

catch’s picture

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?

Berdir’s picture

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.

catch’s picture

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.

andypost’s picture

@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

Berdir’s picture

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

catch’s picture

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.

larowlan’s picture

So how to proceed here? (and on the general {comment}.entity_id only supports integers)

Another option is that we simply don't support comment-fields on entity types with non integer ids.

larowlan’s picture

Title:Remove {comment_entity_statistics} table and store data in field tables» {comment} and {comment_entity_statistics} only support integer entity ids
Issue summary:View changes

New title and updated issue summary to outline two alternate approaches

larowlan’s picture

Issue summary:View changes

added proposed approach 3

larowlan’s picture

Issue summary:View changes
Issue tags:+Needs architectural review

Adding needs arch review tag to get some requests for feedback

larowlan’s picture

Personal preference is option 2, a similar approach will be needed for history module so some reusable schema building logic could be extracted from field module and used by all three.

andypost’s picture

Issue summary:View changes

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

moshe weitzman’s picture

Priority:Critical» Major

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

catch’s picture

Priority:Major» Critical
Issue tags:+beta blocker

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.

xjm’s picture

Category:Task» Bug report

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

xjm’s picture

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?

Berdir’s picture

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.

xjm’s picture

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

xjm’s picture

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

Berdir’s picture

Yeah, you're right, sorry :)

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

andypost’s picture

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

catch’s picture

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

xjm’s picture

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.
larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new7.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,707 pass(es), 7 fail(s), and 16 exception(s).
[ View ]

Something like this

Status:Needs review» Needs work

The last submitted patch, 39: comment-string-ids-2205215.16c2c23.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 39: comment-string-ids-2205215.16c2c23.patch, failed testing.

dixon_’s picture

Assigned:Unassigned» dixon_

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

dixon_’s picture

Status:Needs work» Needs review
StatusFileSize
new15.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,196 pass(es), 5 fail(s), and 3 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 44: 2205215-comment-string-ids-44.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new1.12 KB
new7.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,850 pass(es).
[ View ]

What if we only use that type in route discovery?

larowlan’s picture

Winner winner chicken dinner

dixon_’s picture

Status:Needs review» Reviewed & tested by the community

That approach looks good and does the job!

dixon_’s picture

Assigned:dixon_» Unassigned
catch’s picture

Status:Reviewed & tested by the community» Needs work
  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?

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new7.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,889 pass(es).
[ View ]
new3.06 KB

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.

larowlan’s picture

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?

dixon_’s picture

Status:Needs review» Reviewed & tested by the community

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.

  • Commit e05d622 on 8.x by alexpott:
    Issue #2205215 by larowlan, jessebeach, dixon_: Fixed {comment} and {...
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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.

Status:Fixed» Closed (fixed)

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

andypost’s picture

davidwbarratt’s picture

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!

attiks’s picture

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: UX problem with comment types