humanmade / aws-ses-wp-mail

An AWS SES wp_mail() drop-in
184 stars 51 forks source link

Fixed #34: Added support for attachments #35

Open faishal opened 6 years ago

faishal commented 6 years ago
joehoyle commented 6 years ago

I thing the biggest problem here is that working with attachments means different filters and way of sending the message, which is going to be annoying to work with. As a developer I can't rely on aws_ses_wp_mail_ses_send_message_args in case email would happen to have an attachment. That's probably not such a big issue if we have "higher level" hooks for most configuration. I know I've had to use this catch-all hook for things before as a suitable WordPress hook for email modification hasn't existed.

We could also consider only ever sending raw, so there's only one method to deal with. I don't see much value in keeping around SendMessage if we have to have a fully reliable SendRawMessage implementation. Having a branch is undoubtedly going to cause some bugs. However, SendMessage is presumably easier to implement and less error prone, so we need to weigh up the benefit of that versus the pretty rarely used sending of attachments.

If we push ahead I think get_raw_message needs to be broken up to have the parsing be in their own functions to actually be able to read this code without getting lost. This like concat of the strings with \n are also pretty gnarly to read, we should be able to make this is a little clearer with some imploding etc.

faishal commented 6 years ago

@joehoyle That makes sense, After reading your comment I dig into what we can do to make it simple.

How about using PHPMailer class of WordPress and build raw email?

I looked into the wp_mail function for phpmailer usage and able to generate a raw email using it : https://gist.github.com/faishal/5d6d1c2a0f03d9a7534f9e836bcd6c34

We can also create a custom class to do that but I feel like phpMailer is providing all encoding supports.

joehoyle commented 6 years ago

Interesting, I think using PHPMailer might be handy. In that case, do we even need to hook wp_mail, can't we just capture the PHPMailer getSentMIMEMessage pre-send?

faishal commented 6 years ago

Do you mean removing wp_mail pluggable method from plugin?

In that case it will not require to build the mail, we just have to send it via aws ses api. Unfortunately, There isn't any hook/filter in class-phpmailer.php, but there is one way I can think of.

PHPMailer::postSend calls Method based on PHPMailer::Mailer value, so if we set PHPMailer::Mailer value to awsses then it will call awssesSend method.

If we don't want to repeat what wp_mail is doing and just want to hook custom send method then we can do following.

  1. Define SES_PHPMailer from base class PHPMailer with custom awssesSend method.
  2. Hook into wp_mail filter and initiate custom SES_PHPMailer class object and assign it to global $phpmailer only if we have the valid AWS constants defined.
  3. Hook into phpmailer_init and set PHPMailer::Mailer value to awsses so that it will call awssesSend method.
  4. in awssesSend we will get raw mail body by calling getSentMIMEMessage method, We just have to initiate aws client and send mail using sendRawEmail method of aws and throw an appropriate phpmailerException exceptions if we catch any.
joehoyle commented 6 years ago

Cool yeah that's what I was getting at. I think this approach is certainly interesting. I'm not 100% whether to proceed - what would be your suggested route?

faishal commented 6 years ago

@joehoyle We can set milestone 0.2.0 which will include the above-mentioned refactoring and close this PR. We can test this with other major plugins to verify its working as expected.

faishal commented 6 years ago

@joehoyle - I created PR https://github.com/humanmade/aws-ses-wp-mail/pull/37 with above suggested changes

roborourke commented 2 years ago

Is this already in use? I noticed we have a 2.0.0-alpha tag with this feature.