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

Make Mailgun Header compatible with other Bridges#41380

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
nicolas-grekas merged 1 commit intosymfony:5.2fromjderusse:mailgun-header
May 23, 2021

Conversation

@jderusse
Copy link
Member

QA
Branch?5.2
Bug fix?no
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

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

See:

@carsonbotcarsonbot added this to the5.2 milestoneMay 22, 2021
@Nyholm
Copy link
Member

I think you misunderstand.

In 5.0, all headers was prefixed withh:. 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 headerh: foobar they have to add headerh: foobar. Just addingfoobar should not be enough.

OskarStark reacted with thumbs up emoji

@jderusse
Copy link
MemberAuthor

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

What's if I implement aFallBackTransport 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.

Copy link
Member

@NyholmNyholm 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 theh: was for arbitrary headers. But I see fromthe docs thatReply-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 useo:,v:,h:, etc prefixes.

if (\in_array($prefix, ['h:','t:','o:','v:']) ||\in_array($name, ['recipient-variables','template','amp-html'])) {
$headerName =$name;
}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
MemberAuthor

Hm.. I was convinced that theh: was for arbitrary headers. But I see fromthe docs thatReply-To is such header.

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

Copy link
Member

@NyholmNyholm 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

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

Nyholm reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Thank you@jderusse.

@nicolas-grekasnicolas-grekas merged commit019b2c6 intosymfony:5.2May 23, 2021
derrabus added a commit that referenced this pull requestMay 23, 2021
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)#40643But removed was not needed because reverted here (5.2)#41380Commits-------dc5c28d Remove deprecation dependency
This was referencedMay 31, 2021
nicolas-grekas added a commit that referenced this pull requestJun 20, 2025
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       | MITthis was added in#36148 and the deprecation was later on reverted in#41380Commits-------4b6be4a remove useless `@legacy` annotation
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@NyholmNyholmNyholm approved these changes

@chalasrchalasrchalasr approved these changes

Assignees

No one assigned

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
[8]ページ先頭
©2009-2025 Movatter.jp