I ran into an unfortunate bug with the Comment module. Steps to reproduce:

  1. Create an article as the admin user.
  2. Log in as a regular user and leave a comment on the article.
  3. Log back in as the admin, disable the Comment module, delete the regular user, and then turn the Comment module back on again.
  4. Visit the node, and you get a nice fatal EntityMalformedException error.

The issue is that while the Comment module is disabled, its hooks (for user deletion) cannot run, so it can't delete the comment, and an orphaned comment is left over. (Similar orphaned comments occur if you delete a node while the Comment module is disabled, but in that case I'm not currently aware of any visible side effects; maybe you can get some with Views though.)

I debated between "normal" and "major" for this issue. It takes an odd set of steps to reproduce, but if you do find yourself in this situation you have broken nodes on your site and no way to repair them. I went with "major" for now.

Patches will follow in a second.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Here's a D8 patch (and one with tests only to demonstrate the bug), plus a D7 patch.

The proper fix for this would presumably involve the Comment module deleting all orphaned comments en masse, as soon as it is turned back on. The problem is that once the author is gone, the comments can't even be loaded, so they can't be properly deleted with comment_delete_multiple() either. So the best you could do in terms of cleaning this up in the database is to manually delete the rows in the {comment} table, but not do a proper deletion that cleans up everything else associated with the comment.

That seemed worse than not deleting the comment at all, so I went with a simpler solution that leaves the comment orphaned but makes sure the display code properly deals with the possibility that none of the comments that are expected to be attached to the node actually exist anymore.

See also: #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed

David_Rothstein’s picture

Title: Deleting a comment author while the Comment module is disabled leads to an EntityMalformedException error » Deleting a comment author while the Comment module is disabled leads to an EntityMalformedException error after it's reenabled

More accurate title.

yched’s picture

Back in #1067750: Let Field API fail in a tale-telling way on invalid $entity, while trying to sort out the various avatars of "Cannot access empty property in field.attach.inc" errors, I found a couple reports about those happening on cron, during node indexing.
Was comment_load_multiple() returning erroneous results for some reason (I couldn't do extensive remote debugging of the user's {comment} table).

The fix looks correct to me.

webchick’s picture

Priority: Major » Normal

The steps to reproduce this are very unlikely to happen very often in the real world, and it's not worth holding up D8 for this IMO. Reducing to normal.

David_Rothstein’s picture

Like I said, I was on the fence... although I will point out that an issue like this one does seem to be exactly along the lines of what the documentation describes as major:

"An example would be a PHP error which is only triggered under rare circumstances or which affects only a small percentage of all users."

kscheirer’s picture

Retesting against latest HEAD since it has been over a year.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, comment-author-deleted-1451072-1.patch, failed testing.

pwolanin’s picture

Issue summary: View changes
Issue tags: +Needs backport to D7, +Needs reroll
PawelR’s picture

Status: Needs work » Needs review
FileSize
4.15 KB

Rerolled patch.

Status: Needs review » Needs work

The last submitted patch, 9: comment-author-deleted-1451072-9.patch, failed testing.

PawelR’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: comment-author-deleted-1451072-9.patch, failed testing.

alansaviolobo’s picture

Status: Needs work » Needs review
FileSize
876 bytes
4.59 KB

Status: Needs review » Needs work

The last submitted patch, 13: deleting_a_comment-1451072-13.patch, failed testing.

DuttonMa’s picture

Assigned: Unassigned » DuttonMa
DuttonMa’s picture

Version: 8.0.x-dev » 7.x-dev

I'm pretty sure that we don't need to patch this, as it is no longer possible to reproduce the problem in Drupal 8.

I just tried to go through the steps to reproduce, but I could not disable the comment module.

I tried to disable it through the admin screens, but the checkbox is greyed out on the modules 'list' tab and the module does not appear on the 'uninstall' tab.
Of course it is not possible to disable modules through drush anymore, but I tried 'drush pm-uninstall comment' and that did not work either.

All that remained was to try deleting the user anyway, so I tried all 4 options for that:
- disable the account and keep its content - works OK and leaves comment
- disable the account and unpublish its content - comment disappears correctly
- delete the account and make its content belong to the anonymous user - comment remains correctly and is attributed to the old (now non-existent) user name with the text '(not verified)' after the user name
- delete the account and its content - comment disappears correctly

This problem does still exist in Drupal 7, however, so I am changing the 'version' on this issue to 7.x-dev and will start work on the D7 patch.

DuttonMa’s picture

D7 patch re-rolled.

N.B. I have also added code to update the comment count if it is currently greater than zero and the new code detects that the comments have actually been deleted.

mgifford’s picture

Issue tags: -Needs reroll

This looks like a good patch to me. Test works just fine in a new install on SimplyTest.me.

Ran through the steps to reproduce in the summary and ya.. That's one ugly error without the patch.

Not sure what else is required to mark this RTBC.

steinmb’s picture

Assigned: DuttonMa » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D7

I did re-run bot, but I think the failing tests on PHP 7.1 is unrelated. Without this patch we still fail with:

    Notice: Undefined variable: key in comment_prepare_thread() (line 901 of drupal/modules/comment/comment.module).
    Warning: Creating default object from empty value in comment_prepare_thread() (line 901 of drupal/modules/comment/comment.module).
    EntityMalformedException: Missing bundle property on entity of type comment. in entity_extract_ids() (line 7933 of drupal/includes/common.inc).

I think we can try and RTBC this.

AltaGrade’s picture

Coming from https://www.drupal.org/project/drupal/issues/2363369#comment-13270030 This is just to confirm the patch in #17 took care of the problem in our use case.

The last submitted patch, 21: 1451072-21_test-only.patch, failed testing. View results

The last submitted patch, 21: 1451072-21_test-only.patch, failed testing. View results

poker10’s picture

Issue tags: +Pending Drupal 7 commit

If we are OK with the fact, that the orphaned comments are not deleted and actually left in the database (see the comment #1), then I think this seems good.

Just a note - these orphaned comments are not visible in the node detail, nor in the comments list at admin/content/comment, so users will have no idea about these data being left in the database. This patch will remove the error, but the actual cause of the problem will not be resolved. On the other hand, it would require a lot bigger effort to allow correctly delete the comments because of comment_load_multiple (which is probably not worth the effort in this phase).

Adding a tag for the second maintainer review.

The last submitted patch, 21: 1451072-21_test-only.patch, failed testing. View results

The last submitted patch, 21: 1451072-21_test-only.patch, failed testing. View results

The last submitted patch, 21: 1451072-21_test-only.patch, failed testing. View results

mcdruid’s picture

Issue tags: -Pending Drupal 7 commit +RTBM

I think this looks good.

If anyone was afflicted with an orphaned comments problem because of a particular workflow on their site, they could write a module / drush command to address that. I don't think we need to hold this fix up on trying to solve that problem though.

  • poker10 committed 7c73718b on 7.x
    Issue #1451072 by David_Rothstein, DuttonMa: Deleting a comment author...
poker10’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBM

Committed, thanks everyone!

Status: Fixed » Closed (fixed)

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