Problem/Motivation
If you have a site where the comments are translateable and your user cancel method is 'user_cancel_reassign', then in case if a user is being cancelled and it has comments which are also translated, those comments will be deleted.
Steps to reproduce
Set user cancel settings to user_cancel_reassign.
Have a node where the comments can be translated.
Create a user.
Comment with that user.
Translate with that user (or probably any other user).
Then cancel this user account with user_cancel_reassign.
Proposed resolution
Reassign the translations too to Anonymous user, not just the original comment (this is what the node module does too).
The problem lays in the comment_user_predelete() function, where the
$entity_query = \Drupal::entityQuery('comment');
$entity_query->condition('uid', $account->id());
$cids = $entity_query->execute();
will find the translated comments too, therefore when user is being deleted, the comment will die with it.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None.
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff_3166561_18-20.txt | 1.72 KB | andregp |
#20 | 3166561-20.patch | 5.48 KB | andregp |
| |||
#18 | 3166561-18.patch | 5.56 KB | ranjith_kumar_k_u |
#14 | 3166561-interdiff_9-14.txt | 4.18 KB | Matroskeen |
#14 | 3166561-14.patch | 5.57 KB | Matroskeen |
Comments
Comment #2
golddragon007 CreditAttribution: golddragon007 at European Commission and European Union Institutions, Agencies and Bodies, Petend commentedHere is the fix for it.
Comment #3
golddragon007 CreditAttribution: golddragon007 at European Commission and European Union Institutions, Agencies and Bodies, Petend commentedComment #4
larowlanComment #5
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commented@larowlan
I've uploaded the initial patch to test the comment translation delete.
I checked the node module and didn't found any test cases to validate node translation updates.
Do you think it would be helpful to add those in this issue?
Comment #6
henry.odiete CreditAttribution: henry.odiete at Portage CyberTech commentedReviewed the test and can confirm the patch provided works for comments. I don't see why not for the node module. though it's not part of the issue
Comment #7
henry.odiete CreditAttribution: henry.odiete at Portage CyberTech commentedComment #8
catchThe fix itself looks fine.
Some minor comments on the test coverage, and it would also be good to have a test-only patch here to show the test failure.
All of this code is self-explanatory and would be fine without a comment.
The first assertion could be split into two separate assertions - i.e. first test the owner ID is 0, then in a separate assertion, test that the comment it published.
The assertEqual() call doesn't need a custom message.
Comment #9
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedTried to address comment #8, please reivew.
Comment #11
MatroskeenJust re-uploading a patch from #9, so it stops failing and moving back to NW :)
[please don't credit me for that]
Comment #12
catchThis can be
$this->assertEqual($test_comment->getOwnerId(), 0);
now that it's been split.Comment #13
MatroskeenComment #14
MatroskeenI noticed some additional issues like:
1) Unused variable;
2) Deprecated asserts;
3) Unnecessary t() calls;
Here is the patch with a clean-up. Let's see.
Comment #18
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRe-reolled #14 for 9.5
Comment #20
andregp CreditAttribution: andregp at CI&T commentedFixed failing test from #18
Comment #21
victorml CreditAttribution: victorml commentedReviewed and tested. Works perfectly fine.
Comment #24
catchCommitted/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x.
Leaving RTBC against 9.4.x since we just did the patch release, will cherry-pick there once it's unfrozen.
Comment #26
xjmThe aforementioned cherry-pick never happened, so cherry-picked and pushed now after a different commit freeze on 9.4.x. :)