I ran into an unfortunate bug with the Comment module. Steps to reproduce:
- Create an article as the admin user.
- Log in as a regular user and leave a comment on the article.
- Log back in as the admin, disable the Comment module, delete the regular user, and then turn the Comment module back on again.
- 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.
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedHere'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
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedMore accurate title.
Comment #3
yched CreditAttribution: yched commentedBack 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.
Comment #4
webchickThe 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.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedLike 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."
Comment #6
kscheirerRetesting against latest HEAD since it has been over a year.
Comment #8
pwolanin CreditAttribution: pwolanin commentedComment #9
PawelR CreditAttribution: PawelR commentedRerolled patch.
Comment #11
PawelR CreditAttribution: PawelR commented9: comment-author-deleted-1451072-9.patch queued for re-testing.
Comment #13
alansaviolobo CreditAttribution: alansaviolobo commentedComment #15
DuttonMa CreditAttribution: DuttonMa commentedComment #16
DuttonMa CreditAttribution: DuttonMa commentedI'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.
Comment #17
DuttonMa CreditAttribution: DuttonMa commentedD7 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.
Comment #18
mgiffordThis 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.
Comment #19
steinmb CreditAttribution: steinmb as a volunteer commentedI did re-run bot, but I think the failing tests on PHP 7.1 is unrelated. Without this patch we still fail with:
I think we can try and RTBC this.
Comment #20
AltaGrade CreditAttribution: AltaGrade commentedComing 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.
Comment #21
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedRerolled the patch from #17 as it no longer applied. Also uploading a separate test-only version to see if the test is working correctly.
Comment #24
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedIf 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 ofcomment_load_multiple
(which is probably not worth the effort in this phase).Adding a tag for the second maintainer review.
Comment #28
mcdruidI 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.
Comment #30
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedCommitted, thanks everyone!