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.

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

Swiftmailer is end-of-life. Need to upgrade to Symfony Mailer #10097

Closed
1 task
brynwhyman opened this issue Sep 21, 2021 · 11 comments
Closed
1 task

Swiftmailer is end-of-life. Need to upgrade to Symfony Mailer #10097

brynwhyman opened this issue Sep 21, 2021 · 11 comments

Comments

@brynwhyman
Copy link
Contributor

brynwhyman commented Sep 21, 2021

Overview

Symfony recently announced that maintenance of Swiftmailer will officially end in November 2021 in favour of Symfony Mailer. See EOL announcement.

We'll need to investigate the migration path and set a plan for doing this work.

Acceptance criteria

  • CMS5 branch is using Symfony Mailer v6

Side note

  • Yes... Work just happened to upgrade to v6 of Swiftmailer for the CMS 4.9 release 🤦
  • The goal of the card is just to update to Symfony mailer however feel free to take extra time to refactor things along the way and pay down some technical debt.

PRs

@ScopeyNZ
Copy link
Contributor

Maybe we should take this opportunity to switch to an adapter pattern, and then it would theoretically be less of a problem with this situation in the future.

@maxime-rainville
Copy link
Contributor

Maybe we should take this opportunity to switch to an adapter pattern, and then it would theoretically be less of a problem with this situation in the future.

silverstripe/mailer FTW

@frankmullenger @danaenz How will this impact S/MIME?

@emteknetnz
Copy link
Member

emteknetnz commented Sep 22, 2021

Realistically we're only going to give proper to support to one mailer, almost certainly SymfonyMailer.

An adapter pattern sounds like it could be over-abstraction? I think the FullTextSearch module started off with similar thinking and just ended up as an over-abstraction for solr

To be fair, email is a lot simpler than search so extra-abstraction would be less painful.

Could make sense if we need to dual support both SwiftMailer v6 + SymfonyMailer? We must prevent email from breaking on upgrade for the same reasons we backported v5 MailTransport in our own codebase

Whatever we do, any existing project config for email is probably going to stop working cos the classnames change :-/

@ScopeyNZ
Copy link
Contributor

I don't think you can really ever classify an email adapter as over abstraction to be honest. It's abstraction for a completely valid reason.

I think the FullTextSearch module just failed to implement the adapter pattern, so ended up not actually being an abstraction at all, just a bunch of cruft around Solr.

You could get the side effect of being able to keep existing deprecated SwiftMailer code and also adding support for SymfonyMailer. You'd also get the benefits of simpler guidance around development environments (using something like MailTrap).

Other benefits include improved testability of Mail APIs. Abstraction of complicated per-mailer logic into their own adapters, potentially simplifying the interfaces.

Downside is that it might be hard to do without breaking changes, but I guess that's why Max is saying "silverstripe/mailer FTW".

@emteknetnz
Copy link
Member

emteknetnz commented Sep 22, 2021

Right now we've got Email.php which extends ... ViewableData :-)

We'll need to keep the API in Email.php as is since that's how you send Emails in SIlverstripe

Looks like it was there was some intention at some point to have the mailer swappable:

But right now Email.php is pretty hardcoded to swiftmailer

A)
If we go for mutli-mailer support (including swiftmailer v6) we'd probably have to do abstraction in Email.php and refactor the thing.

B)
If we go for "you must use SymfonyMailer" we'd need to refactor Email.php to use SymfonyMailer. Might break some sites on upgrade which is bad eh?

silverstripe/mailer)
Not sure what benefit silverstripe/mailer would bring? We could move all the email stuff to a different module sure, though it would still be requirement of silverstripe/framework (you gotta have password reset functionality after all). If you went for a different mailer in a module you'd still be installing silverstripe/mailer by default, just you wouldn't use it

@xini
Copy link
Contributor

xini commented Sep 27, 2021

I think refactoring to use SymfonyMailer makes a lot of sense. SymfonyMailer supports the standard built-in transports like smtp and sendmail (https://symfony.com/doc/current/mailer.html#using-built-in-transports) but also has a lot of third-party transports available already (https://symfony.com/doc/current/mailer.html#using-a-3rd-party-transport).

@GuySartorelli
Copy link
Member

We have a module we use on almost all of our sites (signify-nz/silverstripe-mailblock) which includes a plugin for Swiftmailer - dropping support for Swiftmailer altogether in favour of SymfonyMailer would break that module.

IMO while it would be good to provide support for SymfonyMailer, Swiftmailer also needs to be supported until SS5.0 for backwards compatability, since dropping it would be a breaking change.

@maxime-rainville
Copy link
Contributor

Looking at the mail config, we could implement a SynfonyMailer that implements the SilverStripe\Control\Email\Mailer and provide a updated email config that supersedes the framework one. We would still have to ship SwiftMailer with framework, but that shouldn't conflict with SymphonyMailer because it's now under a different namespace.

---
Name: emailconfig
---
SilverStripe\Core\Injector\Injector:
Swift_Transport: Swift_MailTransport
Swift_Mailer:
constructor:
- '%$Swift_Transport'
SilverStripe\Control\Email\Mailer:
class: SilverStripe\Control\Email\SwiftMailer
properties:
SwiftMailer: '%$Swift_Mailer'

@maxime-rainville maxime-rainville changed the title Swiftmailer soon to be end-of-life Swiftmailer is end-of-life. Need to upgrade to Symfony Mailer Feb 1, 2022
@sminnee
Copy link
Member

sminnee commented Jun 27, 2022

Maybe we should take this opportunity to switch to an adapter pattern, and then it would theoretically be less of a problem with this situation in the future.

Isn't SilverStripe\Control\Email\Mailer an adapter?

@emteknetnz
Copy link
Member

emteknetnz commented Jun 27, 2022

Isn't SilverStripe\Control\Email\Mailer an adapter?

Yes technically? Mailer is very lightweight interface - it's set here here. I'd be surprised if you could just swap this out and things just work. I'd assume that in the codebase there are various calls to email methods that are not defined in the Mailer interface

The work for actually sending mails is done by Email (and the Email class is a docblock param for the Mailer interface anyway). Email is very much hardcoded to SwiftMailer e.g. https://github.com/silverstripe/silverstripe-framework/blob/4/src/Control/Email/Email.php#L263

Seems messy to turn this into an adapter at this stage in CMS4. We have CMS5 on the horizon so that gives us the opportunity to do this cleaner

@GuySartorelli
Copy link
Member

All PRs merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants