@sgiehl opened this Pull Request on April 30th 2020 Member

fixes #14841

@Findus23 commented on April 30th 2020 Member

If I could wish for one more minor feature:

Would it be possible to add the verbose logging ($mail->SMTPDebug = SMTP::DEBUG_SERVER;) to the output of the core:test-email command? This way debugging issues with E-Mail delivery would be quite a bit easier.

@sgiehl commented on April 30th 2020 Member

@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?

@Findus23 commented on May 4th 2020 Member

I would at least test the following:

  • [ ] sending an E-Mail using SMTP via Gmail (https://github.com/matomo-org/matomo/issues/8613)
  • [ ] sending an E-Mail using both SSL and TLS
  • [ ] sending an E-Mail with an Attachment
  • [ ] sending an E-Mail on a normal webhost without SMTP
  • [ ] (probably not possible by you, but someone else could contribe): test if https://github.com/matomo-org/matomo/issues/12007 is fixed
  • [ ] sending an E-Mail with various invalid settings (incorrect host/user/password) and check if the user gets a helpful error message (no more fwrite(): send of 6 bytes failed with errno=32 Broken pipe)
  • [ ] sending an E-Mail with a Matomo not set to English and check if the error messages are localized
  • [ ] sending an E-Mail on Windows
@sgiehl commented on May 4th 2020 Member

@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 thephpleague/oauth2-client, which pulls in some other libs and polyfills. Wondering if it might be better, to put that into a separate plugin as I don't think using gmail is a common use case, is it?
(The Mail.send event could be actually used to inject the oauth settings before sending)

@mattab @tsteur what do you think re OAuth support?

@Findus23 commented on May 4th 2020 Member

@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.
Sending E-Mails via Gmail should still be possible "the normal way": https://github.com/PHPMailer/PHPMailer/blob/master/examples/gmail.phps

If people really need it, creating a plugin for it is probably easier.

@sgiehl commented on May 4th 2020 Member

Ok. So far I have tested:

  • use Gmail with app specific token
  • use custom smtp host with TLS (don't have one still working with ssl)
  • use local sendmail
  • check attachments are correct (inline images and pdfs)
  • check some errors with invalid configs
  • ensure phpmailer errors are in current language

Seems to work so far and should be ready for a review now.

@Findus23 commented on May 4th 2020 Member

Sounds good. The rest could probably be done with general Matomo 4 testing duing the beta releases.

@tsteur commented on May 11th 2020 Member

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 vendor/phpmailer library and things still work because we're not allowed to ship our plugin with PHPMailer dependency. Ideally, we would have something like

class Mail {
public function send() {
   $phpmailer = new PHPMailer(); $phpmailer->.... send();
}
}

instead of class Mail extends PHPMailer{}. This would make our mailer implementation more independent of the actual library but of course this has potentially some downsides. Not sure how easy that would be doable...

Did a quick test... by default WP does not find PHPMailer
image

It does work when using include ABSPATH . '/wp-includes/class-phpmailer.php';

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)
image

Ideally, we'd not extend the PHPMailer class and ideally, if possible, only use PHPMailer as part of the send method or so.

@sgiehl commented on May 12th 2020 Member

@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....

@tsteur commented on May 13th 2020 Member

@sgiehl just tested it but noticed there's now a method addBcc missing.

The setReplyTo method might be good to not deprecate it but instead actually make it a set (meaning overwrite any previously set replyTo)?

Otherwise looks good and things worked. Also text mails and various attachments. Be good to make the two changes and then merge 👍

@sgiehl commented on May 14th 2020 Member

@tsteur applied the feedback. will merge once the tests passed...

This Pull Request was closed on May 14th 2020
Powered by GitHub Issue Mirror