Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:masterfromkbond:mailer-metadata
Jan 30, 2020
Merged

[Mailer] added tag/metadata support#35050

fabpot merged 1 commit intosymfony:masterfromkbond:mailer-metadata
Jan 30, 2020

Conversation

kbond
Copy link
Member

@kbondkbond commentedDec 19, 2019
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#35047
LicenseMIT
Doc PRtodo

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:

useSymfony\Component\Mailer\Header\MetadataHeader;useSymfony\Component\Mailer\Header\TagHeader;$email->getHeaders()->add(newTagHeader('password-reset'));$email->getHeaders()->add(newMetadataHeader('Color','blue'));$email->getHeaders()->add(newMetadataHeader('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

AndreiIgna and IndraGunawan reacted with thumbs up emojifbourigault and jonleverrier reacted with heart emojiKoc reacted with rocket emoji
@kbond
Copy link
MemberAuthor

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
Tag: single value
Metadata: key/value pair

Mailgun
Tag: single value
Metadata: key/value pair, referred to as "variables"

SendGrid
Tag: multiple values, referred to as "categories"
Metadata: key/value pair, referred to as "custom_args"

Amazon
Looks like tags/metadata is combined as "Tags"

@AndreiIgna
Copy link

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 asendEmail test, maybe you can add the one from my PRhttps://github.com/symfony/symfony/pull/34766/files#diff-e50347a040c466692646cd1e6b7fe257

@kbond
Copy link
MemberAuthor

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).

  1. A singleMetadataHeader that takes a free form array. Providers would have knowledge of how to parse this array. This would be added as a json encodedX-Metadata header for providers that don't support this. This is the most generic solution but complex from a usage perspective.

    $email->getHeaders()->addMetadata(['tag' =>'password-reset','metadata' => ['Color' =>'blue','Client-Id' =>'12345',    ],]);
  2. Each provider can define it's own header(s) (that would live in the bridge):PostmarkHeader,MailgunHeader, etc.

    $email->getHeaders()->add((newPostmarkHeader())    ->tag('password-reset')    ->metadata(['Color' =>'blue','Client-Id' =>'12345',    ]));

    Or multiple headers:

    $email->getHeaders()->add(newPostmarkTag('password-reset'));$email->getHeaders()->add(newPostmarkMetadata(['Color' =>'blue','Client-Id' =>'12345',]));

    This would allow us to utilize all the different features of each provider in a way that is easy for the user to use.

My vote would be foroption 2.

@AndreiIgna
Copy link

Hmm I think option 2 adds much more complexity.
I think option 1 or current implementation in PR would work better, because it doesn't require custom code for providers in app's code.

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.
At the moment I use Postmark for a project, but may change this to Sendgrid later. I'm using Mailer exactly for this reason, to change only the config once, and don't worry about how emails are created.

More genericHeaders like Tag or Metadata are better in this case, and each transport uses this as it sees fit.

@kbond
Copy link
MemberAuthor

kbond commentedDec 20, 2019
edited
Loading

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?

@kbond
Copy link
MemberAuthor

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.

OskarStark reacted with thumbs up emoji

@acasademont
Copy link
Contributor

acasademont commentedJan 10, 2020
edited
Loading

Sendgrid user here, I'll take a look at this asap, it might fix#34700

kbond reacted with heart emoji

Copy link
Member

@fabpotfabpot left a 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)

@fabpotfabpot changed the title[POC][Mailer] added tag/metadata support[Mailer] added tag/metadata supportJan 30, 2020
@kbond
Copy link
MemberAuthor

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.

@stof
Copy link
Member

Mandrill (Mailchimp) also support these features (multiple values are supported for tags). Here are their doc:

@fabpot
Copy link
Member

To fix the tests, you need to require at Mailer 5.1 for the bridges so that the new headers exist.

@kbond
Copy link
MemberAuthor

Ok, travis has been fixed.

@fabpot
Copy link
Member

@kbond Can you try to do the same for Mandrill with the docs suggested by@stof? It not, happy to merge as is and let someone else take over for the other transport providers.

@kbond
Copy link
MemberAuthor

kbond commentedJan 30, 2020
edited
Loading

Ok, I added Mandrill support for this feature. Again, I have not "live" tested this.

Mailgun and Mandrill both have anHttpTransport - this PR does not add tag/metadata support to these. Since these transports send the raw headers, it should work if using the proper header names for each service.edit: it should be possibly to add support to these using the same logic as the SmtpTransports, let me know if you'd like me to add before merging.

@fabpot
Copy link
Member

@kbond I think having support in HttpTransport classes as you describe would be good for consistency. Last thing to do before merge :)

@kbond
Copy link
MemberAuthor

Ok, added support for Mandrill/Mailgun Http Transports. I added 2 traits to reduce duplication.

@fabpot
Copy link
Member

Thank you@kbond.

kbond reacted with hooray emoji

fabpot added a commit that referenced this pull requestJan 30, 2020
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
@fabpotfabpot merged commitf2cdafc intosymfony:masterJan 30, 2020
@kbondkbond deleted the mailer-metadata branchJanuary 30, 2020 16:07
OskarStark added a commit to symfony/symfony-docs that referenced this pull requestFeb 5, 2020
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
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

[Mailer] message metadata and tags
7 participants
@kbond@AndreiIgna@acasademont@stof@fabpot@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp