10000 [Mailer][Mailgun] Support more headers by Nyholm · Pull Request #36148 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Mailer][Mailgun] Support more headers #36148

New issue 8000

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
Mar 23, 2020
Merged

Conversation

Nyholm
Copy link
Member
@Nyholm Nyholm commented Mar 20, 2020
Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Fix #35776
License MIT
Doc PR

Im not sure if this should be classified as a bug since "setting headers are broken". Ie, you cannot use some Mailgun API features.

Or, this may be a feature because now you may specify any supported custom header you want.

@greedyivan
Copy link
Contributor

How to set header without prefix?

As it was mentioned in the original issue, t:version and t:text mean nothing without the template header.

@Nyholm
Copy link
Member Author
Nyholm commented Mar 20, 2020

Ooops. I forgot those.

Is “template” and “amp-html” the only two ‘non-prefixed’ headers?

I think I’ll make an exception for those two then.

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 20, 2020
@greedyivan
Copy link
Contributor

Is “template” and “amp-html” the only two ‘non-prefixed’ headers?

recipient-variables, as well.

The current solution requires update every time new headers are added to Mailgun, as the current history of Mailgun tells us that this happens after a while.

It is just a matter of time before the same issue reappears.

@Nyholm
Copy link
Member Author
Nyholm commented Mar 22, 2020

Thank you @greedyivan for the review.

I've deprecated passing a header without the "h:" prefix. So we need to "keep up to date" with Mailguns API until Symfony 6.0.

@fabpot
Copy link
Member
fabpot commented Mar 23, 2020

Thank you @Nyholm.

@fabpot fabpot merged commit 0c22ab8 into symfony:master Mar 23, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
@akalineskou
Copy link
Contributor

I'm using Symfony\Component\Mime\Email to create the email:

$email
    ->from($from)
    ->replyTo($replyTo)
    ->to($to)
;

And I'm getting User Deprecated: Not prefixing the Mailgun header name with "h:" is deprecated since Symfony 5.1. Use header name "h:reply-to" instead.
I was wondering where should the h: addition to the reply-to happen.

@Nyholm
Copy link
Member Author
Nyholm commented Apr 23, 2021

Excellent question!

I suggest to open up a new issue for that. That way it wont get lost on a closed PR.

} else {
// fallback to prefix with "h:" to not break BC
$headerName = 'h:'.$name;
@trigger_error(sprintf('Not prefixing the Mailgun header name with "h:" is deprecated since Symfony 5.1. Use header name "%s" instead.', $headerName), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems weird to me. The h: prefix is a mailgun specific feature. Requiring the Email object to set it means writing Mailgun-specific code in a place that may not even be aware of which transport is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nyholm I suggest removing the deprecation warning

nicolas-grekas added a commit that referenced this pull request May 23, 2021
This PR was merged into the 5.2 branch.

Discussion
----------

Make Mailgun Header compatible with other Bridges

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

This revert deprecating passing a header without the required Mailgun `h:` prefix.
And makes the bridge compatible with other bridges.

See:
- #36148 (comment)
- #41308 (comment)

Commits
-------

82f1f9c Make Mailgun Header compatible with other Bridges
symfony-splitter pushed a commit to symfony/mailer that referenced this pull request Sep 28, 2021
This PR was merged into the 5.2 branch.

Discussion
----------

Make Mailgun Header compatible with other Bridges

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

This revert deprecating passing a header without the required Mailgun `h:` prefix.
And makes the bridge compatible with other bridges.

See:
- symfony/symfony#36148 (comment)
- symfony/symfony#41308 (comment)

Commits
-------

82f1f9c618 Make Mailgun Header compatible with other Bridges
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][Mailgun] Support more headers
8 participants
0