Fixed #34: Added support for attachments #35

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 <[email protected]>

rmccue

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

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

inc/class-ses.php

+	 *
+	 * @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

inc/class-ses.php

+						$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

inc/class-ses.php

+		$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

inc/class-ses.php

+			$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

inc/class-ses.php

+
+		$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

inc/class-ses.php

+		$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

inc/class-ses.php

+			}
+
+			$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 <[email protected]>

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

inc/class-ses.php

+		}
+		$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.

Fixed #34: Added support for attachments

feb0bf7

Signed-off-by: Faishal Saiyed <[email protected]>

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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #34: Added support for attachments #35

Fixed #34: Added support for attachments #35

faishal commented Feb 23, 2018

rmccue left a comment

rmccue Feb 26, 2018

rmccue Feb 26, 2018

rmccue Feb 26, 2018

faishal Feb 26, 2018

rmccue Feb 26, 2018

rmccue Feb 26, 2018

rmccue Feb 26, 2018

rmccue Feb 26, 2018

rmccue Feb 26, 2018

joehoyle commented Feb 26, 2018

faishal commented Feb 27, 2018 •

edited

Loading

joehoyle commented Mar 3, 2018

faishal commented Mar 5, 2018

joehoyle commented Mar 8, 2018

faishal commented Mar 12, 2018

faishal commented Apr 5, 2018

tareiking Feb 4, 2019

roborourke commented Nov 15, 2021

Fixed #34: Added support for attachments #35

Are you sure you want to change the base?

Fixed #34: Added support for attachments #35

Conversation

faishal commented Feb 23, 2018

rmccue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehoyle commented Feb 26, 2018

faishal commented Feb 27, 2018 • edited Loading

joehoyle commented Mar 3, 2018

faishal commented Mar 5, 2018

joehoyle commented Mar 8, 2018

faishal commented Mar 12, 2018

faishal commented Apr 5, 2018

Choose a reason for hiding this comment

roborourke commented Nov 15, 2021

faishal commented Feb 27, 2018 •

edited

Loading