:bug: N°7916 SF#2274 EmailLaminas.php: Keep charset with part header in multipart email

[vlk-charles]

Base information

Question	Answer
Related to a SourceForge thead / Another PR / Combodo ticket?	SourceForge ticket 2274
Type of change?	Bug fix

Symptom

Since upgrading to iTop 3.1 (after switching to Laminas-mail, N°4307), we have been experiencing issues with character encoding with emails that contain attachments. More information is in the above-linked SourceForge issue.

Reproduction procedure

1. On iTop 3.1.1-1
2. With PHP 7.4.33
3. Create a script file emailtest.php in your iTop root directory:

   <?php
   require_once('approot.inc.php');
   
   $oEmail = new EMail();
   $oEmail->SetSubject('Email encoding test');
   $oEmail->SetRecipientFrom('[email protected]');
   $oEmail->SetRecipientTO('[email protected]');
   
   $oEmail->SetBody('Čeština s háčky a čárkami', 'text/plain');
   //$oEmail->AddAttachment('Attachment content', 'attachment.txt', 'text/plain');
   $oEmail->Send($unused);
   ?>
   

4. From your browser, navigate to the script to execute it, e.g. http://localhost/itop/emailtest.php.
5. See that the non-ASCII characters in the body are correct in the received email.
6. Uncomment the line with AddAttachment.
7. Execute the script again.
8. See that now the non-ASCII characters in the body are different.

Cause

Multipart content type is necessary to include attachments. The iTop code interacting with the Laminas-mail library does some unnecessary and counterproductive steps when trying to convert an email to a multipart one.

Proposed solution

Let the library handle conversion to multipart content type by removing blocks that test for $oBody->isMultiPart().

Checklist before requesting a review

• [x] I have performed a self-review of my code
• [x] I have tested all changes I made on an iTop instance
• [ ] I have added a unit test, otherwise I have explained why I couldn't
• [x] Is the PR clear and detailed enough so anyone can understand digging in the code?

Checklist of things to do before PR is ready to merge

[Molkobain]

PR looks great, but it would be nice to have a test case for that in the existing unit tests, or to give 2 eml files (actual / expected) so Combodo can help / make the test.

[steffunky]

Thanks for your contribution! I've logged your bug in our internal bug tracker under ref N°7916

[vlk-charles]

Not sure where to start with the unit tests but I think I can at least provide the before/after eml files.

[vlk-charles]

bugSF2274-singlepart-3.1.1-1.txt bugSF2274-multipart-3.1.1-1.txt bugSF2274-multipart-PR672.txt

The files have been anonymized a little. The relevant (without changes in timestamps and random strings) output from diff bugSF2274-multipart-{3.1.1-1,PR672}.txt:

16,17c16
< Content-Type: text/plain;
<  boundary="=_ad74c45415ef3ef6f6975298450e56ec"
---
> Content-Type: text/plain; charset=UTF-8

[steffunky]

Hey, I added a unit test, I will bring these bugs to the QA team when I have some time. Regarding the technical part, I'm ok with as the PR is right now but if you wish to make cleaner code with @Hipska feel free, I'll check once you are finished and approve this PR :grin:

[vlk-charles]

@steffunky If everyone is OK with it, can you please merge the request as it is? Personally I slightly prefer it over the suggestions.