Proposal for #31: email component based on SwiftMailer

[klimov-paul]

Proposal for #31: email component based on SwiftMailer.

Migrated from #1011

[coveralls]

Coverage remained the same when pulling b611c42 on klimov-paul:email-swift-2 into 10553b6 on yiisoft:master.

[cebe]

what about putting the swift mailer into a yii extension and create a composer meta package for yii-mailer that is then provided by the desired backend?

[klimov-paul]

what about putting the swift mailer into a yii extension and create a composer meta package for yii-mailer that is then provided by the desired backend?

I do not know… I would like to have some email solution inside the core, not just as extension. But maybe this make sense.

[qiangxue]

This new design looks promising to me. Below are my feedback:

MessageInterface

• addText() and addHtml() should be removed. When both setText() and setHtml() are called, the message will use Html as the primary body content, and Text` as the alternative part.
• Methods such as setTo(), setFrom() probably should return static to support chaining operation.
• The name createAttachment() is a bit confusing as it sounds like you are creating an attachment object but not adding it to the mail. How about attachContent() or attachContentAsFile()? (similar to Response::sendFile() and Response::sendContentAsFile())
• attachFile() probably needs an $options parameter at the end so that users can set additional options for the attachment (e.g. disposition, encoding)
• Perhaps we should add embedding file method to the interface.
• We may consider adding toString() to the interface
• May consider adding getters for "to", "from", etc., for completeness (otherwise these properties would be write-only, which is not common).

BaseMailer

• I think ViewResolver should be dropped. Perhaps we should introduce an interface ViewResolverInterface (to "base") which should be implemented by classes (e.g. Controller, Widget) who want to support rendering a view using a name.
• Need to consider layout as it is common different HTML-based email templates sharing the same layout.
• _defaultMessageConfig can be renamed to messageConfig and made public (thus removing the getter/setter).
• Add createMessage() so that when creating a new message, we don't need to use new ClassName.

BaseMessage

• Need to reconsider render() method. Perhaps something like setTextByView()? This depends on how a message is being composed typically.

Swift Mailer and Message

• The above classes and interface may be put under /yii/mail.
• SwiftMailer-based mailer and message should be put under /extensions/swiftmailer as a core extension. Note that this does not mean the extension is a second-class citizen. The reason we package it as an extension is to make it easier to users to choose the mail solution and better maintain dependency. In our advanced and basic apps, we should list this extension is a requirement in their composer.json files.
• Remove autoloading-related code because we will rely on Composer to do this.

[klimov-paul]

MessageInterface

addText() and addHtml() should be removed. When both setText() and setHtml() are called, the message will use Html as the primary body content, and Text` as the alternative part.

Well, it has a problem… Swift_Message does not allow such behavior.
Setting body with different content types does not create “multipart alternative” content, instead latest set will override previous:

$message = new Swift_Message();
$message->setBody($text, 'text/plain');
$message->setBody($html, 'text/html'); // $html will override $text

If I use “Swift_Message::addPart()” another problems appear. If Message::setText() implementation will be following:

public function setText($text)
{
    $this->getSwiftMessage()->addPart($text, 'text/plain');
}

Multiple invokes of it will add multiple content parts:

$message = new Message();
$message->setBody($defaultText);
$message->setBody($overwriteText); // Not replaces previous, but adding one more part.

Also if “Swift_Message::addPart()” is invoked the message will be “multipart alternative” even if it contains only single part.
At the moment, I have no idea how I can resolve these problems.

Methods such as setTo(), setFrom() probably should return static to support chaining operation.

I have started with such interface, but abandoned it later. It is unlikely someone will use “chaining” style to setup message, probably people will use property interface for it: $message->from = $email

We may consider adding toString() to the interface

What it should return then?

May consider adding getters for "to", "from", etc., for completeness (otherwise these properties would be write-only, which is not common).

Do you want these be added to the interface? In this case it can become very heavy.

BaseMailer

I think ViewResolver should be dropped. Perhaps we should introduce an interface ViewResolverInterface (to "base") which should be implemented by classes (e.g. Controller, Widget) who want to support rendering a view using a name.

While developing this solution I have think about inconsistences around “view” layer. I think it should be named “ViewContextInterface”, which could handle view name resolving (if it does not has alias via ‘@’) as well as context rendering (see \yii\base\View::render()). I will open separated issue when (and if) I compose a solution for it.

Still I don’t want to drop “Mailer::viewResolver” property. I have separated this component to be able changing the way the email templates are searched without extending entire “Mailer” class.
For the CMS solutions the common practice is storing email templates inside the database, allowing administrator to edit them. At the current architecture I can compose my own “ViewResolver”, which can find records in DB and compose them into the PHP files, which then can be rendered (something similar to Smarty approach):

protected function resolveView($view)
{
    $fileName = Yii::getAlias(‘@app/runtime/email’) . ‘/’ . $view . ‘.php’;
    if (!file_exists($fileName)) {
        $emailPattern = EmailPattern::find()->…->one();
        file_put_contents($fileName, $emailPattern->content);
    }
    return $fileName;
}

Then I just need to configure the mailer property. If I want to switch to another mailer like Amazon SES, I can still keep my ViewResolver without extra efforts.

Need to consider layout as it is common different HTML-based email templates sharing the same layout.

At the moment we can always use “View::beginContent()”“View::endContent()”. I am not sure about layout: it makes sense for HTML content, but seems useless for subject.

Add createMessage() so that when creating a new message, we don't need to use new ClassName.

Actually at the moment Mailer component is not mandatory. You may develop a mail solution via MessageInterface, which will NOT require any external mailer. So method ‘Mailer::createMessage()’ seems redundant.

BaseMessage

Need to reconsider render() method. Perhaps something like setTextByView()? This depends on how a message is being composed typically.

I thought about this, but have to drop it, once I have realized “setBody” and “setText” have problems (see the first entry).

Swift Mailer and Message

The above classes and interface may be put under /yii/mail. SwiftMailer-based mailer and message should be put under /extensions/swiftmailer as a core extension.

What about unit tests? Where they should be placed?

[qiangxue]

Well, it has a problem… Swift_Message does not allow such behavior.
Setting body with different content types does not create “multipart alternative” content, instead latest set will override previous:

I see. I checked the implementation of both SwiftMailer and Zend Mail. It seems to me we should add addPart() to MessageInterface, and replace setText() and setHtml() with setBody(). What do you think?

Methods such as setTo(), setFrom() probably should return static to support chaining operation.

It's fine we don't support this.

May consider adding getters for "to", "from", etc., for completeness (otherwise these properties would be write-only, which is not common).

It's fine we don't add the getters. I just don't feel good when seeing write-only properties.

We may consider adding toString() to the interface

It returns a string representation of the message (may just call Swift_Message::toString()). It's useful when you want to quickly check the composed message during development.

For the CMS solutions the common practice is storing email templates inside the database, allowing administrator to edit them.

The same requirement may exist for non-email view templates. This can be supported using a DB-backed ViewRenderer.

Need to consider layout as it is common different HTML-based email templates sharing the same layout.

Adding beginContent(), endContent() to every view template has too much redundant code, and it is troublesome if you want to switch to a different layout. We may consider adding layout property to Mailer (in some sense Mailer is like a controller).

Need to reconsider render() method...

We still need to reconsider this because it doesn't fit very well in the process of composing a new message.

Actually at the moment Mailer component is not mandatory. You may develop a mail solution via MessageInterface, which will NOT require any external mailer. So method ‘Mailer::createMessage()’ seems redundant.

The createMessage() method is to decouple the application code from the actual mail implementation. You can certainly create a message by instantiating a new message object, but your code is attached to the actual message class.

What about unit tests? Where they should be placed?

Good question. Probably extensions/swiftmailer/tests.

[coveralls]

Coverage decreased (-0.02%) when pulling 73a8e77 on klimov-paul:email-swift-2 into 2a71c72 on yiisoft:master.

[coveralls]

Coverage decreased (-0.26%) when pulling 774c4db on klimov-paul:email-swift-2 into 155e9c6 on yiisoft:master.

[coveralls]

Coverage remained the same when pulling 4c27d88 on klimov-paul:email-swift-2 into 842971c on yiisoft:master.

[samdark]

@psihius your one-liner example isn't correct and can be rendered in a very ugly way in some clients such as old MS Outlook. Also if you'll send text/plain with HTML it will be still rendered as plain text. If you'll send HTML only some clients will not render it at all (can't find an example now but I've encountered such behavior).

[klimov-paul]

@psihius, you can setup single HTML body using following code:

Yii::$app->mail->message()
    ->from('from@domain')
    ->to('to@domain.com')
    ->subject('subject')
    ->html('Transaction <b>#XXXXX</b> was rejected...')
    ->send();

[psihius]

@samdark As I said, in some cases I just don't care - I know where it goes and will render fine no matter how I screw it up (well, except using some weird encoding) :)

@klimov-paul ahh.., ok. That solves it all. Thanks. No more questions on this part.

Although my brain just produced an interesting question: what happens if:

Yii::$app->mail->message()
    ->from('from@domain')
    ->to('to@domain.com')
    ->subject('subject')
    ->html('Transaction <b>#XXXXX</b> was rejected...')
    ->renderHtml('some/template')
    ->send();

or the html() and renderHtml are done in reverse order (same goes for text/renderText)? The latter call takes priority?

[klimov-paul]

@psihius, here is clarification:
There are 3 methods, which allows to setup message body directly from string without any render and any layout:

• MessageInterface::text($text) – setups plain text body
• MessageInterface::html($html) – setups HTML body
• MessageInterface::body($body) – setups HTML body as $body, and plain text body as strip_tags($body)

Then there are 3 matching render* methods, which fill message body by view render result, applying layout:

• MessageInterface::renderText($view, $params) – setups plain text body as render result
• MessageInterface::renderHtml($view, $params) – setups HTML body as render result
• MessageInterface::renderBody($view, $params) – setups HTML body as render result, and plain text body as strip_tags() on that render resut.

Any “render*” method is a wrapper around corresponding setter method, so last call always take precedence over previous

Yii::$app->mail->message()
    ->from('from@domain')
    ->to('to@domain.com')
    ->subject('subject')
    ->html('Transaction <b>#XXXXX</b> was rejected...')
    ->renderHtml('conatct', [...]) // override previous line
    ->send();

However if you set both “html” and “text” the message will become “multipart alternative” containing both plain text and HTML body:

Yii::$app->mail->message()
    ->from('from@domain')
    ->to('to@domain.com')
    ->subject('subject')
    ->html('Transaction <b>#XXXXX</b> was rejected...')
    ->text('You need HTML support client to view this message') // message becomes “multipart alternative”
    ->send();

[samdark]

Looks fine to me.

[qiangxue]

Another thing is that it should be possible somehow to render CSS bundles inline instead of linking to it.

You can just call $this->render($cssFile) inside your mail view. We may need a helper to support commonly used HTML rendering for emails (most email HTML need to use tables to do layout), but this is a separate task if needed.

@klimov-paul There are several problems with the latest design:

• Too many methods (6!) are exposed via MessageInterface just for body, which could cause confusion. MessageInterface should contain only the most basic methods to set various parts of an email. It should NOT be involved with view rendering.
• The name renderXyz() doesn't sound right because when I see this name, I would think the method would spit out something rather than setting an internal property.
• The name html() and text() are not self-explanatory. I'd like to see body($html, $text) or htmlBody()/textBody().
• The name message() doesn't sound right too because it is a noun. It's worse than compose(), createMessage() or create().
• When you use renderHtml() and renderText() to compose both HTML and TEXT body (a common practice), you have to duplicate the parameters twice, which requires you to prepare the parameters first.

I still think we should support $app->mail->compose($view, $params)->...->send(). When you can use chained method to explicitly set (helped by IDE) various parts of a mail, there is little benefit to do so via a configuration array. Also, it is almost certain that every mail needs to render its body through views (if not, you can always set body via the corresponding method), why not bringing it up to compose()? Other parts of mail are not all common. For example, you may need to set to but not from, or the other way around.

[cebe]

@samdark, your proposal requires instantiating separated view instance per each (!) mail message.
If all message rendering will use shared View, assets registered for one message will also applied to any other.

current implementation of View will unset all registered css, js and meta tags in endPage() method so it can not be reused.

[klimov-paul]

Too many methods (6!) are exposed via MessageInterface just for body, which could cause confusion.

Indeed 6 methods, as well as 2 different layouts – emails are not so simple as you may think. HTML body and plain text body are different values, each of them needs a rendering, so it is “*2”, then you insists on the ability to render both html and text at once, so we end up with 6 methods.

MessageInterface should contain only the most basic methods to set various parts of an email. It should NOT be involved with view rendering.

I always thought the “yii/mail” layer should do nothing about rendering views, but you have insistent it should. Now you are telling rendering inside “yii/mail” gone too far... Sorry I can’t feel the line, when it became this way.
I would prefer to create separated layer, which will allows message composition via rendering. It is like “yii/db” and “yii/db/ar”. We do not create models and relations at “yii/db” level, we create them at “yii/db/ar”. Same should be for the mail.

// Simple mail sending:
Yii::$app->message()
    ->from('from@domain.com')
    ->to('to@domain.com')
    ->subject('subject')
    ->text('content')
    ->send();

// Mail composition:
$mail = new ContactMail(['form' => $form]);
$mail->send();

The name renderXyz() doesn't sound right because when I see this name, I would think the method would spit out something rather than setting an internal property.

I can rename them to “composeXyz()”.

The name html() and text() are not self-explanatory. I'd like to see body($html, $text) or htmlBody()/textBody().

Syntax body($html, $text) is unacceptable: it will make setup of plain text body look ugly:

$message->body(null, $text);

At the beginning I was thinking about “htmlBody()/textBody()”, but looking at them I thought “Body” word is redundant. In email message context there could not be other ‘text’ or ‘html’.
Using your terms, I can say “ActiveRecord::find()” should be “ActiveRecord::findRecords()”.

The name message() doesn't sound right too because it is a noun. It's worse than compose(), createMessage() or create().

“message” is shorter name, which matches the method functionality. What about method “yii\base\Widget::widget()”? As far as I know “widget” is a noun, or it is not?

When you use renderHtml() and renderText() to compose both HTML and TEXT body (a common practice), you have to duplicate the parameters twice, which requires you to prepare the parameters first.

That is why I have created “renderBody()” – it allows to render both “html” and “text” views at once using the same parameters:

$message->renderBody(['text'=>'text/view', 'html'=>'html/view'], ['param1'=>'value1']);

At the moment solution provides flexible interface: you can render only HTML, only plain text or both, just as you wish.

Other parts of mail are not all common. For example, you may need to set to but not from, or the other way around.

And why is that?
If I am building mail message from ‘contact’ pattern, I already know it will be sent to application admin email.
If I am building mail from ‘forgotPassword’ pattern, I already know it will be sent to the user’s email address.

[samdark]

I would prefer to create separated layer, which will allows message composition via rendering.

Such as \yii\email\View? It makes sense but the fact that emails w/o templates are rare nowadays makes sense as well.

[klimov-paul]

emails w/o templates are rare nowadays makes sense as well.

Can you tell me then, why neither SwiftMailer nor ZendEmail do NOT provide mail template solution build in?

[psihius]

@klimov-paul I thought the answer to that question is obvious, no? Zend has loose coupling between components, so you have to render the template somehow, get it's rendered contents as a string and pass it to the mail component.
SwiftMail is just a mailer component, it assumes you just pass the body as string to it.

Yii was always different in that regard, it does not do decoupled components just for the sake of it, it provides an API that if you may need to replace the standard one, it's easy to do. So I do not see why would you even ask this question. Because it's comfortable that way to use out of the box.

UPD. Actually, I don't use Zend_Mail directly, I've created a helper, that has 2 public methods for sending e-mails: one as a view template, second as text and just pass params to it. Because, well, it's kind'a pita to write every time to render HTML body, then TEXT body, them set up all the headers, then select the proper transport based on the OS (Win/*nix), select the settings for the mailer for current project and finally send the e-mail :)

[samdark]

@klimov-paul both ZF and Symfony approach is to prefer theory (decoupling) over practice (simpler API where possible).

[samdark]

Personally I've got used to rendering views seaparately as I've used Zend Mailer w/ 1.1 but having API shortcut will be handy, I think.

[qiangxue]

Indeed 6 methods, as well as 2 different layouts – emails are not so simple as you may think...

It's not an excuse to say our API has to be complex because email is complex. In my proposal, we would only need two methods: one for setting body directly, the other for building body via views (in Mailer, not Message). This is very clear, I think.

I always thought the “yii/mail” layer should do nothing about rendering views...

There's some confusion here. I'm saying MessageInterface shouldn't need to deal with views, but MailerInterface can and should, in order to simplify email composition. We already have the View layer, and Mailer in some sense is like a controller which controls which view to render and populate data into it. Why do we need another layer specifically for rendering? In very special use case, you can always extend View to develop more features for the view layer, and there's also view renderer option.

Syntax body($html, $text) is unacceptable: it will make setup of plain text body look ugly

It's not totally unacceptable, if we all agree HTML body is needed in most cases. In rare cases when only text body is needed, such ugliness is tolerable, I think.

...Using your terms, I can say “ActiveRecord::find()” should be “ActiveRecord::findRecords()”.

I'd say Mailer::compose() is close to the spirit of ActiveRecord::find(). The method exists in a context. Message::html() doesn't convey if this is about body, as we know Message has many parts. But ActiveRecord::find() conveys we are finding ActiveRecord.

...What about method “yii\base\Widget::widget()”?

You have a point here, but there's a subtle difference between widget and mail. For mail, it is Mailer that creates a message, while for widget it is the widget class itself and it is used in a context of echoing. Another thing is that widget has the variant beginWidget()/endWidget() that we need to consider to match. This naming is not critical, but if we have better alternative, we should use the better one.

[klimov-paul]

Requested changes applied. Hopefully, I have not forgotten anything.

[coveralls]

Coverage increased (+0.73%) when pulling 104cfd7 on klimov-paul:email-swift-2 into 842971c on yiisoft:master.

[qiangxue]

no more renderText.

[qiangxue]

Looks great to me! Thank you for making the changes. You may go ahead and merge it.

[coveralls]

Coverage remained the same when pulling 9dbf4d7 on klimov-paul:email-swift-2 into 65d7ffa on yiisoft:master.

[qiangxue]

Thanks!