Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.This is related to 3158310.
I'm testing this today on drupal/private_message dev-2.x at a91c8f9 and Drupal 8.9.3. No other contrib modules installed.
Steps to reproduce
- Enable private message, grant permission to use it and view user profiles to authenticated users.
- Create three users.
- Sign in as User A, click Create Private Message (private-message/create), and send a message to both User B and User C.
- Sign in as User B, click Create Private Message (private-message/create), and send a message to just User C.
- Sign in as User C, click Create Private Message (private-message/create), and send a message to just User A.
All of the above messages end up on a single thread for all three users (see attached image).
Proposed resolution
A new thread should be created when a user creates a new private message to a different group of recipients than already exists. When User B sends a message just to User C, User A should not receive it.
I'm not going to set the priority higher than Normal but this seems like a big deal, no? Wouldn't this means that if someone spams and sends private messages with a bunch of recipients, then they'll always be included in discussions between any of those recipients?
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | private_message-threads_collapse-3164669-5.patch | 967 bytes | floydm |
| Screen Shot 2020-08-11 at 1.03.19 PM.png | 489.37 KB | floydm | |
Issue fork private_message-3164669
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
floydm CreditAttribution: floydm at Affinity Bridge commentedComment #3
floydm CreditAttribution: floydm at Affinity Bridge commentedComment #4
floydm CreditAttribution: floydm at Affinity Bridge commentedI think this is related to some code dropped from src/Mapper/PrivateMessageMapper.php in commit 9e0c4cb for Issue #3154558 1116 Too many tables; MariaDB can only use 61 tables in a join
The dropped section in question began with the comment "Next, do a left join for all rows that don't contain the users, and ensure that there aren't any additional users in selected threads". I may be missing something but I don't see that logic being recreated anywhere else.
Comment #5
floydm CreditAttribution: floydm at Affinity Bridge commentedI believe the attached patch restores the missing and logic fixes the issue of the wrong thread being selected.
Admittedly I am not sure what the broader implications of this change would be on performance or other related tickets.
Comment #6
floydm CreditAttribution: floydm at Affinity Bridge commentedComment #7
deedy243 CreditAttribution: deedy243 commentedHello Floydm,
It doesn't work for me. Can you help me.
Comment #8
apadernoFor some reason, the patch hasn't been queued for tests. I manually added it to the test queue.
Comment #9
artem_sylchukThanks @floydm,
Looks like the patch wasn't created against the latest dev version, I'll try updating it and reviewing the code.
Comment #11
artem_sylchukAttached the MR with re-roll of the patch. Additionally I simplified a bit the rest of the method's code.
It was trying to rely on the count of the members (assuming that should be unique).
As we see in @floydm example it didn't work correctly.
So I removed the part doing count comparison.
I will try to add tests for this code.
Also I like the idea of allowing to have multiple threads for the same users (I've seen the patch for that before, will try to find a review it too).
Comment #12
artem_sylchukLooks like my test highlighted another problem:
It comes from the members widget and the way it is added to the private message form.
Comment #14
artem_sylchukComment #15
artem_sylchuk