Skip to content

Make Mailgun Header compatible with other Bridges#41380

Merged
nicolas-grekas merged 1 commit intosymfony:5.2from
jderusse:mailgun-header
May 23, 2021
Merged

Make Mailgun Header compatible with other Bridges#41380
nicolas-grekas merged 1 commit intosymfony:5.2from
jderusse:mailgun-header

Conversation

@jderusse
Copy link
Copy Markdown
Member

@jderusse jderusse commented May 22, 2021

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

jderusse commented May 22, 2021

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
Copy Markdown
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
Copy Markdown
Member

@Nyholm Nyholm May 22, 2021

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

jderusse commented May 22, 2021

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

nicolas-grekas commented May 23, 2021

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
@xabbuh xabbuh mentioned this pull request Jun 20, 2025
Merged
nicolas-grekas added a commit that referenced this pull request Jun 20, 2025
@nicolas-grekas
minor #60850 [Mailer] remove useless @legacy annotation (xabbuh)
6b52625 
This PR was merged into the 6.4 branch.

Discussion
----------

[Mailer] remove useless `@legacy` annotation

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

this was added in #36148 and the deprecation was later on reverted in #41380

Commits
-------

4b6be4a remove useless `@legacy` annotation
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