Replace Zend_Mail (ZF1) with Zend\Mail (ZF2) by schmengler · Pull Request #8608 · magento/magento2

schmengler · 2017-02-19T23:19:56Z

This PR probably needs more work, at least more testing, I cannot believe that it is so easy.

Opening for review and acceptance tests. If it's working I plan to refactor the code a bit (see TODO comments) - feedback appreciated!

orlangur · 2017-02-21T17:22:18Z

needs more work, at least more testing, I cannot believe that it is so easy

True, true... One integration test is failing, another one caused fatal error :) Check Travis CI.

Note that refactoring should be performed in such a way that all existing customizations relying on Magento_Framework and not Zend_Mail directly work as before (just like integration tests, people will not be happy getting such errors after upgrade).

Will share my thoughts on the code a bit later.

schmengler · 2017-02-21T17:56:39Z

True, true... One integration test is failing, another one caused fatal error :) Check Travis CI.

I've seen it, but could not look into the details yet. I tried to take care not to change the Magento_Framework behavior, and it's possible that tests are only failing because mocks have to be changed.

I'm waiting for your feedback on the code now!

orlangur · 2017-02-22T06:24:17Z

+use Zend\Mime\Part;
+
+/**
+ * @todo composition instead of inheritance for better testability


Do it now and expose only necessary public methods.

Otherwise people may rely on Zend implementation rather then on narrowed interface and we will continue to have problems like Fatal Error with absent getBodyHtml.

orlangur · 2017-02-22T06:28:46Z

    public function __construct($charset = 'utf-8')
    {
-        parent::__construct($charset);
+        $this->encoding = $charset;


Rename constructor argument accordingly and use setEncoding method of \Zend\Mail\Message instead (currently $this->getHeaders()->setEncoding($encoding); is missed).

orlangur · 2017-02-22T06:30:06Z

     * @return $this
     */
-    public function setBody($body)
+    public function setMessageType($type)


Please preserve order of methods to simplify checking of correspondence during review (getBody -> getBody, setBody -> setBody etc).

orlangur · 2017-02-22T06:39:40Z

    {
-        return $this->messageType == self::TYPE_TEXT ? $this->getBodyText() : $this->getBodyHtml();
+        if (is_string($body) && $this->messageType === MessageInterface::TYPE_HTML) {
+            $body = self::htmlMimeFromString($body);


Method name must begin with verb, I suggest createHtmlMimeFromString.

orlangur · 2017-02-22T06:47:29Z

-    public function getBody()
+    public function setBody($body)
    {
-        return $this->messageType == self::TYPE_TEXT ? $this->getBodyText() : $this->getBodyHtml();


Calls of getBodyHtml must be fixed all over the code, this is why we saw Fatal Error on Travis. Integration tests do not use mocks usually.

orlangur · 2017-02-22T06:51:53Z

 namespace Magento\Framework\Mail;

-class Transport extends \Zend_Mail_Transport_Sendmail implements \Magento\Framework\Mail\TransportInterface
+


Useless new line and phpcs does not enforce it yet.

orlangur · 2017-02-22T06:56:41Z


-class Transport extends \Zend_Mail_Transport_Sendmail implements \Magento\Framework\Mail\TransportInterface
+
+class Transport extends \Zend\Mail\Transport\Sendmail implements \Magento\Framework\Mail\TransportInterface


Do aggregation instead of inheritance.

orlangur · 2017-02-22T06:58:41Z

    {
-        if (!$message instanceof \Zend_Mail) {
-            throw new \InvalidArgumentException('The message should be an instance of \Zend_Mail');
+        if (!$message instanceof \Zend\Mail\Message) {


I don't think we should keep classes inherited from Zend classes in the system.

Just create necessary instance of \Zend\Mail\Message in sendMessage method instead.

orlangur · 2017-02-22T07:06:37Z

+
+/**
+ * @todo composition instead of inheritance for better testability
+ * - add a ZendMailDecorator interface with getZendMail() method for usage in \Magento\Framework\Mail\Transport


Please no. To me it looks like \Zend\Mail\Message has all necessary setters already.

orlangur · 2017-02-22T07:08:26Z

+ * @todo composition instead of inheritance for better testability
+ * - add a ZendMailDecorator interface with getZendMail() method for usage in \Magento\Framework\Mail\Transport
+ * @todo get rid of temporal coupling (setMessageType() + setBody())
+ * - deprecate setMessageType(), implement a HtmlMessage decorator instead


I don't think such decorator would be really valuable. Do we have some needed cases not covered with current text/html types implementation?

The problem with the current implementation is that you have to call setMessageType() before setBody(). I'm now considering changing the MessageInterface interface. As it should be used via TransportBuilder, it should not have serious impact.

The current signatures setBody(mixed $body) and mixed getBody() are not meaningful. Either "mixed" must be replaced with "string" or we are leaking the zend abstraction.

The interface does not allow multitype mails (this is the needed case not covered. Something that Magento always has been lacking of)

One idea is, replacing setMessageType(), setBody() and getBody() with:

setBodyHtml(string $html)

setBodyText(string $text)

string getBodyText()

string getBodyHtml()

This allows text mails, HTML mails, and multitype mails, does not require calling them in any specific order and does not reveal anything of the underlying Zend mail component.

Another advantage is that this resembles the old zf1 implementation, so 3rd party code is actually less likely to break even though we have to change an @api interface

orlangur · 2017-02-22T07:12:35Z

@schmengler, thanks for your efforts, implementation in general looks quite clean and minimalistic (just does what is really necessary).

Please make sure all builds are green before next review iteration.

okorshenko · 2017-03-16T02:52:37Z

Hi @schmengler
Are you going to proceed this with PR?

schmengler · 2017-03-16T16:19:38Z

@okorshenko yes as soon as I find some time for it

okorshenko · 2017-03-17T14:45:59Z

@schmengler thank you. Please let us know when you will be ready. Thank you

Conflicts: composer.lock

schmengler · 2017-03-18T23:59:14Z

Because it's hidden now in the "outdated" review, here again my concerns about the message interface:

The problem with the current implementation is that you have to call setMessageType() before setBody(). I'm now considering changing the MessageInterface interface. As it should be used via TransportBuilder, it should not have serious impact.

The current signatures setBody(mixed $body) and mixed getBody() are not meaningful. Either "mixed" must be replaced with "string" or we are leaking the zend abstraction.
The interface does not allow multitype mails (this is the needed case not covered. Something that Magento always has been lacking of)

One idea is, replacing setMessageType(), setBody() and getBody() with:

setBodyHtml(string $html)
setBodyText(string $text)
string getBodyText()
string getBodyHtml()

This allows text mails, HTML mails, and multitype mails, does not require calling them in any specific order and does not reveal anything of the underlying Zend mail component.

Another advantage is that this resembles the old zf1 implementation, so 3rd party code is actually less likely to break even though we have to change an @api interface

I already added one method getRawMessage() because otherwise it would not be possible to have a Zend transport implementation by only using the message interface and not the underlying Zend message, so the PR will cause a BC break anyways. But IMHO it's a minor one and it is necessary to get the mail interfaces right.

schmengler · 2017-03-19T00:13:33Z

And while we are at it, shouldn't TransportInterface look like this:

    function sendMessage(MessageInterface $message);

instead of

    function sendMessage()

?

I don't understand, why the "Transport" implementation receives the message in the constructor. I'd rather have the transport instance reusable for different messages.

With the current implementation we have the TransportBuilder class, which actually builds a message. That would be much simpler and clearer if we did not couple the transport to the message like currently.

The transport now can take any implementation of the message interface as argument

- updated composer.lock file

okorshenko · 2017-07-10T22:38:23Z

Hi @schmengler
Just FYI: we stabilized this branch and working on the testing of new changes.

- Introduced new interface MailMessageInterface and deprecated MessageInterface to keep backward compatibility - get rid of temporal coupling (setMessageType() + setBody()) - deprecated setMessageType(), setBody() and getBody() - implemented setBodyHtml(), setBodyText(), getBodyHtml() and getBodyText() - changed usage in \Magento\Framework\Mail\Template\TransportBuilder::prepareMessage()

- removed methods getBodyHtml() and getBodyText(). Use method getBody()

- fixed unit tests

- fixed integration tests

- code review changes

0m3r · 2018-12-19T12:54:45Z

What about getHeaders and setHeaders for \Magento\Mail\Message?

I want to set headers like a in-reply-to, message-id, references etc.

klein0r · 2019-03-30T18:06:51Z

Is there a reason why this has been included into 2.2.8?

orlangur · 2019-04-01T23:41:51Z

@klein0r yep, backward compatibility is the reason.

…fix-10272023 Cia 2.4.7 beta3 develop bugfix 10272023

schmengler added 4 commits February 19, 2017 19:50

Install ZF 2 mail component

3309db2

Replace ZF1 Zend_Mail with ZF2 Zend\Mail

584a17a

Remove unused variable

ca1a3b2

Update unit tests for ZF 2 mail

d1be154

orlangur requested changes Feb 22, 2017

View reviewed changes

okorshenko self-assigned this Feb 26, 2017

okorshenko added this to the February 2017 milestone Feb 26, 2017

maksek modified the milestones: February 2017, March 2017 Mar 1, 2017

okorshenko added this to the March 2017 milestone Mar 1, 2017

okorshenko removed their assignment Mar 17, 2017

okorshenko removed this from the March 2017 milestone Mar 17, 2017

schmengler added 2 commits March 18, 2017 22:16

Merge branch 'develop' into zend-mail-2

0d2f2a6

Conflicts: composer.lock

Remove zend mail inheritance

c1d8e0d

schmengler and others added 4 commits March 19, 2017 00:26

Remove conflicting class imports

0be511c

Fix integration test and code style issues

00bc417

Remove redundant unit test

e02e28d

The transport now can take any implementation of the message interface as argument

Merge branch 'develop' into zend-mail-2

dd755bc

okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

ishakhsuvarov self-assigned this Jun 8, 2017

ishakhsuvarov added the Progress: needs update label Jun 8, 2017

okorshenko modified the milestones: June 2017, July 2017 Jul 2, 2017

Merge branch 'develop' into zend-mail-2

e7f62fe

okorshenko self-assigned this Jul 6, 2017

Oleksii Korshenko added 2 commits July 6, 2017 16:00

Merge branch 'develop' into zend-mail-2

b633cd3

MAGETWO-67680: Replace Zend_Mail (ZF1) with Zend\Mail (ZF2) #8608

b7186fc

- updated composer.lock file

okorshenko removed the Progress: needs update label Jul 10, 2017

okorshenko added the Progress: accept label Jul 10, 2017

magento-team merged commit b7186fc into magento:develop Jul 12, 2017

magento-team pushed a commit that referenced this pull request Jul 12, 2017

MAGETWO-67680: Replace Zend_Mail (ZF1) with Zend\Mail (ZF2) #8608

843be9b

- removed methods getBodyHtml() and getBodyText(). Use method getBody()

magento-team pushed a commit that referenced this pull request Jul 12, 2017

MAGETWO-67680: Replace Zend_Mail (ZF1) with Zend\Mail (ZF2) #8608

32f94ce

- fixed unit tests

magento-team pushed a commit that referenced this pull request Jul 12, 2017

MAGETWO-67680: Replace Zend_Mail (ZF1) with Zend\Mail (ZF2) #8608

212632f

- fixed integration tests

magento-team pushed a commit that referenced this pull request Jul 12, 2017

MAGETWO-67680: Replace Zend_Mail (ZF1) with Zend\Mail (ZF2) #8608

f570ebf

- code review changes

magento-team pushed a commit that referenced this pull request Jul 12, 2017

MAGETWO-67680: Replace Zend_Mail (ZF1) with Zend\Mail (ZF2) #8608

19d8405

mhauri mentioned this pull request Jun 2, 2018

Eliminate usage of Zend_Mail from Magento 2 Open Source magento-engcom/php-7.2-support#136

Open

4 tasks

0m3r mentioned this pull request Aug 20, 2018

Replace Zend_Mail (ZF1) with Zend\Mail\Message (ZF2) swissup/module-email#7

Closed

magento-devops-reposync-svc pushed a commit that referenced this pull request Mar 7, 2024

Merge pull request #8608 from magento-cia/cia-2.4.7-beta3-develop-bug…

f59f51e

…fix-10272023 Cia 2.4.7 beta3 develop bugfix 10272023

		namespace Magento\Framework\Mail;

		class Transport extends \Zend_Mail_Transport_Sendmail implements \Magento\Framework\Mail\TransportInterface


		class Transport extends \Zend_Mail_Transport_Sendmail implements \Magento\Framework\Mail\TransportInterface

		class Transport extends \Zend\Mail\Transport\Sendmail implements \Magento\Framework\Mail\TransportInterface

Conversation

schmengler commented Feb 19, 2017 • edited by okorshenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Feb 21, 2017

Uh oh!

schmengler commented Feb 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orlangur Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orlangur commented Feb 22, 2017

Uh oh!

okorshenko commented Mar 16, 2017

Uh oh!

schmengler commented Mar 16, 2017

Uh oh!

okorshenko commented Mar 17, 2017

Uh oh!

schmengler commented Mar 18, 2017

Uh oh!

schmengler commented Mar 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

okorshenko commented Jul 10, 2017

Uh oh!

0m3r commented Dec 19, 2018

Uh oh!

klein0r commented Mar 30, 2019

Uh oh!

orlangur commented Apr 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

schmengler commented Feb 19, 2017 •

edited by okorshenko

Loading

orlangur Feb 22, 2017 •

edited

Loading

schmengler commented Mar 19, 2017 •

edited

Loading