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
Replace Zend Mail with PHPMailer #15891
Conversation
If I could wish for one more minor feature: Would it be possible to add the verbose logging ( |
@Findus23 thanks for the hint. Did not yet manually test anything only tried to get the tests passing. Guess we will need to do some manual testing with various settings before merging this. Could you maybe give me some common scenarios that should be tested? And maybe also some that didn't work with Zend_Mail if you know any? |
I would at least test the following:
|
@Findus23 Adding support for Gmail XOAuth would be possible using PHPMailer, but the required packages are not included by default. We would need to require |
@sgiehl I think this XOAuth support is not needed as it would add a lot of overhead and potential confusion to users for little gain. If people really need it, creating a plugin for it is probably easier. |
Ok. So far I have tested:
Seems to work so far and should be ready for a review now. |
Sounds good. The rest could probably be done with general Matomo 4 testing duing the beta releases. |
@@ -121,6 +121,14 @@ public function doUpdate(Updater $updater) | |||
// switch to default provider if GeoIp Legacy was still in use | |||
LocationProvider::setCurrentProvider(LocationProvider\DefaultProvider::ID); | |||
} | |||
|
|||
// @todo migrate that to a config migration. See utf8mb4 branch |
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.
as per comment. I've already built a config migration for the utf8mb4 branch. We should change that once that's merged. See 8907203
I'm also seeing another problem re WordPress. See https://developer.wordpress.org/plugins/wordpress-org/detailed-plugin-guidelines/#13-plugins-must-use-wordpress-default-libraries We will need to test that in Matomo for WordPress we can safely remove the
instead of Did a quick test... by default WP does not find PHPMailer It does work when using WP is currently using PHPMailer 5.2.27 in their latest release. However, older versions of WP might use different versions and our PHPMailer version might have slightly different definitions causing issues potentially like these (could happen in the future) Ideally, we'd not extend the PHPMailer class and ideally, if possible, only use PHPMailer as part of the send method or so. |
@tsteur completely refactored the code now. The Piwik\Mail class is now independent and uses a Transport class to send the mail using PHPMailer. The transport class can be replaced, which can be used by wordpress or cloud plugins.... |
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 @sgiehl I think that's something I will be able to work with (hopefully). I'm expecting some trouble where we might need to add quite some code in Matomo for WordPress around attachments but will try to workaround and can always make further changes later if needed. Left a few other comments to look at. Cheers
plugins/ScheduledReports/ReportEmailGenerator/AttachedFileReportEmailGenerator.php
Outdated
Show resolved
Hide resolved
plugins/ScheduledReports/ReportEmailGenerator/AttachedFileReportEmailGenerator.php
Outdated
Show resolved
Hide resolved
@sgiehl just tested it but noticed there's now a method The Otherwise looks good and things worked. Also text mails and various attachments. Be good to make the two changes and then merge 👍 |
@tsteur applied the feedback. will merge once the tests passed... |
Hi, If anyone need to disable $SMTPAutoTLS functionalty because of a getting an error: SMTP Error: Could not connect to SMTP host. PHPMailer.php
You can set true to false. This change may cause security vulnerability that we cannot predict. Please also read the ongoing messages. |
Hi @hahayidu, If I understand it correctly setting this setting to false, means PHPMailer will accept any invalid certificate which means anyone can man-in-the-middle your E-Mail connection and read and modify it and phpmailer will just accept their faked certificate as correct. So I'd recommend you to fix the actual error instead of disabling security checks. |
Hi @Findus23, Yes you are right if matomo send mail directly to wide area network without encryption someone can be listen. In particular, sending a password reset request can be more dangerous than reports. In my case, I wasn't even getting a certificate error. Because our internal smtp relay server doesn't a valid TLS configuration. (Probably doesn't have an any best practices configuration) I know this kind of case is not common. I think real problem is PHPMailer override the mail configuration. This is my configuration.
If $SMTPAutoTLS is true matomo can't send e-mail. I expect if I don't select any encryption, phpmailer shouldn't encrypt to connection. Empty is also an option in matomo ui. So, Opportunistic TLS (which is default for phpmailler) is not useful some case. Thank you for your response. I will update first post with your concerns. |
fixes #14841