Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete attachments (but keep email) #629

Merged
merged 1 commit into from
Jul 7, 2023
Merged

Conversation

josaphatim
Copy link
Member

Pullrequest

Removes specified attachment part keeping the rest of email.

Issues

@josaphatim josaphatim marked this pull request as draft September 29, 2022 13:26
@josaphatim josaphatim changed the title Draft: Delete attachments (but keep email) Delete attachments (but keep email) Sep 29, 2022
@josaphatim josaphatim force-pushed the delete-att branch 6 times, most recently from e60252e to 907cb45 Compare September 29, 2022 17:59
@josaphatim josaphatim marked this pull request as ready for review September 29, 2022 18:22
@josaphatim josaphatim changed the title Delete attachments (but keep email) Draft: Delete attachments (but keep email) Dec 13, 2022
@josaphatim josaphatim changed the title Draft: Delete attachments (but keep email) Delete attachments (but keep email) Dec 13, 2022
@josaphatim josaphatim force-pushed the delete-att branch 4 times, most recently from 929258b to fb83199 Compare December 13, 2022 13:05
@marclaporte
Copy link
Member

@jasonmunro @kroky Since this is deleting data, I could use a third opinion :-)

@kroky
Copy link
Member

kroky commented Apr 21, 2023

Code looks good but the remove attachment itself looks a bit hacky with all that string manipulation of a mime message. Isn't it better to use a Mime lib to parse the message, remove the attachment from the message structure and then format the message again? I am afraid the current code may fail in some edge cases or ill-formatted messages and replace more than just the attachment in which case a data loss will occur.

@dumblob
Copy link
Member

dumblob commented Apr 21, 2023

Hm, is this done transactionally? If not, could we at least do a manual two-way commit?

Deleting anything is always extremely dangerous...

@dumblob
Copy link
Member

dumblob commented Apr 21, 2023

Got a crazy idea now. If this functionality is important for the end user (for me personally it actually is important), then why not to separate attachments from the email immediately after it was received but just before putting it into INBOX. Maybe by having a special filter or whatever. Then presenting it as one email (perhaps this is impossible and IMAP does not support anything like "merge 2 emails on the go" nor anything like "merge an email with some attachments on the go").

This way the received timestamp, IMAP ordering, etc. everything shall be "preserved". Deleting attachments at any time later would be a trivial operation not affecting the message itself.

Thoughts from IMAP experts?

@kroky
Copy link
Member

kroky commented Apr 24, 2023

The actual deletion consists of 3 steps done synchronously:

  1. Remove attachment from the text representation of the message.
  2. Create a new message on the imap server.
  3. Remove the old message.

This means that data loss can't occur from the IMAP operations. If an op fails (like number 3), the worst case scenario is leaving 2 mails in the mailbox.

I think data loss could occur from improper search/replace operations on the message itself and thus I proposed to use a MIME message lib to handle the message decomposition and composition better.

Another potential problem is the creation of the message on step 2 might actually change the date of the message as IMAP stores custom creation datetime besides the Date coming from message headers. Not sure if we can modify that creation date to be the same as the original message.

@josaphatim
Copy link
Member Author

The actual deletion consists of 3 steps done synchronously:

  1. Remove attachment from the text representation of the message.
  2. Create a new message on the imap server.
  3. Remove the old message.

This means that data loss can't occur from the IMAP operations. If an op fails (like number 3), the worst case scenario is leaving 2 mails in the mailbox.

I think data loss could occur from improper search/replace operations on the message itself and thus I proposed to use a MIME message lib to handle the message decomposition and composition better.

Another potential problem is the creation of the message on step 2 might actually change the date of the message as IMAP stores custom creation datetime besides the Date coming from message headers. Not sure if we can modify that creation date to be the same as the original message.

Thanks for that @kroky. I used https://github.com/zbateson/mail-mime-parser to remove attachment.

@kroky
Copy link
Member

kroky commented May 17, 2023

Looks good to me now, thanks @josaphatim !

modules/imap/functions.php Outdated Show resolved Hide resolved
@kroky
Copy link
Member

kroky commented Jun 23, 2023

@josaphatim can you please resolve the conflict as well, so we can merge?

@josaphatim
Copy link
Member Author

@josaphatim can you please resolve the conflict as well, so we can merge?

Hello @kroky. Conflicts fixed.

@josaphatim josaphatim force-pushed the delete-att branch 2 times, most recently from e6bd8c0 to 7b74882 Compare July 6, 2023 14:14
@kroky kroky merged commit 3057ede into cypht-org:master Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete attachments (but keep email)
5 participants