Fixed #34: Added support for attachments by faishal · Pull Request #35 · humanmade/aws-ses-wp-mail

faishal · 2018-02-23T07:37:20Z

Created multipart email for mails with attachments.
Added param in wp-cli commad.
Updated readme with example command.

Signed-off-by: Faishal Saiyed <saiyedfaishal@gmail.com>

rmccue

I'd also want review from @joehoyle on this.

rmccue · 2018-02-26T03:37:22Z

+	 *
+	 * @return string
+	 */
+	private function get_raw_message( $to, $subject, $message, $headers = array(), $attachments = array() ) {


This should be protected rather than private

rmccue · 2018-02-26T03:38:40Z

+						$reply_to = array_merge( (array) $reply_to, explode( ',', $content ) );
+						break;
+					default:
+						$raw_message_header .= $name . ': ' . $content . "\n";


Should $content be newline-escaped to avoid header overflow issues?

rmccue · 2018-02-26T03:39:50Z

+		$raw_message .= 'MIME-Version: 1.0' . "\n";
+		$raw_message .= 'Content-type: Multipart/Mixed; boundary="' . $boundary . '"' . "\n";
+		$raw_message .= "\n--{$boundary}\n";
+		$raw_message .= 'Content-type: Multipart/Alternative; boundary="alt-' . $boundary . '"' . "\n";


I think these should be Content-Type with the actual type lowercase?

I looked into RFC for that, In starting it showing Content-Type but in the example, it was showing Content-type, so I tested both and working fine.
Ref: https://www.w3.org/Protocols/rfc1341/rfc1341.html

Yeah, headers are theoretically case-insensitive (likewise in HTTP), but in practice, it's best to use the exact case.

rmccue · 2018-02-26T03:43:32Z

+			$custom_from = sprintf( '%s <%s>', apply_filters( 'wp_mail_from_name', get_bloginfo( 'name' ) ), apply_filters( 'wp_mail_from', $from_email ) );
+		}
+
+		$boundary    = uniqid( rand(), true );


wp_rand() would be better than uniqid( rand() ) I think, and I'd probably add a prefix as well:

$boundary = 'aws-ses-wp-mail-' . wp_rand();

rmccue · 2018-02-26T03:44:11Z

+
+		$raw_message .= 'MIME-Version: 1.0' . "\n";
+		$raw_message .= 'Content-type: Multipart/Mixed; boundary="' . $boundary . '"' . "\n";
+		$raw_message .= "\n--{$boundary}\n";


I'd probably use sprintf() for these instead, I think it's a bit more readable.

rmccue · 2018-02-26T03:45:32Z

+		$raw_message .= "\n--alt-{$boundary}--\n";
+
+		foreach ( $attachments as $attachment ) {
+			if ( ! @is_file( $attachment ) ) {


Is there a better way to check this and avoid the error suppression operator?

rmccue · 2018-02-26T03:45:49Z

+			}
+
+			$raw_message .= "\n--{$boundary}\n";
+			$raw_message .= 'Content-Type: ' . $file_type['type'] . '; name="' . $filename . '"' . "\n";


I'd use sprintf() for this.

Signed-off-by: Faishal Saiyed <saiyedfaishal@gmail.com>

joehoyle · 2018-02-26T15:41:56Z

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 · 2018-02-27T03:31:11Z

@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 · 2018-03-03T17:12:43Z

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 · 2018-03-05T08:10:32Z

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.

Define SES_PHPMailer from base class PHPMailer with custom awssesSend method.
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.
Hook into phpmailer_init and set PHPMailer::Mailer value to awsses so that it will call awssesSend method.
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 · 2018-03-08T16:37:20Z

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 · 2018-03-12T07:55:23Z

@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 · 2018-04-05T05:40:44Z

@joehoyle - I created PR #37 with above suggested changes

tareiking · 2019-02-04T21:54:26Z

+		}
+		$raw_message .= sprintf( "\n--alt-%s--\n", $boundary );
+
+		foreach ( $attachments as $attachment ) {


@faishal - wp_mail $attachments param also accepts a single string file location as a valid param. Currently, passing a single string results in attachments not being sent here.

We could follow cores approach and cast as follows (before entering the loop here)

if ( ! is_array( $attachments ) ) { $attachments = explode( "\n", str_replace( "\r\n", "\n", $attachments ) ); }

…ain email

roborourke · 2021-11-15T11:18:02Z

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

roborourke · 2025-03-12T11:54:06Z

@tomjn do you have a use case for this currently?

tomjn · 2025-03-12T12:02:21Z

@roborourke I had looked at it as part of something else briefly but wanted to close out my tabs and focus elsewhere, I know this had discussion on a client project very recently but @Rayhatron is a better person to ask.

What would be good to have is a summary of where this work currently is, is there remaining blockers, what tasks need completing, how much time would need to be spent to ship this. Hence the help wanted label. Unfortunately I don't have the bandwidth to champion this myself.

Rayhatron · 2025-08-26T15:14:22Z

@roborourke sorry, I seem to have missed this so only seeing it now. A use case for this would be a form that collects user details and generates a certificate of attendance PDF for an event then sends that in an email after the user submits the form.

Due to attachments not being supported, workaround is to send a link in the email that when opened will generate and download/show the PDF.

Fixed #34: Added support for attachments

feb0bf7

Signed-off-by: Faishal Saiyed <saiyedfaishal@gmail.com>

faishal requested a review from joehoyle February 23, 2018 07:37

faishal added the review & merge label Feb 23, 2018

rmccue requested changes Feb 26, 2018

View reviewed changes

faishal added 2 commits February 26, 2018 10:48

Fixed CR issues, use sprinf for better reading and added some escaping

a003cd1

use sprinf for sring building

45e75ad

Signed-off-by: Faishal Saiyed <saiyedfaishal@gmail.com>

faishal removed the review & merge label Mar 28, 2018

tareiking reviewed Feb 4, 2019

View reviewed changes

Nikschavan added 6 commits July 10, 2020 20:36

Update to master and fix merge conflicts

b442f28

cs fixes

ef7eef4

Revert unintended changes

c7cec1a

Quote the fron name for the the raw message

e77471a

Reduce unwanted changes from the master branch

6f0c0b4

Set from_email to no-reply@ for the raw email to be similar to the pl…

f18904b

…ain email

abhishek-kaushik requested review from rmccue and tareiking September 14, 2021 10:25

ignore Result output

9f6b27f

Merge branch 'master' into feature/add-attachment-support

2919cf3

tomjn added the help wanted label Mar 11, 2025

Uh oh!

Conversation

faishal commented Feb 23, 2018

Uh oh!

rmccue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joehoyle commented Feb 26, 2018

Uh oh!

faishal commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joehoyle commented Mar 3, 2018

Uh oh!

faishal commented Mar 5, 2018

Uh oh!

joehoyle commented Mar 8, 2018

Uh oh!

faishal commented Mar 12, 2018

Uh oh!

faishal commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roborourke commented Nov 15, 2021

Uh oh!

roborourke commented Mar 12, 2025

Uh oh!

tomjn commented Mar 12, 2025

Uh oh!

Rayhatron commented Aug 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

faishal commented Feb 27, 2018 •

edited

Loading