search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

Enable Swiftmailer v6 support #9834

Closed brynwhyman closed 3 years ago

brynwhyman commented 3 years ago

Overview

There is a new version of Swiftmailer (v6) which provides PHP 7.4 support and resolves other compatibility issues (S/MIME being an example).

Acceptance criteria

Very important note:

Note

PRs

danaenz commented 3 years ago

Hey @brynwhyman , I'm keen to pick this up—who can confirm the version requirements of the first AC? Alternatively I can start and note down the effort/impact to support both Swiftmailer versions.

dhensby commented 3 years ago

We need some input from more of the @silverstripe/core-team for this, but my initial view on version support is that we don't need to support both v5 and v6. We just need to make sure our API does not break. I would say that the change should go into a minor release, not a patch, though.

brynwhyman commented 3 years ago

I'm not part of the core team but agree that if we can, we should reduce the maintenance effort and just support v6 if possible, making necessary changes in the CMS to support it.

There's also the chance that projects might be using other parts of Swiftmailer v5 that don't come out of the box with the CMS which we might need to consider if projects are pushed to upgrade to v6 as part of a future CMS minor upgrade. Docs in a changelog are one way to advise people of this, but that could also be missed...

Also worth noting though that Swiftmailer v5 isn't compatible with PHP 7.4 (according to https://github.com/silverstripe/silverstripe-framework/issues/9796) and PHP 7.3 goes EOL in December this year. If we want to retain support for v5 in the CMS and not force people to make the upgrade to v6, it looks like we'd be doing it for projects that are also not keeping up with in-support versions of PHP which isn't ideal.

Alternatively I can start and note down the effort/impact to support both Swiftmailer versions.

That sounds like a good way to help get this moving though :)

dhensby commented 3 years ago

making necessary changes in the CMS to support it.

There shouldn't be any necessary changes to any other modules, otherwise it would be a breaking change... unless those changes are just to take advantage of some new feature we add to our API.

There's also the chance that projects might be using other parts of Swiftmailer v5 that don't come out of the box with the CMS which we might need to consider if projects are pushed to upgrade to v6 as part of a future CMS minor upgrade. Docs in a changelog are one way to advise people of this, but that could also be missed...

If consumers use swiftmailer v5 they should be declaring that in their composer.json in their project and then when they try to upgrade they'll get an error and thus know they need to upgrade their implementations.

chillu commented 3 years ago

Late to the party, but I don't see a reason to maintain v5 compat.

brynwhyman commented 3 years ago

Updated the ACs in relation to v5 support:

  • [ ] Swiftmailer v5 support is dropped on introduction to v6 support
emteknetnz commented 3 years ago

Internal slack convo about the sendmail binary and php configuration https://silverstripeltd.slack.com/archives/CEYE6K29Y/p1627958863002200

emteknetnz commented 3 years ago

The v5 and v6 versions of SendmailTransport.php are basically identical

v5.4.12 https://github.com/swiftmailer/swiftmailer/blob/181b89f18a90f8925ef805f950d47a7190e9b950/lib/classes/Swift/Transport/SendmailTransport.php

v6.2.7 https://github.com/swiftmailer/swiftmailer/blob/15f7faf8508e04471f666633addacf54c0ab5933/lib/classes/Swift/Transport/SendmailTransport.php

This implies that that the v5 version of MailTransport.php should be usable within v6 if native php mail() functionality is required (which it almost certainly is)

emteknetnz commented 3 years ago

Core issue here is that swiftmailer v6 no longer support the mail() function, meaning there needs to be some level of server configuration either with installing the sendmail binary and updating php.ini to use it, or to use SMTP which requires a fair bit more configuration

Swiftmailer v5 has a couple of known issues with it relating to sending encrypted emails, and a deprecation notice

Here's a few options: a) Fork switftmailer v5, backport fixes for known issues, similar to what we did with sminnee/phpunit, never recieve updates again. Not really a long term solution b) Require swiftmailer v6, tell everyone to install sendmail or else emails will no longer be sent. This would be the most "proper" solution c) Copy paste v5 MailTransport.php into our code base (it should be compatible with v6) and give credit to original authors. It has MIT license so I presume we can do this?

b) or c) we could potentially dual support v5 and v6, though I'd prefer that we only specify v6

dhensby commented 3 years ago

Copy paste v5 MailTransport.php into our code base (it should be compatible with v6) and give credit to original authors. It has MIT license so I presume we can do this?

This is specifically one of the solutions that is offered up in the rather long thread

emteknetnz commented 3 years ago

Linked PR will work out of the box on Silverstripe Ltd cloud hosting and CWP hosting, so from that perspective at least I'm less fussed about the missing MailTransport class

I'm not too keen on the ^5 || ^6 because it's additional maintainence, and sites will just automatically upgrade to 6 anyway when they run composer update and will switch to using SendmailTransport i.e. it's no different to just having ^6. The only benefit is they can specify ^5 in their project composer.json, though they could also just use an alias if they really had stay of ^5 for whatever reason. Also staying on ^5 just seems like an edge case to me.

In terms of the missing MailTransport class ...

I don't think forking Swiftmailer 5 is really a viable option, it's just too short sighted.

Here are some new options: 1) Update docs saying if your server can't do sendmail/smtp, then either upgrade your server (best idea) or copy-paste MailTransport.php into your project code base 2) Create a new repo, add MailTransport.php to it, update docs saying you can require this is you don't have sendmail/SMTP 3) Create a new repo, add MailTransport.php to it, require it by framework 4) Add MailTransport.php to framework

I think 1) is the ideal solution, as 2), 3) and 4) are kind of adding tech-debt and maintenance. We'd just need to make it very prominent that "If your site is no longer sending emails after upgrade, this is why". Keeping in mind that loads of people doing upgrades won't read patch notes, and all sites have "forgot password" functionality

dhensby commented 3 years ago

Whilst we can do what SwiftMailer did and consider the "technically" best solution (ie: drop support for mail) we need to consider the "developer experience" of upgrading to the next minor and the pain that a lot of our community will feel from "just" upgrading their server or duplicating mailer transports.

For semver compliance the upgrade should not require any new configuration or changes to the application code, so from that point of view I feel that the framework should ship with a MailTransport class and it could issue some deprecation warning or notice saying people should aim to stop using it, but I think we need to provide that support out-of-the-box.

As you point out, a note in upgrade docs probably won't help that much and it'll be something people find out only once they realise emails haven't been sent for a few weeks/months/wtv.

maxime-rainville commented 3 years ago

Yeah, I'm on the "Add MailTransport to framework" bandwagon. Probably should deprecate it right way and get it to throw warnings.

Is there anyway we could add something to the installer and/or recipe-core so people don't use MailTransport out-of-the-box for new projects? I'm thinking we probably need to provide clear guidance on what we think the optimal setup is as well.

emteknetnz commented 3 years ago

Yeah you could keep this in framework

---
Name: emailconfig
---
SilverStripe\Core\Injector\Injector:
  Swift_Transport: Swift_MailTransport

And get this copied into myemailconfig.yml as part of composer create-project

---
Name: myemailconfig
After: emailconfig
---
SilverStripe\Core\Injector\Injector:
  Swift_Transport: Swift_SendmailTransport

And then put in some docs about "if email don't work, configure your server, or just delete this file"

emteknetnz commented 3 years ago

Update: I've kind of backpeddled on the idea of adding in the sendmail config on installer. The issue is that if you have a site that doesn't normally send emails, you'll only discover that your email functionality is broken the first time you need to use the 'forgot password' functionality, meaning you've just locked yourself out from your own site.

I think we'll just have to document that upgrading to Swift_SendmailTransport is recommended

maxime-rainville commented 3 years ago

It's not that atypical to have a host where your email set up doesn't quite work out of the box. Many ISP or hosting service will block you from using the underlying server to send email to make sure you don't host a SPAMMING service. Instead, they will force you to route all your emails to a dedicated SMTP server ... or to use your own mailing service like AWS SES or SendGrid or MailGun.

In short, assuming that your site can send emails without testing it upfront is a bad idea.

If you do end up having to reset your password, you have alternatives like setting the SS_DATABASE_USERNAME/SS_DATABASE_PASSWORD env variable.

If I have a choice between making our default setup a little more likely to send email out-of-the-box or a little more secure out-of-the-box, I'm picking secure.

emteknetnz commented 3 years ago

In short, assuming that your site can send emails without testing it upfront is a bad idea.

I've got no issues with forcing developers to do more work upfront to get their email sending more secure, in fact that would be my preference. Unfortunately I just don't know how we could enforce this. We don't require an email to be sent as part of building a site.

If you do end up having to reset your password, you have alternatives like setting the SS_DATABASE_USERNAME/SS_DATABASE_PASSWORD env variable.

And what if you're not a developer? What if you're a non-technical site owner who paid a developer to build a site 6 months ago? Now you're locked out of your own site.

maxime-rainville commented 3 years ago

And what if you're not a developer? What if you're a non-technical site owner who paid a developer to build a site 6 months ago? Now you're locked out of your own site.

That's just as likely to happen right now. You can't put a website on the internet and just expect it to work forever without someone looking after it.

dhensby commented 3 years ago

I think we need to consider the developer experience and also the impact this has on site owners.

Developers expect these upgrades to be easy and for functionality (like emailing) to work between upgrades without having to reconfigure or install server side software and services.

We can bang on all day about what developers "should" be doing and what "best practice" is, but we also need to accept reality. Sure, I wouldn't have any website of mine sending through email through the webserver at all, but I can't make that expectation of everyone running a Silverstripe site.

I feel it also goes against our semver principals to make what feels like an agressive change. IMO it's fine to change major versions of our dependencies as long as our behaviour and APIs stay consistent, by removing the default to send using mail() we are breaking that principle.