🐛 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

• I have performed a self-review of my code
• 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
• 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

[Hipska]

What do you think of these suggestions? I think they will be clearer for the future..

[Hipska]

I would leave this line together with my following suggestion..

[Hipska]

Suggested change

	// setBody called only to refresh Content-Type to multipart/mixed
	$this->m_oMessage->setBody($this->m_oMessage->getBody()->addPart($oNewAttachment));
	$oBody->addPart($oNewAttachment);
	$this->m_oMessage->setBody($oBody);

[Hipska]

Similar as previous suggestion:

Suggested change

	// setBody called only to refresh Content-Type to multipart/mixed
	$this->m_oMessage->setBody($this->m_oMessage->getBody()->addPart($oNewPart));
	$oBody->$this->m_oMessage->getBody()
	$oBody->addPart($oNewPart)
	$this->m_oMessage->setBody($oBody);

[vlk-charles]

• First added line should be $oBody = $this->m_oMessage->getBody();.
• Second added line is missing a ;.
• First two lines can be combined to $oBody = $this->m_oMessage->getBody()->addPart($oNewPart);. The method returns the object. They call it "fluent interface". It is up to you, if you want 1, 2, or 3 lines. I am fine with either.
• I would keep the comment about why setBody is called at all. It is not obvious, because the previous addPart already modifies the messages body. What is being done is essentially $this->m_oMessage->setBody($this->m_oMessage->getBody());. The call is only necessary if the body was single-part previously. This ensures that the main Content-Type header gets updated accordingly (here). IMHO, this is a workaround in the library's deficiency.

Everything said applies to the first suggestion as well.

[Hipska]

I would keep the comment about why setBody is called at all. It is not obvious, because the previous addPart already modifies the messages body.

That's exactly why I suggested to break that apart to make it more clear 😉
I was indeed a bit too quick, here's a better suggestion:

Suggested change

	// setBody called only to refresh Content-Type to multipart/mixed
	$this->m_oMessage->setBody($this->m_oMessage->getBody()->addPart($oNewPart));
	$oBody = $this->m_oMessage->getBody();
	$oBody->addPart($oNewPart);
	$this->m_oMessage->setBody($oBody);

Could also be

Suggested change

	// setBody called only to refresh Content-Type to multipart/mixed
	$this->m_oMessage->setBody($this->m_oMessage->getBody()->addPart($oNewPart));
	$oBody = $this->m_oMessage->getBody()->addPart($oNewPart);
	$this->m_oMessage->setBody($oBody);

I thought the idea was clear enough without being lexically/syntaxically correct.

[vlk-charles]

Which one do you prefer?

[Hipska]

It's up to @Combodo team to decide, I was just suggesting. I'm a person from the community just like you.

PS: You can join us on Slack if you like.

[steffunky]

I have no preferences on this matter, I'll let others answer 😅

[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 😁

[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.