laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.7k stars 11.06k forks source link

Ability to pass additional API parameters to Mail Transporters #13447

Closed trevorgehman closed 8 years ago

trevorgehman commented 8 years ago

I'm glad to see SparkPost was added as a driver, but one issue I'm running into is that for sending transactional emails, the API requires you to send an additional parameter so that the "Unsubscribe" link is not present. Obviously for password resets and things like that, that would not be good. (See API docs: https://developers.sparkpost.com/api/#/reference/transmissions)

So I'm wondering, what is the best way to add this? Should we just add an options array into the mail.php configuration for SparkPost (and potentially others)?

'sparkpost' => [
    'secret' => 'your-sparkpost-key',
    'options' => [ 
        'transactional' => true,
    ],
],

Then we would have to change the SparkPostTransport class:

/**
 * Create a new SparkPost transport instance.
 *
 * @param  \GuzzleHttp\ClientInterface  $client
 * @param  string  $key
 * @return void
 */
public function __construct(ClientInterface $client, $key, $options = [])
{
    $this->client = $client;
    $this->key = $key;
    $this->options = $options;
}

/**
 * {@inheritdoc}
 */
public function send(Swift_Mime_Message $message, &$failedRecipients = null)
{
    $this->beforeSendPerformed($message);
    $recipients = $this->getRecipients($message);
    $message->setBcc([]);
    $options = [
        'headers' => [
            'Authorization' => $this->key,
        ],
        'json' => [
            'recipients' => $recipients,
            'options' => $this->options,
            'content' => [
                'html' => $message->getBody(),
                'from' => $this->getFrom($message),
                'reply_to' => $this->getReplyTo($message),
                'subject' => $message->getSubject(),
            ],
        ],
    ];
    return $this->client->post('https://api.sparkpost.com/api/v1/transmissions', $options);
}

Is something like this a good idea to build out? Am I missing anything? The only issue with this method is that I can imagine some scenarios where these additional API options would not be universal, but would need to be altered for specific emails. In that instance I'm not sure what to do...

pochocho commented 8 years ago

I'm looking for a way to do this also but for other options, specially "capaign_id" since it seems to be the only way to group messages in the Sparkpost UI to check performance.

But about the unsubscribe link you mention. I do all my templates in Laravel and the emails I send don't seem to have the unsubscribe link, are you building the template on sparkpost? or your list?

Anyhow I think being able to configure the API call a bit further is ideal but I don't think adding it to config is the answer, it should be set up on a per message basis I think. The thing is that I really don't know what the approach should be to add this to the transport since the transport gets created behind the scenes when the mailer singleton is created. If anyone can shed some light on this I would gladly appreciate.

pochocho commented 8 years ago

so after playing around with this, I found a way to get extra parameters in but its for sure very hacky and not expressive at all. Any suggestions on doing this differently?

What I did was set a header on the message (the header from the message is discarded when sending through sparkpost's API... I think so at least, correct me if I'm wrong)

So here is what I did:

On the Send I set up the header

return Mail::send('emails.welcome',$data, function($message) use($user){

        $message->to('email@host.com')
            ->subject('welcome');

        $headers = $message->getHeaders();
        $headers->addTextHeader('campaign-id', 'test_campaign');

    });

then on SparkPostTransport:

public function send(Swift_Mime_Message $message, &$failedRecipients = null)
{
    $this->beforeSendPerformed($message);
    $recipients = $this->getRecipients($message);
    $message->setBcc([]);
    $options = [
        'headers' => [
            'Authorization' => $this->key,
        ],
        'json' => [
            'recipients' => $recipients,
            'campaign_id' => $message->getHeaders()->has('campaign-id') ? $message->getHeaders()->get('campaign-id') : null,
            'content' => [
                'html' => $message->getBody(),
                'from' => $this->getFrom($message),
                'reply_to' => $this->getReplyTo($message),
                'subject' => $message->getSubject(),
            ],
        ],
    ];
    return $this->client->post('https://api.sparkpost.com/api/v1/transmissions', $options);
}

Obviously this workaround would not work for arrays so configuring the whole options this way would not be favorable. It works in my particular scenario because I really only need the campaign_id but would like to find a solution that gives some flexibility to add parameters into transport

OzanKurt commented 7 years ago

@pochocho How did you overwrite SparkPostTransport?

jeff-h commented 7 years ago

I hope to use Sparkpost's web hook mechanism to have them send message event data back to Laravel, but for that to be useful I need a way of tying the original "send email" request to the event data I receive back via the web hook.

Sparkpost provide a 'metadata' API option so you can send a msg id with the original send-email request, which they then send back in each msg event, but as far as I can see Laravel 5.4 provides no way to send this metadata.

I also would be very interested in how you overwrote SparkPostTransport. It feels like Laravel 5.4 is rather rigid in its approach to mail transports :(