The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
The uploaded patch fixes the reported bug but it actually creates another problem: potentially too many comments being deleted. This should probably be tackled first, there are several open issues for that.
Problem
Two things with the following code:
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')->accessCheck(FALSE);
$entity_query->condition('entity_type', $field->getEntityTypeId());
$entity_query->condition('field_name', $field->getName());
$cids = $entity_query->execute();
$comment_storage = \Drupal::entityTypeManager()->getStorage('comment');
$comments = $comment_storage->loadMultiple($cids);
$comment_storage->delete($comments);
}
}
- it doesn't actually delete anything, because queries for the wrong entity type. (Should be something like "node", actually is something like "node.article.comment" (or whatever the entity type of a field config entity is).
- but if that was fixed, it would delete all comments for this field, when we only removed the field from one bundle.
Steps to reproduce
Proposed resolution
Attached.
Test should fail; test+patch should pass.
If there is a better way to get from a FieldInstanceConfigInterface to its target entity's bundle field name, please tell me.
Remaining tasks
Postponed on #89181: Use queue API for node and comment, user, node multiple deletes
Update patch
Review
Commit
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | interdiff-2338457-4.txt | 1.2 KB | roderik |
| #4 | comment-field-delete-2338457-4.patch | 4.03 KB | roderik |
| comment-field-delete-testfail.patch | 2.36 KB | roderik |
Comments
Comment #3
berdirFix is missing a check if there are any ID's, that is easy to fix.
The query is already a problem in HEAD if it would work, as it doesn't scale. If you have 100k comments, you're going to have a bad time.
Nodes are currently *not* forcefully deleted when a node type is removed, see #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference. We will need a solution that scales for that, so we can possibly use that for comments and other entity types too. I can either think of triggering a queue backend task but then we need ensure that we can at least load and delete entities when the bundle doesn't exist anymore or we need to move this to a validation that you can not delete unless you deleted the entities first, which a delete UI should then support.
Comment #4
roderikThanks for the summary. Added a related issue for reference (there are many)
Fix attached but no needs-review. I'm unassigning myself not because I'm not interested, but realistically I won't get to it. If I get time I'll be interested in looking into all the queue API related issues.
Comment #5
roderik(since this is going to remain open: fixing my own lazy wording, to minimize confusion. As overheard on irc ;) )
Comment #6
larowlanPatch looks good but lets postpone on #89181: Use queue API for node and comment, user, node multiple deletes?
Comment #7
roderikAs per #2318875-8: Redo CommentStatisticsInterface: TODO:insert a
\Drupal::service('comment.statistics')->deleteMultiple();after the entity delete.Comment #19
quietone commentedUpdated Issue summary and tagging
Comment #20
andypostfixed title and summary for the current name and state