Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted

Adding tag Migrate Drupal which indicates that this is a blocker for Migrate Drupal becoming stable.

I checked the behavior of other field types when the field instance is deleted. When there is still content left in the database for the field in question, there is no other warning to the site builder than Are you sure you want to delete the field {field_name}? This action cannot be undone.

There is no need to have more warnings for comment field instance deletions when there are still comments left in the database like I originally proposed. Issue summary updated accordingly.

First observations.

When the comment field is deleted, hook implementation comment_field_config_delete is invoked.

This attempts to delete all comments of the comment field instance.

function comment_field_config_delete(FieldConfigInterface $field) {
  if ($field->getType() == 'comment') {
    
    // Delete all comments that used by the entity bundle.
    $entity_query = \Drupal::entityQuery('comment');
    $entity_query->condition('entity_type', $field->getEntityTypeId());
    $entity_query->condition('field_name', $field->getName());
    $cids = $entity_query->execute();
    entity_delete_multiple('comment', $cids);
  }
}

Unfortunately this fails to delete any comments because the query seems to return an empty result set. This seems to be because of
$entity_query->condition('entity_type', $field->getEntityTypeId());

When we modify this query a bit, we can get the comments deleted as expected. The only thing that we need to be super careful with is that the same comment field can be re-used on multiple content types. For example, the D8 standard comment comment field can be re-used on multiple content types and we need to make sure that we are limiting the deletion to just the content type where we removed the comment field from.

Deleting the entries from comment_entity_statistics is going to be difficult for this same reason. The field_name is too broad WHERE clause because we only want to delete the entries of that content type where the comment field was deleted from.

I can try to work out with the patch in the next couple of days. If somebody else wants to pick this first, feel free to do so. Just assign this to yourself so that we are not doing double effort on this. I'll assign this to myself when I have time to dig deeper...

Cheers,
Markus

Re: #7.

This issue will resolve the issue for Article content type (when using D8 Standard) even if the site was migrated earlier. After this issue is resolved, the site builder can delete the comment comment field from Article and that will remove the duplicate entries from comment_entity_statistics. The deletion of comment comment field of Article should be safe to do because the comments were migrated to a comment field comment_node_article.

This issue will not help for Forum comments that were migrated earlier. The reason is that comment migration incorrectly migrated the Forum comments to comment_node_forum instead of comment_forum. #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics will fix that Forum comments will be migrated correctly after #2853872 lands but even that will not fix the already migrated Forum comments.

The Forum comment topic is anyway in the scope of #2853872, not this issue.

Cheers,
Markus

Updated the issue title to better describe the two issues we have here:
1) Comments are not deleted when the comment field is deleted
2) We are also leaving orphan entries to comment_entity_statistics

I'll most probably have time this week to write the first patch.

Notes to self for the patch:

Hook implememtation for comment_field_config_delete()

We need to have two parts here:

1. First find all comments where a) comment field name equals the comment field instance that was deleted and b) bundle (for example content type) equals the bundle where the comment field instance was deleted from. Then delete these comments with entity_delete_multiple like we try to do now.

2. comment_entity_statistics table alone doesn't have all the information that is needed for deleting the data from that table. The reason is that field_name is too broad selection criteria due to a fact that a comment field can be used on multiple bundles (e.g. node content types).

We know the bundle (e.g. node content type) where the comment field was deleted from. We also know the comment field_name. Therefore:

First obtain a list of entity_ids (for example node ids) of the bundle where the comment field was deleted from. Then, delete all records from comment_entity_statistics where entity_id is on this list AND comment field_name is the comment field which was just deleted.

The attached patch should do the trick.

Setting to Needs Review to get feedback on the approach. Tests are not included, I'll have to leave that to someone who is more experienced with test automation. Please set this back to Needs work at least for the test but possibly also for other needed updates.

I would like to thank heddn for IRC mentoring regarding the "bundleability" of different entity types. heddn++

Proposed test cases:
1. Comment field shared between two node content types

• Same comment type (for example 'comments') is used by content type A and B.
• There are nodes of type A and B, all have comments
• Comment field 'comment' is deleted from B
• Expected result: all comments of B are deleted. All comments of A are not deleted
• Expected result: Statistics for B / comments are deleted from comment_entity_statistics. Statistics for A and comments are not deleted

2. Content type has two comment fields

• Content type 'Article' has two comment fields: 'comment' and 'comment_node_article'
• There are Article nodes. The nodes have entries in both comment fields.
• Comment field 'comments' is deleted
• Expected result: The comments of the 'comments' field are deleted. Comments of the 'comment_node_article' field are not deleted.
• Expected result: The comment statistics of Article / comments are deleted. Comment statistics of Article / comment_node_article are not deleted

3. User comments

• Comment fields can be attached to any entity types, not just nodes.
• So let's also add a test for User entity with comment_user field. User entity is not "Bundleable"
• There are Users with comments.
• Comment field 'comment_user' is deleted.
• Expected result: The comments of the 'comment_user' field are deleted.
• Expected result: The comment statistics for 'comment_user' are deleted.

Cheers,
Markus

Thanks for your review heddn!

• Both remarks from #13 are now addressed.
• Issue summary has been updated
• Remaining tasks: add tests, final review, commit
• I'll leave this to Needs work for the tests. As mentioned earlier, I need to leave this to someone with more experience with tests.

Cheers,
Markus

Changed the version to 8.4.x. Reasons:

1) This bug leads to orphan entries in the database. I'm not quite sure if orphan entries in the database are considered as corruption of stored data which would justify this to be critical. I'll leave this to major even though the corrupted / orphan entries in comment_entity_statistics do cause issues with for example views that have last commented / updated field.

2) As long as this bug is present the upgrade path to D8 is broken if D8 site was installed using Standard install profile and the site uses Article content type with comments (see issue summary for further details on this aspect).

3) Because of 2, this blocks migrate drupal from being stable in core.

Cheers,
Markus

Hi @andypost and @larowlan!

Re: #16-17 and the proposed split.

Re: 16.1: I think we have a consensus that the migration part should be handled under #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics.

Re: 16.3: I don't see how Forum would need separate handling for this issue which is about deleting *any* comment field. If you were referring to the migration of Forum comments, that should be covered under #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics.

To summarize the remaining tasks (from 16.2)
1. Enhance the UI for comment field deletion

• For the sake of clarity: we are talking about the deletion of comment field from an entity. We are not talking about the deletion of comment type itself.
• Currently the UI warns the user that the deletion of comment field is permanent and can't be undone. This warning applies to deletion of all fields, regardless of the type of the field that is about to be deleted.
• According to 16.2 we should enhance this UI so that this warning says (in addition to the existing warning) that all content of the field that is about to be deleted will permanently be lost. We should probably show the number of comments that would be deleted in this warning.
• This UI enhancement is proposed to be handled as a separate child issue

2. Handle the deletion of comments and comment statistics as a batch job

• Currently we try to delete the comments from hook implementation comment_field_config_delete
• But currently this does not delete anything because of the bug in the entityquery conditions, see #6 for root cause analysis
• The proposed approach from 16.2 is to delete the comments and comment statistics using a batch job instead of directly deleting them from the hook implementation like we try to do today.
• This change can be handled under this issue i.e. 2906470

@andypost, @larowlan: could you please review and confirm that this is the approach you prefer? I can then update the issue summary accordingly.

Cheers,
Markus

Child issue #2908607: Show warning when comment field is about to be deleted created for the improved UI part and issue summary updated to reflect #16-19.

@andypost, I read through the related issue that you mentioned: #2282119: Make the Entity Field API handle field purging

That looks really promising. However, it has a separate follow-up issue for bundle fields, see #2907780: Add a field purger service. I read through that as well and my understanding is that we need that as well.

Before postponing this issue on those two, could you please check if this also your understanding so that we don't postpone this for nothing?

Cheers,
Markus

Edit: fixed the related issue for purging bundle fields, I originally linked to an incorrect issue.

Raised to critical as per webchick's comment here: https://www.drupal.org/node/2853872

Markus

#2282119: Make the Entity Field API handle field purging just landed but my understanding is that we still need #2907780: Add a field purger service.

Hi all,

amateescu or anyone else, could you please bump me into the correct direction so that I could give this another shot?

Previously, the patch #14 implemented hook_ENTITY_ID_delete which was invoked when a comment field instance was deleted. Now that we have #2282119: Make the Entity Field API handle field purging, I understood that we should have improved capabilities to delete the comments when the comment field instance is deleted.

By reading the change record and issue summary of #2282119: Make the Entity Field API handle field purging, it is unfortunately not clear to me how this should be implemented. Could somebody clarify this a bit? We're trying to get the Migrate Drupal stable in core and as mentioned in the issue summary, this issue is a blocker for that.

Summary
When a comment field instance is deleted from a bundle, the comments of that field must be deleted. Could someone please give a piece of advice what is the proper way to do that?

Cheers,
Markus

Hi, thanks for taking the time @andypost for this issue!

I'm a bit confused, though. In #16 your guidance was to NOT delete the database entries directly from a hook implementation but using the Field API purging...?

Markus