8000 Make Mailgun Header compatible with other Bridges by jderusse · Pull Request #41380 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Make Mailgun Header compatible with other Bridges #41380

N 8000 ew 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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 23, 2021
Merged

Make Mailgun Header compatible with other Bridges #41380

merged 1 commit into from
May 23, 2021

Conversation

jderusse
Copy link
Member
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:

@carsonbot carsonbot added this to the 5.2 milestone May 22, 2021
@Nyholm
Copy link
Member
Nyholm commented May 22, 2021

I think you misunderstand.

In 5.0, all headers was prefixed with h:. That made it not possible to use other valid prefixes. So we should remove the automatic prefixing and tell the user that if they want header h: foobar they have to add header h: foobar. Just adding foobar should not be enough.

@jderusse
Copy link
Member Author

I think the issue is, that make the bridge incompatible with all other bridges because headers have to be formatted in a Mailgun way.

What's if I implement a FallBackTransport that use Mailgun and then fallback to Amazon in case or error or whatever? The original email can't have a header compatible with both formats.

Nyholm
Nyholm reviewed May 22, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Hm.. I was convinced that the h: was for arbitrary headers. But I see from the docs that Reply-To is such header.

Yeah, I think it make sense to not deprecate this. The fix introduced in #36148 will still make sure one can use o:, v:, h:, etc prefixes.

src/Symfony/Component/Mailer/Bridge/Mailgun/Transport/MailgunApiTransport.php Outdated
@@ -137,7 +137,6 @@ private function getPayload(Email $email, Envelope $envelope): array
} else {
// fallback to prefix with "h:" to not break BC
Copy link
Member

Choose a reason for hiding this comment

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

This comment should be fixed

@jderusse
Copy link
Member Author

Hm.. I was convinced that the h: was for arbitrary headers. But I see from the docs that Reply-To is such header.

I was convinced by the exact same sentence in their doc 😉

Nyholm
Nyholm approved these changes May 22, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

Are you sure this should be to 5.2 and not 5.3?

@stof
Copy link
Member
stof commented May 22, 2021

@Nyholm to me, that's a bugfix. The current deprecation is buggy

@jderusse jderusse mentioned this pull request May 23, 2021
Merged
@nicolas-grekas
Copy link
Member

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit 019b2c6 into symfony:5.2 May 23, 2021
@jderusse jderusse mentioned this pull request May 23, 2021
Merged
derrabus added a commit that referenced this pull request May 23, 2021
@derrabus
minor #41389 [Mailer] Remove deprecation dependency (jderusse)
66ef835 
This PR was merged into the 5.3 branch.

Discussion
----------

[Mailer] Remove deprecation dependency

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

Introduced here (5.3) #40643
But removed was not needed because reverted here (5.2) #41380

Commits
-------

dc5c28d Remove deprecation dependency
This was referenced May 31, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
6 participants
@jderusse @Nyholm @stof @nicolas-grekas @chalasr @carsonbot
0