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

Update SwiftMailer from v5 to v6 #10031

Conversation

danaenz
Copy link
Contributor

@danaenz danaenz commented Jul 22, 2021

Parent issue #9834

Issues addressed

We noticed issues with SwiftMailer v5 and encrypted file attachments. We implemented a fork successfully with our (WIP) SMIME encryption module.

Configuration updates

Default Swift_Transport changed from Swift_MailTransport to Swift_SendmailTransport. No configuration changes are required in project code—it is assumed that any variation on configuration in the affected area is by using an alternative SwiftMailer transport class (eg, Swift_SmtpTransport) which does not use a new classname. The docs have been updated to reflect this change.

Release version

Since this is a major version change for Swiftmailer and includes a configuration update, I would suggest this to be a minor release.

Deprecation notes

I did not find any additional deprecation notices across PHP 7.4 and 7.3, and will rely on CI to catch other notices.

@danaenz danaenz changed the title Addresses issue #9834: Update SwiftMailer from v5 to v6 Jul 22, 2021
@danaenz danaenz force-pushed the pulls/swiftmailer-v6-support branch from 8d35281 to 4b9248e Compare July 22, 2021 04:04
- Fixes silverstripe#9834
- Update default Swift_Transport to use Swift_SendmailTransport
- Update version restraint for Swiftmailer
- Address new parameter type for Swift_Message::setDate()
- Update class references in docblocks
@danaenz danaenz force-pushed the pulls/swiftmailer-v6-support branch from 4b9248e to 4b3cebc Compare July 22, 2021 04:25
@danaenz
Copy link
Contributor Author

danaenz commented Jul 26, 2021

Post-Innovation Day notes:

Acceptance Criteria needs to be checked off, and new tests identified and created (if required). After that, this PR will be ready to come out of draft mode.

Copy link
Contributor

@dhensby dhensby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the SendmailTransport is not a drop-in replacement for the MailTransport in that it doesn't make use of PHP's built-in mail() function.

I think this needs to be addressed and out-of-the-box we need to be using the mail() function

@@ -14,7 +14,7 @@ covers how to create an `Email` instance, customise it with a HTML template, the
SilverStripe provides an API over the top of the [SwiftMailer](http://swiftmailer.org/) PHP library which comes with an
extensive list of "transports" for sending mail via different services.

Out of the box, SilverStripe will use the built-in PHP `mail()` command via the `Swift_MailTransport` class. If you'd
Out of the box, SilverStripe will use the built-in PHP `mail()` command via the `Swift_SendmailTransport` class. If you'd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't what the sendmail transport does, it interacts with the sendmail binary on the operating system - https://swiftmailer.symfony.com/docs/sending.html#transport-types

I think we will either need to pull in transport that uses mail() or write our own :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@emteknetnz emteknetnz Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this isn't so great

We cannot guarantee if a server has the sendmail binary is available nor if it's enabled in php.ini sendmail_path e.g. some webhosts will use postfix as the backend for mail()

It means that when a site upgrades to say framework 4.9 (assuming this change is merged in) then a site that sends email may suddenly (and probably silently) stop sending email

I think we will either need to pull in transport that uses mail() or write our own :/

Pretty much, I don't see any way around having add back support for the mail() function

@@ -269,7 +272,7 @@ public function getSwiftMessage()
*/
public function setSwiftMessage($swiftMessage)
{
$swiftMessage->setDate(DBDatetime::now()->getTimestamp());
$swiftMessage->setDate(new DateTime());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks our ability to mock the datetime centrally.

@emteknetnz emteknetnz mentioned this pull request Aug 3, 2021
9 tasks
@emteknetnz
Copy link
Member

Closed in favor of #10048 - I've kept the original commit in this PR, thanks for your contribution :-)

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

Successfully merging this pull request may close these issues.

None yet

3 participants