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] added tag/metadata support#35050
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Replying to#35047 (comment): My experience is only with Postmark but I was able to find a similar concept for these 3rd party providers: Postmark Mailgun SendGrid Amazon |
AndreiIgna commentedDec 20, 2019
This is great and much better implementation than#34766 I was focusing on Postmark (using this atm) and that code targets only that transport. PostmarkApiTransport doesn't have a |
I have thought of some alternate solutions to account for the provider differences. Both would require knowledge of the transport when creating the email (which doesn't seem unreasonable).
My vote would be foroption 2. |
AndreiIgna commentedDec 20, 2019
Hmm I think option 2 adds much more complexity. For example, with option 2 emails are created and sent by adding specific tags "PostmarkTag/PostmarkHeader". If email transport is changed later for a project, instead of changing the project's config now it requires code changes too. More generic |
kbond commentedDec 20, 2019 • 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.
It looks like SendGrid has more options than just tags and metadata, they have personalizations and substitutions. The current PR implementation would not be able to provide these options. Option 1 would but if you switched providers, it would likely silently break in a way that isn't obvious. Option 2 would also break but in a more obvious way (there would be an exception because the header class would be missing). Maybe the best course is to stick with the current PR implementation since multiple providers can support it and possibly add custom provider headers later? |
src/Symfony/Component/Mailer/Bridge/Mailgun/Transport/MailgunSmtpTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I have moved the new Header classes to the Mailer component per@fabpot's request. I updated the PR description. I have confirmed this works on Postmark but I don't use Mailgun. I'd appreciate someone with a Mailgun account to test. If this is acceptable, I will add tests. |
acasademont commentedJan 10, 2020 • 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.
Sendgrid user here, I'll take a look at this asap, it might fix#34700 |
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.
LGTM, minor comments (and a bug)
src/Symfony/Component/Mailer/Bridge/Mailgun/Transport/MailgunApiTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Mailer/Bridge/Mailgun/Transport/MailgunApiTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Mailer/Bridge/Mailgun/Transport/MailgunSmtpTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
I made the changes and added tests. The tests are failing on travis (deps=low). Can I get some direction on how to fix? Again, I don't feel confident in the Mailgun implementation as I can't do a live test but Postmark works as expected. |
Mandrill (Mailchimp) also support these features (multiple values are supported for tags). Here are their doc:
|
To fix the tests, you need to require at Mailer 5.1 for the bridges so that the new headers exist. |
Ok, travis has been fixed. |
kbond commentedJan 30, 2020 • 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.
Ok, I added Mandrill support for this feature. Again, I have not "live" tested this. Mailgun and Mandrill both have an |
@kbond I think having support in HttpTransport classes as you describe would be good for consistency. Last thing to do before merge :) |
Ok, added support for Mandrill/Mailgun Http Transports. I added 2 traits to reduce duplication. |
Thank you@kbond. |
This PR was merged into the 5.1-dev branch.Discussion----------[Mailer] added tag/metadata support| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets |Fix#35047| License | MIT| Doc PR | todoThis is an alternative to#34766 for adding tag and metadata support in a more generalized way.Most transports allow for open/click tracking headers - maybe this should be handled in a similar way?I added implementations for the Postmark (SMTP and API) and Mailgun (SMTP and API) transports. I can add others and tests/docs if this is acceptable.### Example:```phpuse Symfony\Component\Mailer\Header\MetadataHeader;use Symfony\Component\Mailer\Header\TagHeader;$email->getHeaders()->add(new TagHeader('password-reset'));$email->getHeaders()->add(new MetadataHeader('Color', 'blue'));$email->getHeaders()->add(new MetadataHeader('Client-ID', '12345'));```The Postmark/Mailgun providers will parse these into their own headers/payload. For transports that don't support tags/metadata, these are just added as custom headers:```X-Tag: password-resetX-Metadata-Color: blueX-Metadata-Client-ID: 12345```Commits-------f2cdafc [Mailer] added tag/metadata support
This PR was merged into the master branch.Discussion----------[Mailer] add tag and metadata docs| Q | A || -- | -- || Target branch | master/5.1 || Issue |closes#13017 || Feature PR |symfony/symfony#35050 |Commits-------424f35a [mailer] add tag and metadata docs
Uh oh!
There was an error while loading.Please reload this page.
This is an alternative to#34766 for adding tag and metadata support in a more generalized way.
Most transports allow for open/click tracking headers - maybe this should be handled in a similar way?
I added implementations for the Postmark (SMTP and API) and Mailgun (SMTP and API) transports. I can add others and tests/docs if this is acceptable.
Example:
The Postmark/Mailgun providers will parse these into their own headers/payload. For transports that don't support tags/metadata, these are just added as custom headers: