Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 30, 2020
Merged

[Mailer] added tag/metadata support #35050

merged 1 commit into from
Jan 30, 2020

Conversation

kbond
Copy link
Member

@kbond kbond commented Dec 19, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #35047
License MIT
Doc PR todo

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:

use 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-reset
X-Metadata-Color: blue
X-Metadata-Client-ID: 12345

@kbond
Copy link
Member Author

kbond commented Dec 19, 2019

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

@kbond
Copy link
Member Author

kbond commented Dec 20, 2019

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 single MetadataHeader that takes a free form array. Providers would have knowledge of how to parse this array. This would be added as a json encoded X-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((new PostmarkHeader())
        ->tag('password-reset')
        ->metadata([
            'Color' => 'blue',
            'Client-Id' => '12345',
        ])
    );

    Or multiple headers:

    $email->getHeaders()->add(new PostmarkTag('password-reset'));
    $email->getHeaders()->add(new PostmarkMetadata([
        '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 for option 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 generic Headers like Tag or Metadata are better in this case, and each transport uses this as it sees fit.

@kbond
Copy link
Member Author

kbond commented Dec 20, 2019

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
Member Author

kbond commented Jan 10, 2020

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
Copy link
Contributor

acasademont commented Jan 10, 2020

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

Copy link
Member

@fabpot fabpot 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)

@fabpot fabpot changed the title [POC][Mailer] added tag/metadata support [Mailer] added tag/metadata support Jan 30, 2020
@kbond
Copy link
Member Author

kbond commented Jan 30, 2020

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

stof commented Jan 30, 2020

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

@fabpot
Copy link
Member

fabpot commented Jan 30, 2020

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

@kbond
Copy link
Member Author

kbond commented Jan 30, 2020

Ok, travis has been fixed.

@fabpot
Copy link
Member

fabpot commented Jan 30, 2020

@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
Member Author

kbond commented Jan 30, 2020

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

Mailgun and Mandrill both have an HttpTransport - 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

fabpot commented Jan 30, 2020

@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
Member Author

kbond commented Jan 30, 2020

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

@fabpot
Copy link
Member

fabpot commented Jan 30, 2020

Thank you @kbond.

fabpot added a commit that referenced this pull request Jan 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        | todo

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:

```php
use 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-reset
X-Metadata-Color: blue
X-Metadata-Client-ID: 12345
```

Commits
-------

f2cdafc [Mailer] added tag/metadata support
@fabpot fabpot merged commit f2cdafc into symfony:master Jan 30, 2020
@kbond kbond deleted the mailer-metadata branch January 30, 2020 16:07
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Feb 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-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mailer] message metadata and tags
7 participants