sendgrid / wordpress

SendGrid plugin for WordPress
51 stars 64 forks source link

Add support for smtpapi filters #38

Closed jmfield2 closed 7 years ago

jmfield2 commented 8 years ago

Like x-smtpapi-to, this adds support for setting x-smtpapi-filters via $headers as a comma-delimited string:

filter_name:param_name=param_value,filter_name:param_name=param_value

Which allows the plugin to set e.g: open and click tracking filters like so: x-smtpapi-filters: opentrack:enable=1,clicktrack:enable=1

Hopefully it is OK to use the addFilter method of $mail->smtpapi directly. Also, the only other comment is that this is missing basic validation on the inputs - that $filter has only 1 ':' and that $params has only 1 '='.

Otherwise, I think this is good now and it seems to work for me. :)

efimovalex commented 7 years ago

We are trying to move away from this approach, we recommend that you use the V2 SendGrid PHP Email object and send it in the $headers param of wp_mail.

jmfield2 commented 7 years ago

OK, cool. Are you sure this will actually work, though? I did try doing it this way before, and from looking at the code (https://github.com/sendgrid/wordpress/blob/master/lib/sendgrid/sendgrid-wp-mail.php#L229), it seems like any extra headers that happen to be set as you propose I do are never actually set on the Sendgrid_WP object which sends the mail.

Am I missing something?

Thanks!

EDIT: I notice you say 'send the object in the $headers param', which would work, except how can I ensure this happens in plugins that I did not write, for example, with WooCommerce? It seems like there should be a way to set default headers via the wp_mail filter and not just when it is passed via the object.

sebastianplesciuc commented 7 years ago

Hi.

This approach will not work for code that is already using wp_mail() and it's not directly implemented by you.

The reason we're trying to move away from this is because in the next versions we will slowly try to move to the SendGrid v3 API since v2 will be deprecated in the future. As such, we won't add support for this just yet. We'll keep this in mind for future versions.