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.

Sign up for GitHub

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

Jump to bottom

[2.3] Transfer Encoding of emails changed to QUOTED-PRINTABLE #23649

Merged
merged 1 commit into from
Jul 18, 2019
Merged

[2.3] Transfer Encoding of emails changed to QUOTED-PRINTABLE #23649

merged 1 commit into from
Jul 18, 2019

Conversation

gwharton
Copy link
Contributor

@gwharton gwharton commented Jul 10, 2019 •
edited
Loading

Description (*)

When Magento used Zend Framework 1 for emails, MIME emails were sent with Transfer Encoding set to QUOTED-PRINTABLE. This means that emails with long lines (particularly emails with large inline css) are encoded properly. This can be confirmed by running tests under 2.2.7.

When Magento switched to Zend Framework 2, the default Transfer Encoding was set to 8-bit. This emails with long lines were being rejected by some mail servers as they were not encoded quoted-printable as expected.

Magento 2.2.8+ and 2.3.0+ are affected by this.

This PR changes the default encoding of all MIME emails to quoted-printable.

Fixed Issues (if relevant)

  1. Mime parts of email are no more encoded with quoted printable #23643: Mime parts of email are no more encoded with quoted printable

Manual testing scenarios (*)

Backend
=======
Marketing -> Email Templates -> Add New Template
    Template Name = Newsletter Success Plain
    Subject = Success Subject
    Template Content = Success Body
    Convert to Plain Text
    Save Template
Marketing -> Email Templates -> Add New Template
    Template Name = Newsletter Success Html
    Subject = Success Subject
    Template Content = <html><body>Success Body</body></html>
    Save Template
Store -> Configuration -> General -> Store Email Addresses
    General Contact
        Sender Name = Owner Name
        Sender Email = [email protected]
Store -> Configuration -> Customers -> Newsletter -> Subscription Options
    Success Email Template = Newsletter Success Plain

Frontend
========
Subscribe to newsletter

Ensure email received contains the following headers

MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Backend
=======
Store -> Configuration -> Customers -> Newsletter -> Subscription Options
    Success Email Template = Newsletter Success Html

Frontend
========
Subscribe to newsletter

Ensure email received contains the following headers

MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Jul 10, 2019

Hi @gwharton. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@gwharton gwharton mentioned this pull request Jul 10, 2019
Merged
4 tasks
pmclain
pmclain previously requested changes Jul 10, 2019
View reviewed changes
Copy link
Contributor

@pmclain pmclain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwharton Please review the integration test checks https://testing-service.magento-community.engineering/reports/magento/magento2/pull/23649/948900ad-78aa-453d-b0f1-ee38478a911e/11125/Integration/allure-report-ce/index.html

@ghost ghost assigned pmclain Jul 10, 2019
@pmclain
Copy link
Contributor

pmclain commented Jul 10, 2019

I'm also struggling to find where earlier versions used quoted-printable for the message body. The magento fork for ZF1 looks to have the same default as what is currently used.

ZF1 was using quoted-printable for encoding headers and that does seem to be missing from the current ZF2 implementation.

@gwharton
Copy link
Contributor Author

Take a look at the ZF1 Zend_Mail class here.

https://github.com/magento/zf1/blob/master/library/Zend/Mail.php#L362-L377

and

https://github.com/magento/zf1/blob/master/library/Zend/Mail.php#L404-L419

which are the setBodyText and setBodyHtml functions from ZF1. In Magento 2.2.7, /Magento/Framework/Mail/Message.php inherited from Zend_Mail.

With the move to ZF2, these two functions were effectively moved into /Magento/Framework/Mail/Message.php.

It could also be argued that Zend_Mime::DISPOSITION_INLINE should also be set, as that was also the default in ZF1.

@gwharton
Copy link
Contributor Author

With request to the test failures, I'm not at all sure what to do. The tests are failing because encoding the email with quoted-printable, results in an email which has the body split into multiple lines with CRLF at the end of each line. Thus tests that are looking for strings in the email body fail because in many cases the string that is being looked for has a CRLF inserted into it, if it spans a line boundary. I'll see if I can find out why these tests were passing under 2.2 which did encode emails in this way.

@gwharton
Copy link
Contributor Author

gwharton commented Jul 10, 2019 •
edited
Loading

OK, i've had to modify the failing integration tests to get them to pass. They were originally written to use the \Zend\Message::getRawMessage function to get the body of the email message, but this function returns the email body encoded. Thus now that the emails are changed to encoding quoted-printable, the raw message returned in the test is encoded and the tests fail.

The tests have been modified to use the \Zend\Mime\Part::getRawContent() instead which returns the email body unencoded, allowing the tests to pass.

Just waiting on the green lights from the automated tests.

Let me know if you think Zend_Mime::DISPOSITION_INLINE should also be applied, and whether you want me to do anything with the header encoding. Makes sense to get all these in together.

pmclain
pmclain reviewed Jul 11, 2019
View reviewed changes
Copy link
Contributor

@pmclain pmclain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's set the disposition to \Zend\Mime::DISPOSITION_INLINE. I took a deeper look at the ZF2 header encoding and they are already quoted-printable so we should be good there.

lib/internal/Magento/Framework/Mail/Message.php Show resolved Hide resolved
Change Encoding of MIME Emails to QUOTED-PRINTABLE
226009e 
Added Content-Disposition: inline to all MIME emails

Implementation now matches previous ZF1 implementation
@gwharton gwharton dismissed pmclain’s stale review July 11, 2019 07:42

Changes Integrated

@gwharton
Copy link
Contributor Author

Backport is here #23650

@magento-engcom-team
Copy link
Contributor

Hi @pmclain, thank you for the review.
ENGCOM-5433 has been created to process this Pull Request
✳️ @pmclain, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@pmclain pmclain added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Jul 12, 2019
@engcom-Delta engcom-Delta self-assigned this Jul 12, 2019
@engcom-Delta
Copy link
Contributor

✔️ QA passed

@engcom-Foxtrot engcom-Foxtrot self-assigned this Jul 12, 2019
@magento-engcom-team magento-engcom-team mentioned this pull request Jul 18, 2019
Closed
@magento-engcom-team magento-engcom-team merged commit 226009e into magento:2.3-develop Jul 18, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 18, 2019

Hi @gwharton, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team pushed a commit that referenced this pull request Jul 18, 2019
@sivaschenko
ENGCOM-5433: [2.3] Transfer Encoding of emails changed to QUOTED-PRIN…
6c96cb6 
…TABLE #23649
@magento-engcom-team magento-engcom-team added this to the Release: 2.3.3 milestone Jul 18, 2019
@gwharton gwharton deleted the 2.3-develop-quoted-printable branch July 19, 2019 09:48
This was referenced Sep 10, 2019
Closed
Closed
@hostep hostep mentioned this pull request Oct 26, 2019
Merged
4 tasks
jaywilliams added a commit to RevivalParts/magento2-postmark that referenced this pull request Apr 28, 2020
@jaywilliams
Fix Magento 2.3 Email Encoding Compatibility
2d1e4b7 
See Also magento/magento2#23649
@jaywilliams jaywilliams mentioned this pull request Apr 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmclain pmclain pmclain approved these changes

Assignees

@pmclain pmclain

@engcom-Delta engcom-Delta

@engcom-Foxtrot engcom-Foxtrot

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Component: Mail Progress: accept Release Line: 2.3
Projects
None yet
Milestone
Release: 2.3.3
Development

Successfully merging this pull request may close these issues.
5 participants
@gwharton @pmclain @magento-engcom-team @engcom-Delta @engcom-Foxtrot