Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Mailer] Sendgrid API Transport: allow use of mail_settings#42915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
[Mailer] Sendgrid API Transport: allow use of mail_settings#42915
Uh oh!
There was an error while loading.Please reload this page.
Conversation
6f80065 to5634789Comparecarsonbot commentedSep 8, 2021
Hey! I think@versgui has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Passing this as a fake header is a bit hacky I suppose, but I don't have a better idea. Can you please add some test cases? And can you also please send a PR for the doc?
Uh oh!
There was an error while loading.Please reload this page.
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 to80e4083Compare80e4083 to54e8c9cComparemikefox commentedOct 8, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
4b7cd65 toc009262Compare@nicolas-grekas Thanks for your comments, I've addressed your feedback. |
MaximePinot commentedJul 5, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
6.2 now
| 6.1 | ||
| --- | ||
| * Check the `Envelope` options for a "mail_settings" property and send the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing space at head of line
the description is misleading I think: the code doesn't check formail_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This allows overriding any pre-existing payload key. This might be too risky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we do this instead?
| $payload[$name] =$value; | |
| if (!array_key_exists($name,$payload) { | |
| $payload[$name] =$value; | |
| } |
| @@ -1,6 +1,11 @@ | |||
| CHANGELOG | |||
| ========= | |||
| 6.1 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
6.2
| 6.1 | ||
| --- | ||
| * Added the `options` property to `Envelope` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing space
568a523 to21063b6Compare
Uh oh!
There was an error while loading.Please reload this page.
The Sendgrid APIallows 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.