[Mailer] Sendgrid API Transport: allow use of mail_settings #42915
Conversation
6f80065 to
5634789
Compare
|
Hey! I think @versgui has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
|
I'm also not a big fan of using a fake header. In any case, we don't add specific features of providers as we want devs to be able to switch from one provider to another one. But if this feature could be added to many providers, then, let's think about a proper way to support it. But the first step is to really understand if this can be supported by more than just Sendgrid. |
|
@fabpot @nicolas-grekas I think a less hacky version would require adding a new optional property to the |
8cf95ef to
80e4083
Compare
80e4083 to
54e8c9c
Compare
|
A better solution that's extensible for other transport providers. |
|
@fabian @nicolas-grekas any thoughts on this? |
|
I'm still not sure this is something we want to support. In any case, this is probably not something that can be part of the Envelope. |
4b7cd65 to
c009262
Compare
|
@nicolas-grekas Thanks for your comments, I've addressed your feedback. |
|
Friendly ping @nicolas-grekas and @fabpot. What do you think of @mikefox's implementation? I can help with this PR. I would also like to be able to use the EDIT: Eventually did it with a decorated HttpClient and a CompilerPass: https://gist.github.com/MaximePinot/85ef21d21f06ada31c097709769eb084 |
There was a problem hiding this comment.
Here are some comments. This PR is related to #41714
@fabpot I think we need to figure out a way for Mailer to support those features. Eg templates: I don't think we can abstract them in the component, but support for them looks desired. The code is almost capable of doing it. Can we reconsider the rejection of such changes, as done in #41714?
| @@ -1,6 +1,12 @@ | |||
| CHANGELOG | |||
| ========= | |||
|
|
|||
| 6.1 | |||
| 6.1 | ||
| --- | ||
|
|
||
| * Check the `Envelope` options for a "mail_settings" property and send the |
There was a problem hiding this comment.
missing space at head of line
the description is misleading I think: the code doesn't check for mail_settings anywhere
| @@ -145,6 +145,10 @@ private function getPayload(Email $email, Envelope $envelope): array | |||
| $personalization['custom_args'] = $customArguments; | |||
| } | |||
|
|
|||
| foreach ($envelope->getOptions() as $name => $value) { | |||
| $payload[$name] = $value; | |||
There was a problem hiding this comment.
This allows overriding any pre-existing payload key. This might be too risky.
There was a problem hiding this comment.
Should we do this instead?
| $payload[$name] = $value; | |
| if (!array_key_exists($name, $payload) { | |
| $payload[$name] = $value; | |
| } |
| @@ -1,6 +1,11 @@ | |||
| CHANGELOG | |||
| ========= | |||
|
|
|||
| 6.1 | |||
| 6.1 | ||
| --- | ||
|
|
||
| * Added the `options` property to `Envelope` |
568a523 to
21063b6
Compare
The Sendgrid API allows filters to be sent via the
mail_settingsfield when sending an email. This change allows users of the Sendgrid API mail transport to use this setting by setting amail_settingstext header on an email. This will be transformed into an array value that's sent as the mail_setting parameter.