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.

Files: 
CommentFileSizeAuthor
#17 comment-author-deleted-1451072-2-D7.patch4.38 KBDuttonMa
PASSED: [[SimpleTest]]: [MySQL] 40,975 pass(es).
[ View ]
#13 interdiff.txt4.59 KBalansaviolobo
#13 deleting_a_comment-1451072-13.patch876 bytesalansaviolobo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,509 pass(es), 30 fail(s), and 18 exception(s).
[ View ]
#9 comment-author-deleted-1451072-9.patch4.15 KBPawelR
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-author-deleted-1451072-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 comment-author-deleted-1451072-1.patch4.14 KBkscheirer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-author-deleted-1451072-1_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 comment-author-deleted-1451072-1-TESTS-ONLY.patch2.64 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 34,636 pass(es), 1 fail(s), and 3 exception(s).
[ View ]
#1 comment-author-deleted-1451072-1.patch4.14 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 34,657 pass(es).
[ View ]
#1 comment-author-deleted-1451072-1-D7.patch4.1 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-author-deleted-1451072-1-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

David_Rothstein’s picture

Status:Active» Needs review
StatusFileSize
new4.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-author-deleted-1451072-1-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new4.14 KB
PASSED: [[SimpleTest]]: [MySQL] 34,657 pass(es).
[ View ]
new2.64 KB
FAILED: [[SimpleTest]]: [MySQL] 34,636 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

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

StatusFileSize
new4.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-author-deleted-1451072-1_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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
StatusFileSize
new4.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-author-deleted-1451072-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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
StatusFileSize
new876 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,509 pass(es), 30 fail(s), and 18 exception(s).
[ View ]
new4.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

Status:Needs work» Needs review
StatusFileSize
new4.38 KB
PASSED: [[SimpleTest]]: [MySQL] 40,975 pass(es).
[ View ]

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.