-
Notifications
You must be signed in to change notification settings - Fork 820
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.
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
NEW Move to SwiftMailer powered Emails #6466
Conversation
6bc80bc
to
b1a9e4c
Compare
} | ||
|
||
$message->render(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do devs have to call this manually now? Shouldn’t this just be called by send()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
render
is the replacement of populateTemplate
which had to be called to "render" the email in v3.
So yes, devs do have to call this themselves if they don't use the callback method for creating emails, but this is not a change in behaviour IMO.
The problem with send
calling render is what if a dev has manually set the body of the email? it'll be overridden and it'd be impossible to send a manually constructed email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about if we just check if the body is empty in render()
? It looks like the old implementation would’ve overridden the message body: https://github.com/dhensby/silverstripe-framework/blob/4b7d31377bf8e4e5415da8dd9b5f172967a407a9/src/Control/Email/Email.php#L620-L624 and https://github.com/dhensby/silverstripe-framework/blob/4b7d31377bf8e4e5415da8dd9b5f172967a407a9/src/Control/Email/Email.php#L494-L526
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting - we could check if the body is populated or not, though I do like the certainty of basically saying "you have to render your email" over the ambiguity of it...
$swiftMessage->setDate(DBDatetime::now()->Format('U')); | ||
if ($defaultFrom = $this->config()->admin_email) { | ||
$swiftMessage->setFrom($defaultFrom); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won’t this mean that if you use this method, you’re forced to use admin_email
(if set)? i.e. if I set a default from address, how can I override it on a per-email basis? I think this needs to check if a from address has already been set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I'll update this
b1a9e4c
to
944a0a7
Compare
use SilverStripe\Core\Object; | ||
use Swift_Mailer; | ||
use Swift_MailTransport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s a lot of Swift-specific code in this class, which shouldn’t really be inherited by modules that provide other mailers. Would it be better to make Mailer
an interface?
interface Mailer
{
public function send($message);
}
class SwiftMailer implements Mailer
{
public function send($message)
{
$swiftMessage = $message->getSwiftMessage();
return $this->sendSwift($swiftMessage);
}
// ... other methods
}
// Email.php
Injector::inst()->get('SilverStripe\Email\Mailer')->send($this);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I've taken a bit of a look at how Laravel does it and it's quite closely coupled - mostly because devs can write their own transports (which is basically the bulk of what they'd probably need to do).
There are a lot of out of the box transports, too, which would likely cover most of what people need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second @kinglozzer's suggestion, however I don't think it's critical to fully-decouple from swiftmailer for the sake of the initial implementation. It's not a merge blocker. :)
So I’ve just had a look at SwiftMailer in a bit more detail (I’d never used it before), and I’m guessing the idea here is that you’d write or use a transporter for Swift rather than your own Edit: doesn’t look hard, most of the big names already exist: |
Yep - pretty much - SwiftMailer is pretty popular and there are transports for all the major mail services. |
944a0a7
to
1a75b8e
Compare
I've addressed the 2 issues and pushed up |
04e7ca0
to
7d56db1
Compare
like to use a more robust transport to send mail you can swap out the transport used by the `Mailer` via config: | ||
|
||
```yml | ||
Mailer: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs the full namespace: SilverStripe\Control\Email\Mailer:
if ($body) { | ||
$result .= "\n$body"; | ||
if (!$this->swift) { | ||
$this->setSwiftMailer(new \Swift_Mailer($this->getTransport())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could inject this with dependencies, instead of via PHP.
Injector:
Swift_Transport: Swift_MailTransport
Swift_Mailer:
constructor:
- '%$Swift_Transport'
SilverStripe\Control\Email\Mailer:
properties:
SwiftMailer: '%$Swift_Mailer'
How about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. This approach would be in keeping with what we've done for assets and logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition the plugins could be provided to Swift_Mailer with the Injector's 'calls' option.
I think that SilverStripe\Control\Email\Mailer could be an interface, with the current impelmentation called SilverStripe\Control\Email\SwiftMailer
or something.
The interface would probably be very simple:
namespace SilverStripe\Control\Email;
interface Mailer {
public function send(Email $message);
public function getFailedRecipients();
}
That being the case, rather than Mailer::get_inst()
we'd probably want to use Injector::inst()->get(Mailer::class)
.
Alternatively, we could do away with Mailer altogether and use Injector to provide a Swift_Mailer
class? We can use the yaml config to add the transport and plugins.
For test mailing we could make a test transport instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could do away with Mailer altogether and use Injector to provide a Swift_Mailer class? We can use the yaml config to add the transport and plugins.
The problem with this is we can't send Email
objects directly to the Mailer
for sending as we can now.
We'd basically just be implementing raw SwiftMailer
without any convenience methods.
use SilverStripe\Core\Object; | ||
use Swift_Mailer; | ||
use Swift_MailTransport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second @kinglozzer's suggestion, however I don't think it's critical to fully-decouple from swiftmailer for the sake of the initial implementation. It's not a merge blocker. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review completed. :) Good so far overall.
* @param string $fullBody Prepared message | ||
* @param array $headers Prepared headers | ||
* @return mixed Return false if failure, or list of arguments if success | ||
* @return mixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use @return static
or @return Mailer
for correct type hinting. :)
*/ | ||
protected $ss_template = 'GenericEmail'; | ||
private $template = "SilverStripe\\Email\\Email"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use the full qualified path for the template? The convention is that it should match the same class name.
I.e.
private static $template = 'SilverStripe\Control\Email\Email
, or just private static $template = self::class
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, private $template
not private static $template
in this case. :)
|
||
// Rewrite relative URLs | ||
$this->body = HTTP::absoluteURLs($fullBody); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very easy to miss, but we need to ensure we still do this somewhere. Perhaps convert absoluteURLs() in Email::render() ? Relative links won't work in email content once sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phew - good spot. added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I've moved it to setBody
so we always do this on the body regardless of render. However, maybe the Mailer
or even SwiftPlugin
should do it to absolutely make sure nothing can get through?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep good job.
|
||
// Rewrite relative URLs | ||
$this->body = HTTP::absoluteURLs($fullBody); | ||
public function addAttachmentFromData($data, $name, $mime = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPDoc missing. :)
} | ||
|
||
/** | ||
* @return string|null | ||
* @return array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be @return string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it returns an array see my test testGetSetSender
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I look at SimpleMessage::getSender(), it says @return string
, so something is wrong here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess SimpleMessage::getSender() is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so!
* and it won't be plain email :) | ||
* | ||
* @param bool $isPlain | ||
* @param string $path Path to file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please phpdoc $alias and $mime arguments too.
if ($this->bcc) { | ||
$headers['Bcc'] = $this->bcc; | ||
} | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a good description. Also please document the method signature for the $callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thoughts, see my below comment.
* @param callable|null $callback | ||
* @return static | ||
*/ | ||
public static function create_from_callback($template, $data, $callback = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this is valuable; All it does is wrap Email::create()->setArg()->setArg()? All of these setters return $this, so there's nothing preventing them from being chained. I think we should just drop this method altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - originally it was a nice convenience method which would call render
for you (it had been mandatory to call render
by hand otherwise) but after @kinglozzer's suggestion of checking if the body is empty, this may be less desirable...
/** | ||
* Send the message to the recipients | ||
* | ||
* @return array|bool true if successful or array of failed recipients |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be more concise to have a simple boolean response, and document that the user can call $message->getFailedRecipients() to get the list of failed recipients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is some recipients could be successful and some rejected which means that a true/false return value is misleading. If one recipient is rejected then we should return false, but that doesn't mean a complete failure and so more debugging is needed.
I suppose that just means better documentation is required...
// Send prepared message | ||
return $this->sendPreparedMessage($to, $from, $subject, $attachedFiles, $customHeaders, $fullBody, $headers); | ||
} | ||
private $failedRecipients = array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the service should be storing data related to specific invocations of Mailer::sendSwift(); I suggest to pass this back to Email::send() and have it stored on the email instance instead. Otherwise it dilutes the role of Mailer as a service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good, but I don't think we need the Mailer class. Let's just get Injector to build us a Swift_Mailer object and have Email.php request that service from Injector.
@@ -953,17 +954,16 @@ public function onBeforeWrite() | |||
|
|||
// We don't send emails out on dev/tests sites to prevent accidentally spamming users. | |||
// However, if TestMailer is in use this isn't a risk. | |||
if ((Director::isLive() || Email::mailer() instanceof TestMailer) | |||
if ((Director::isLive() || Mailer::get_inst() instanceof TestMailer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to rely on Injector
for these global services?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to get rid of these references completely. Are tests not run under "live" mode?
@@ -15,513 +13,124 @@ class Mailer extends Object | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well remove 'extends Object' while we're tidying up this class.
if ($body) { | ||
$result .= "\n$body"; | ||
if (!$this->swift) { | ||
$this->setSwiftMailer(new \Swift_Mailer($this->getTransport())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. This approach would be in keeping with what we've done for assets and logging.
|
||
or in PHP | ||
|
||
```php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't encourage the use of PHP-based configuration I think.
$subject .= " [from $from]"; | ||
} | ||
$from = $sendAllfrom; | ||
$numFailed = Mailer::send_message($this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would change this to Injector::inst()->get(Mailer::class)->send($this)
if Mailer is to become an interface.
$from = $sendAllfrom; | ||
$numFailed = Mailer::send_message($this); | ||
if ($numFailed) { | ||
return Mailer::get_inst()->getFailedRecipients(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would change this to Injector::inst()->get(Mailer::class)->getFailedRecipients()
if Mailer is to become an interface.
if ($body) { | ||
$result .= "\n$body"; | ||
if (!$this->swift) { | ||
$this->setSwiftMailer(new \Swift_Mailer($this->getTransport())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition the plugins could be provided to Swift_Mailer with the Injector's 'calls' option.
I think that SilverStripe\Control\Email\Mailer could be an interface, with the current impelmentation called SilverStripe\Control\Email\SwiftMailer
or something.
The interface would probably be very simple:
namespace SilverStripe\Control\Email;
interface Mailer {
public function send(Email $message);
public function getFailedRecipients();
}
That being the case, rather than Mailer::get_inst()
we'd probably want to use Injector::inst()->get(Mailer::class)
.
Alternatively, we could do away with Mailer altogether and use Injector to provide a Swift_Mailer
class? We can use the yaml config to add the transport and plugins.
For test mailing we could make a test transport instead?
7fe018f
to
8b51bb7
Compare
I've implemented pretty much all the feedback.
How could we do this and allow devs to specify many plugins that should be loaded? This could work for our case, but doesn't provide a nice way for devs to add extra plugins: SilverStripe\Core\Injector\Injector:
Swift_Plugin: SilverStripe\Control\Email\SwiftPlugin
Swift_Mailer:
calls:
SwiftPlugin:
- registerPlugin
- [ '%$Swift_Plugin' ] todo:
|
f01662c
to
bab27ba
Compare
Looks like https://github.com/silverstripe/silverstripe-behat-extension's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - I noted a couple of method on SwiftMailer that could be removed / protected, and suggested a different yml style recommendation for docs about changing the transport.
In addition, I think we need a mention of this on the upgrade guide. We should also note that $Body in GenericEmail has been renamed to $EmailContent.
/** | ||
* @return static | ||
*/ | ||
public static function get_inst() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this.
* @param Email $message | ||
* @return int | ||
*/ | ||
public static function send_message($message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this.
* @param Swift_Message $message | ||
* @return int | ||
*/ | ||
public function sendSwift($message, &$failedRecipients = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be tidier if this was protected because it's not part of the interface-provided API. If people start calling it directly it becomes harder for another implementation of the Mailer interface to be dropped in as a replacement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
|
||
```yml | ||
SilverStripe\Control\Email\Mailer: | ||
swift_transport: Swift_SendmailTransport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to recommend that developers override the Swift_Transport
service instead? Otherwise the resulting service definition list will include a service called "Swift_Transport"' that's not actually the swift transport being used...
Swift_Transport:
class: Swift_SendmailTransport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll amend
@dhensby if you can rebase to the latest master and fix up src/Dev/TestMailer.php, behat should Just Work(tm) |
a12b5ba
to
d5127af
Compare
constructor: | ||
- '%$Swift_Transport' | ||
calls: | ||
SwfitPlugin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: SwiftPlugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo still here in your last push :)
'mimetype' => $mimeType, | ||
); | ||
$swiftMessage->setDate(DBDatetime::now()->Format('U')); | ||
if (!$swiftMessage->getFrom() && $defaultFrom = $this->config()->admin_email) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operator precedence; Should the assignment =
be surrounded with braces, since it's lower than the &&
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the assignment is the last condition it won't be messed up an assign a boolean value, however I'll wrap it in parentheses for clarity.
} | ||
|
||
/** | ||
* @return string|null | ||
* @return array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I look at SimpleMessage::getSender(), it says @return string
, so something is wrong here.
} | ||
|
||
/** | ||
* @return string|null | ||
* @return array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess SimpleMessage::getSender() is wrong?
* Mailer objects are responsible for actually sending emails. | ||
* The default Mailer class will use PHP's mail() function. | ||
*/ | ||
class SwiftMailer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot implements Mailer
interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
|
||
return $emailAddress; | ||
} | ||
public function getFailedRecipients(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this going to be moved to Email? SwiftMailer
doesn't implement this, so we can just remove this I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep - removed
* @param Swift_Message $message | ||
* @return int | ||
*/ | ||
public function sendSwift($message, &$failedRecipients = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing PHPDoc.
public function setSwiftMailer($swift) | ||
{ | ||
// register any required plugins | ||
foreach ($this->config()->get('swift_plugins') as $plugin) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a simple setter. Use YML injection to set these plugins on the injected $swift object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is how does that let site developers add 1 more plug-in in addition to what's currently existing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's good enough I guess.
$this->getSwiftMessage()->setContentType('text/plain'); | ||
if (!$this->getBody()) { | ||
$this->render(); | ||
$this->setBody(Convert::xml2raw($this->getBody())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks super risky, especially if you are sending an email multiple times, you are at risk of double-encoding.
Why not have separate messages for plain / html (with separate setBody / setPlainBody) and allow them to be set independently? You can always convert between the two if attempting to send one which is blank. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use cases:
- Send a HTML message with a html template, I want the system to automatically pick a plain text version for me (as current implementation)
- Send a HTML message with a html template, and separate plain text string provided for plain text version
- Sending a plain text message only with a setPlainBody().
- Sending a html and plain message, I will provide setBody() / setPlainBody() for each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed. Perhaps there should be a getPlainBody()
/ setPlainBody()
pair, and this transformation is called is PlainBody isn't set explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively getPlainTextSwiftMessage()
could be a second getter that transforms the email prior to passing it to the mailers. This, however, would mean that the mailer would need to accept a swift message object rather than our email. I don't have a problem with that, but it's another tweak to the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the Convert::xml2plain
is inside the if (!$this->getBody())
block then there's no risk of double encoding because we render and then take a copy we don't repeatedly set the plain part.
Adding the plain part automatically is all round risky. I don't like that the send
method does anything other than send the email (it tries to guess if it should be rendered and then attempts to render a plain part too).
In Laravel you have to set the plain part explicitly and can even define a template for the plain version - perhaps we should do something similar.
We could have something like.
$email = Email::create()
->setHTMLTemplate('MyTemplate')
->setPlainTemplate('MyTemplate_plain')
->send(); //still performs a render
Or if there's no plain template we create a generatePlainPart
method that does what we have at the moment.
I think the biggest challenge is with reusing Email
objects because I'm not so sure how easy it is to detach the previously generated plain part... I'll look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have code in HTMLText that generates plain text from HTML if you want to re-use that. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* Get plain-text version
*
* @return string
*/
public function Plain()
{
// Preserve line breaks
$text = preg_replace('/\<br(\s*)?\/?\>/i', "\n", $this->RAW());
// Convert paragraph breaks to multi-lines
$text = preg_replace('/\<\/p\>/i', "\n\n", $text);
// Strip out HTML tags
$text = strip_tags($text);
// Implode >3 consecutive linebreaks into 2
$text = preg_replace('~(\R){2,}~', "\n\n", $text);
// Decode HTML entities back to plain text
return trim(Convert::xml2raw($text));
}
91f7172
to
b1482fb
Compare
constructor: | ||
- '%$Swift_Transport' | ||
calls: | ||
SwfitPlugin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo still here in your last push :)
b1482fb
to
d908c76
Compare
d908c76
to
902fb72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TestMailer::saveEmail() API change isn't a blocker but it will require another patch to behat-extension.
]); | ||
public function send($message) | ||
{ | ||
$this->saveEmail($message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have to change the API for saveEmail? Now the behat test mailer is broken again :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
An email isn't really suitably represented by the old parameters and I thought best that we just store the email object instead...
This test suite is passing - where are the failures coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$state->emails[] = array_filter($data);
in TestMailer::saveEmail() in behat extension. array_filter will fail on an Email instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestSession state needs to be serialized, which is the main reason these were originally array format. I think maybe we should revert this back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you thinks best :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if they rely on some of the specific data (like Type
) then behat tests will need changing because that's not really a thing anymore.... or we'll need to have a "best guess" at type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of my guesses are the best.
@sminnee I've pushed a change to revert api changes to TestMailer. |
Happy to merge once CI passes. |
OK I'm happy with this. Say the word and I'll squash & merge |
This PR rewrites our
Mailer
andEmail
classes to be powered by SwiftMailer.Email
now acts as a proxy forSwift_Message
and adds our framework's rendering over the top.Mailer
converts anEmail
object into aSwift_Message
and sends it usingSwift_Mailer
using theSwift_MailTransport
by default (php'smail
function) just to keep things the same as they are now.todo: