NEW Migrate from swiftmailer/swiftmailer to symfony/mailer

[emteknetnz]

Issue #10097

[emteknetnz]

We're using still using original Symfony Mailer, which is a final class

Another option was to create own class that implemented MailerInterface, however I think it's better to be more strict and use the official Symfony class as the foundation here, as it's much more likely to "just work" when using services beyond just regular sendmail e.g. email being sent on an async queue

We used to customise the headers on the email sent with a SwiftPlugin - https://github.com/silverstripe/silverstripe-framework/blob/4/src/Control/Email/SwiftPlugin.php - however the "plugin" architecture no longer exists

To replicate what the the plugin used to do, we now use a new customer class MailerSubscriber which uses the Symfony EventDispatcher architecture

However in order to use the EventDispatcher, Mailer demands that we also supply a MessageBus

The MessageBus itself is pretty complicated to create and is largely responsible for the somewhat complicated yml here

However in the end, despite the complication here, I think this is good as it makes it easier for anyone else to copy paste this yml into their own project and use it as a basis for setting up the various symfony classes to support complex email setups

[GuySartorelli]

Need to remove src/Control/Email/SwiftMailer.php and src/Control/Email/SwiftPlugin.php - along with potentially other swiftmailer-related files.

We also need a new PR for whichever module provides the default _config/email.yml file to either update or remove that file.

[GuySartorelli]

When trying to trigger email from the lost password form I get the following error:

[Emergency] Uncaught Exception: Serialization of 'Closure' is not allowed
POST /Security/lostpassword/lostPasswordForm/

Line 527 in /var/www/vendor/silverstripe/framework/src/Control/Email/Email.php
Source

518         if ($htmlRender) {
519             $this->html($htmlRender);
520         }
521 
522         $this->dataAtLastRender = $this->getData();#clone $this->data;
523     }
524 
525     private function getDataHasChangedSinceLastRender(): bool
526     {
527         return serialize($this->getData()) !== serialize($this->dataAtLastRender);
528     }
529 }

Trace

    serialize(SilverStripe\Security\Member)
    Email.php:527
    SilverStripe\Control\Email\Email->getDataHasChangedSinceLastRender()
    Email.php:471
    SilverStripe\Control\Email\Email->updateHtmlAndTextWithRenderedTemplates()
    Email.php:433
    SilverStripe\Control\Email\Email->send()
    LostPasswordHandler.php:247
    
    ...

In case it's relevant, my mailhog config is:

---
Name: mailhog-emailconfig-cms5
Only:
  classexists: SilverStripe\Control\Email\EmailSender
After: '#mailer'
---
SilverStripe\Core\Injector\Injector:
  Symfony\Component\Mailer\Transport\TransportInterface:
    constructor:
      dsn: 'sendmail://default?command=/home/www-data/go/bin/mhsendmail%20-t'

Similarly, if I try to run

$email = new Email(
    '[email protected]',
    '[email protected]',
    'My subject',
    'My body'
);
$email->send();

I get the following:

ERROR [Emergency]: Uncaught Symfony\Component\Mailer\Exception\TransportException: Expected response code "220" but got empty code.
IN GET /dev
Line 316 in /var/www/vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php

Source
======
  307:         }
  308: 
  309:         [$code] = sscanf($response, '%3d');
  310:         $valid = \in_array($code, $codes);
  311: 
  312:         if (!$valid || !$response) {
  313:             $codeStr = $code ? sprintf('code "%s"', $code) : 'empty code';
  314:             $responseStr = $response ? sprintf(', with message "%s"', trim($response)) : '';
  315: 
* 316:             throw new TransportException(sprintf('Expected response code "%s" but got ',
       implode('/', $codes)).$codeStr.$responseStr.'.', $code ?: 0);
  317:         }
  318:     }
  319: 
  320:     private function getFullResponse(): string
  321:     {
  322:         $response = '';

Trace
=====
Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->assertResponseCode(, Array)
SmtpTransport.php:254

Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->start()
SmtpTransport.php:192

Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->doSend(Symfony\Component\Mailer\SentMessage)
AbstractTransport.php:68

Symfony\Component\Mailer\Transport\AbstractTransport->send(Symfony\Component\Mailer\SentMessage, Symfony\Component\Mailer\DelayedEnvelope)
SmtpTransport.php:136

Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->send(SilverStripe\Control\Email\Email, )
SendmailTransport.php:74

Symfony\Component\Mailer\Transport\SendmailTransport->send(SilverStripe\Control\Email\Email)
EmailSender.php:22

SilverStripe\Control\Email\EmailSender->send(Symfony\Component\Messenger\Envelope)
SendMessageMiddleware.php:66

Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle(Symfony\Component\Messenger\Envelope, Symfony\Component\Messenger\Middleware\StackMiddleware)
MessageBus.php:73

Symfony\Component\Messenger\MessageBus->dispatch(Symfony\Component\Mailer\Messenger\SendEmailMessage)
Mailer.php:60

Symfony\Component\Mailer\Mailer->send(SilverStripe\Control\Email\Email)
Email.php:434

SilverStripe\Control\Email\Email->send()
_config.php:18

...

[emteknetnz]

@GuySartorelli I've made a fix to the serializing of $this->getData() which should resolve the issue of closures not being serializable

Have removed 2x old Swift classes

Have created PR to remove email.yml from installer - silverstripe/silverstripe-installer#336 - have added to list of PRs on parent issue

Re your config - try deleting your custom email.yml and add this to your .env instead:
MAILER_DSN="sendmail://default?command=/home/www-data/go/bin/mhsendmail%20-t"

[GuySartorelli]

For future reference: The problem with my yaml config was I was using After: '#mailer' but I should have been using After: '#mailer-dsn-default-config'. The latter works fine, as does the .env config Steve provided.

[GuySartorelli]

My tests have revealed the following unexpected behaviour - though note that I was unable to get mailhog working for me with the old swiftmailer so I can't verify if these are existing behaviours or not.

Plain email differs in content from html email

Setup

Override the template to include some variable (in this case $Bananas).

<%-- the top and bottom of the template are ommitted from this example for brevity - they are unchanged. --%>
<div class="body">
    $EmailContent
    $Bananas
</div>

Use the following code to set and send the email:

$email = new Email(
    '[email protected]',
    '[email protected]',
    'My subject',
);
$email->setBody('<p>This is the body of the message.</p>');
$email->addData('Bananas', 'This is the banana');
$email->sendPlain();
$email->send();

Observed behaviour

This results in the html email having only the content "This is the body of the message." while the plain email has only the content "This is the banana". The plain text is being generated using the template, and the html email is being set using the body.

Expected behaviour

Because I am calling setBody() and not explicitly calling text() or html(), I would expect the content of both emails to be the same, with the only difference being that the plain email has no HTML markup. They should both be generated from the body.

It could be argued that using setPlainTemplate() should cause a difference like the one noticed above, but IMO setBody() should set both bodies, and the template should only ever be used in the following cases:

1. there is no body set at all, in which case both bodies should be generated from a template and data
2. only an html body is set using html() - in which case the plain email body should be generated from a template and data
3. only a plain body is set using text() - in which case the html body should be generated from a template and data.

In other words, the order of precedence for the html body would look like this:

1. a body set via either setBody() or html()
2. the html template and data.

and the order of precedence for the plain text body would look like this:

1. a body set via either setBody() or text()
2. the plain text template and data
3. the html template and data.

Links are not made absolute

Setup

In the body of an email, include a relative URL such as <a href="/about-us">This is the body of the message.</a>

Observed behaviour

The link remains relative

Expected behaviour

Links should be made absolute, so the above should become <a href="http://symfonymailer_06.local/about-us">This is the body of the message.</a> in my setup. This was done in CMS4 with HTTP::absoluteURLs($body) in setBody(). Probably that should be done as part of updateHtmlAndTextWithRenderedTemplates().

It looks like MailerSubscriber is meant to do this (but doesn't seem to be working). If it does stay there, we probably need an extension there or some other way for developers to affect the actual final content (used to just override setBody()).

SS_SEND_ALL_EMAILS_FROM and related config aren't being respected

SS_SEND_ALL_EMAILS_FROM being set in .env should result in all emails being sent from this address. My understanding from the docs is that this should override all config including the to email address set on Email - but it's just not taking effect at all.

This applies to the Email.send_all_emails_from config, as well as all of the other config in https://github.com/silverstripe/silverstripe-framework/blob/4/src/Control/Email/Email.php#L111-L149

It looks like MailerSubscriber is meant to do this but doesn't seem to be working.

setData() with sendPlain() affects send()

Setup

Create an email with a body, and make sure you call setData()

$email = Email::create(
    '[email protected]',
    '[email protected]',
    'My subject',
    'Some Body Here'
);
$email->setData([
   'EmailContent' => 'This is the content of the email',
]);
$email->sendPlain();
$email->send();

Send a copy with exactly the above, and then one with the sendPlain() call commented out.

Observed behaviour

If sendPlain() is called, the html email uses the body set in the constructor. Otherwise, it uses the html template and the set data.

Expected behaviour

The content of the HTML email should be the same regardless of whether sendPlain() is called first.

[GuySartorelli]

People won't realistically name their project config this. They'll have their own naming conventions for their config and probably won't realise that this is semi-important.
Probably After: '*' would be better if we want this to be used as the definitive override.

[emteknetnz]

Cannot update this to After: '*' - it creates a circular dependency when I flush

They can use whatever naming convention they want for their own file. MAILER_DSN .env variable won't work if projects use After: '*' in their project email config, however that's kind of on them for not following the instructions which say use After: 'mailer-dsn-default-config'

Also would imply they're attempting to use both custom .yml AND the MAILER_DSN .env variable at the same time, which you shouldn't do. If their production environment stops sending emails, probably one of the first things that gets check would be to see if project email config was committed to the repo

[GuySartorelli]

They can use whatever naming convention they want for their own file

Then what is mailer-dsn-project-config doing in this After:? That's not something we're adding in this PR, it's what the docs suggest as the name for the project config.

Either it isn't needed in this After: (in which case we should remove it from here) or it is needed here which means we need to make it really clear in the docs that this is an important name for their project-specific mailer config.

Cannot update this to After: '*' - it creates a circular dependency when I flush

I can't replicate that.... I've changed it to After: '*' locally with a fresh copy of the PR and it works perfectly. This makes sense - there is nothing saying After: 'mailer-dsn-env' because it's intended as a global override. So there is no circular dependency.

[emteknetnz]

I wasn't able to replicate a couple of these issues. I tested using the following PHP in PageController::init():

$email = new Email(
    '[email protected]',
    '[email protected]',
    'My subject',
    'My <strong>bold</strong> link to <a href="/about-us">about us</a>'
);
$email->cc('[email protected]');
$email->bcc('[email protected]');
$email->send();

Links are not made absolute

The email body was converted to absolute links i.e.
My <strong>bold</strong> link to <a href="http://bugfix5.test/about-us">

SS_SEND_ALL_EMAILS_FROM and related config aren't being respected

What I set my .env file to

SS_SEND_ALL_EMAILS_TO="[email protected]"
SS_SEND_ALL_EMAILS_FROM="[email protected]"
SS_CC_ALL_EMAILS_TO="[email protected],[email protected]"
SS_BCC_ALL_EMAILS_TO="[email protected],[email protected]

Will will correctly send a single email from [email protected] to [email protected] and set headers that include

Cc: [email protected],[email protected]
X-Original-Bcc: [email protected]
X-Original-Cc: [email protected]
X-Original-From: [email protected]
X-Original-To: [email protected]

Note: I didn't get the cc + bcc messages in my mailhog inbox, though that was the same as if I didn't include all the .env varaibles and just had the original $email->cc() and $email->bcc(), so that's probably a mailhog rather than a Silverstripe/Symfony thing

[emteknetnz]

I've refactored the code to no longer use the compare a serialized copy of getData() with the state of getData() at last render. This was fragile code that cause the issues with contents being different with calling sendPlain() following by send().

Instead there is now private bool $dataHasBeenSet = false; that is set to true when either setData() or addData() is called, essentially switched the render mode to "template" rather than the default "set the body directly via ->html()"

[GuySartorelli]

All of the issues I mentioned from my tests have been resolved.
The suggested priority order was not used - instead of setBody() taking precedence, the template takes precedence if any data is set. I think this is fine.

[GuySartorelli]

They can use whatever naming convention they want for their own file

Then what is mailer-dsn-project-config doing in this After:? That's not something we're adding in this PR, it's what the docs suggest as the name for the project config.

Either it isn't needed in this After: (in which case we should remove it from here) or it is needed here which means we need to make it really clear in the docs that this is an important name for their project-specific mailer config.

Cannot update this to After: '*' - it creates a circular dependency when I flush

I can't replicate that.... I've changed it to After: '*' locally with a fresh copy of the PR and it works perfectly. This makes sense - there is nothing saying After: 'mailer-dsn-env' because it's intended as a global override. So there is no circular dependency.

[emteknetnz]

@GuySartorelli replying here because github's not giving me the option to reply directly to a couple of your comments

... circular dependences with After: '*'

That's still happening on my local. Are you using silverstripe/installer or this PR standalone? The circular dependency comes from silverstripe/cms - _config/version.yml - After: framework/*

Though as noted above, we can probably get away with not have After: '*' and the env config still has precedence, provided the project yml contains After: 'mailer' which I think is a pretty clear instruction.

Worst case scenario is env config does haven't have precedence, though that's only a problem when
a) projects try to define 2 set of config (yml + env)
b) they didn't read the instructions

$html fallback

Have removed the param, you're correct, it is not needed

[GuySartorelli]

re: After: '*'

As discussed in person, it's feasible that the fragile way the order currently works could change, and this would no longer take precedence over project config.
I've created a PR to fix the circular dependency: silverstripe/silverstripe-cms#2793
If you merge that, we can use After: '*' to avoid this order breaking in the future unexpectedly.

[emteknetnz]

@GuySartorelli Have merged and changed to After: '*' - have tested locally that everything is working as expected