-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Upgrade to swiftmailer-6 #9791
Upgrade to swiftmailer-6 #9791
Conversation
Unit tests fail hard :/ |
Yeah need to fix those as well.... |
Codecov Report
@@ Coverage Diff @@
## master #9791 +/- ##
=========================================
Coverage ? 31.71%
Complexity ? 26020
=========================================
Files ? 1661
Lines ? 96178
Branches ? 1291
=========================================
Hits ? 30504
Misses ? 65674
Partials ? 0
|
52ea2df
to
e9a6f7e
Compare
e2817c6
to
bd3d9ab
Compare
Should be ready for review. |
bd3d9ab
to
b6bd2ae
Compare
Please add it to #7827 |
Done. lets get this in! |
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.
Tested and works 👍
core/js/setupchecks.js
Outdated
messages.push({ | ||
msg: t( | ||
'core', | ||
'Use of the the built in php mailer is no longer supported. Please update your e-mail settings.' |
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 would use a different term then "your email settings", otherwise the admin might be confused and check their personal settings.
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.
updated
@@ -523,6 +523,10 @@ protected function getCronErrors() { | |||
return []; | |||
} | |||
|
|||
protected function isPhpMailerUsed(): bool { | |||
return $this->config->getSystemValue('mail_smtpmode', 'smtp') === '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.
The default was php
, so if it is not set, it used to do php
and should also show the warning?
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 new default is smtp
;)
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.
exactly, but smtp requires settings, if they are not there emails fail all of a sudden and you dont get a warning because you used the default, so you get "smtp" now and the warning is not displayed
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.
Ah - got it. Yes this need to be changed to 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.
nice catch!
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.
fixed
b6bd2ae
to
b379bd3
Compare
core/js/setupchecks.js
Outdated
messages.push({ | ||
msg: t( | ||
'core', | ||
'Use of the the built in php mailer is no longer supported. Please update your email server settings.' |
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.
Could also add a link to the settings, because the messages seem to support html.
Also text suggestion to make clear it's not about the mail server:
Please update the email configuration in the <linky>basic settings<linky>.
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. Will take care later
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.
Aaah mmm I'm not sure if we can directly link to settings :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 added
b379bd3
to
2a5b1ae
Compare
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
* \Swift_Mailer handles starting the transport etc properly * Fixed tests Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
2a5b1ae
to
6a0c54d
Compare
@nickvergessen 👍 ?? |
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.
Works
Requires:
Todo: