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

Add the Mailer component#30741

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
fabpot merged 1 commit intosymfony:masterfromfabpot:mailer
Mar 30, 2019
Merged

Add the Mailer component#30741

fabpot merged 1 commit intosymfony:masterfromfabpot:mailer
Mar 30, 2019

Conversation

@fabpot
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRupcoming

https://speakerdeck.com/fabpot/mailer

onEXHovia, wadjeroudi, welcoMattic, Korbeil, lyrixx, ro0NL, BoShurik, chindit, ping-localhost, mms-uret, and 22 more reacted with thumbs up emojiping-localhost, Taluu, javiereguiluz, yceruto, daFish, vudaltsov, TomasPilar, sstok, derrabus, jverdeyen, and 10 more reacted with hooray emojilinaori, andrewmy, rawdreeg, chindit, ping-localhost, javiereguiluz, yceruto, daFish, derrabus, frankb01, and 9 more reacted with heart emojisstok, yceruto, kaznovac, Capenus, and1truong, decima, chalasr, mcsky, and jkobus reacted with rocket emoji
@fabpotfabpotforce-pushed themailer branch 2 times, most recently from6c4dbb0 to9ef1fd9CompareMarch 29, 2019 08:16
@fabpotfabpotforce-pushed themailer branch 2 times, most recently from5dcc1ed to32380c8CompareMarch 29, 2019 11:23
@javiereguiluz
Copy link
Member

In case you missed it, "AMP for mail" is now a reality. I'm not asking to add support for AMP ... but we should review that the current architecture doesn't prevent from adding AMP support in the future (in core or via extensions).

Specifically:

Important things to note:

The text/x-amp-html part must be nested under a multipart/alternative node, it will not be recognized by the email client otherwise.

Some email clients will only render the last MIME part, so we recommend placing the text/x-amp-html MIME part before the text/html MIME part.

Read this for all the tech details:https://www.ampproject.org/docs/interaction_dynamic/amp-email-format

ro0NL reacted with thumbs up emoji

@fancyweb
Copy link
Contributor

@fabpot

What is our plan when a provider deprecate an usage / remove it or change a default configuration ? Shouldn't there be a version system somewhere in the provider bridges ?

Also, what is the criteria for a provider to be in SF core ? I think it should be clear.

@fabpotfabpotforce-pushed themailer branch 2 times, most recently from51f739e to299b53bCompareMarch 29, 2019 15:24
@fabpot
Copy link
MemberAuthor

fabpot commentedMar 29, 2019
edited
Loading

@javiereguiluz I've just read the AMP for email page and the good news is that we are compatible. One needs to add the AMP part "manually", but that's a great use case for using the low-level interface of the component. I don't think that AMP should be a first-class citizen of the component though.

javiereguiluz and sstok reacted with thumbs up emoji

@fabpotfabpotforce-pushed themailer branch 2 times, most recently frombec8e85 to6436f75CompareMarch 29, 2019 15:56
@fabpot
Copy link
MemberAuthor

Tests are green now.

$pass =\urldecode($parsedDsn['pass'] ??'');
\parse_str($parsedDsn['query'] ??'',$query);

switch ($parsedDsn['host']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't look at other places, but I think it is too bad that this is a "fixed" list, there are still problems such assymfony/swiftmailer-bundle#217. So if your mail provider / transport is not implemented here, you can't have a dsn-like custom transport.

I could be wrong though, and that'd be great. :}

Choose a reason for hiding this comment

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

wow, lots of negative statements here :( what would be great? you didn't explain your idea? on my side, I don't know how this list could be otherwise than fixed.

Copy link
Contributor

@TaluuTaluuMar 30, 2019
edited
Loading

Choose a reason for hiding this comment

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

Not a negative comment, it's just something I would have liked to have. :}

I would have liked something like an application of the strategy design pattern, e.g have a bunch of transports (as we have now), which each tells if they support a host / a scheme (a dsn actually), so I can registerany transport and use it through its DSN.

E.g for the mailjet thing I linked, have the possibility to usemailjet://... orapi://mailjet.... at least. Currently (if I understood it right), you can't and are bound to use one of the supported transports...

Once again, I may be wrong. Anyway, even without that, the component is awesome. :}

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean like a factory strategy? I agree, but this is more about usage without the Framework I guess done a helper class rather than something that is fully concrete.

Maybe the static method can be kept for the standalone but allow the registering of other transporters. Likehttps://github.com/rollerworks/search/blob/master/lib/Core/Loader/InputProcessorLoader.php#L46

Taluu reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, sorry I meant the factory strategy indeed.

@fabpotfabpotforce-pushed themailer branch 5 times, most recently from061332e to0c4a417CompareMarch 30, 2019 07:58
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Let's go and iterate!

@fabpotfabpot merged commit69b9ee7 intosymfony:masterMar 30, 2019
fabpot added a commit that referenced this pull requestMar 30, 2019
This PR was merged into the 4.3-dev branch.Discussion----------Add the Mailer component| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | n/a| License       | MIT| Doc PR        | upcominghttps://speakerdeck.com/fabpot/mailerCommits-------69b9ee7 added the Mailer component
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot deleted the mailer branchMay 8, 2019 07:56
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@derrabusderrabusderrabus requested changes

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+10 more reviewers

@SerkanYildizSerkanYildizSerkanYildiz left review comments

@sstoksstoksstok requested changes

@ro0NLro0NLro0NL left review comments

@TaluuTaluuTaluu approved these changes

@onEXHoviaonEXHoviaonEXHovia left review comments

@fancywebfancywebfancyweb left review comments

@tony-trantony-trantony-tran left review comments

@noniagriconomienoniagriconomienoniagriconomie left review comments

@ValouleloupValouleloupValouleloup left review comments

@error56error56error56 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

16 participants

@fabpot@javiereguiluz@fancyweb@Taluu@nicolas-grekas@SerkanYildiz@stof@sstok@ro0NL@derrabus@onEXHovia@tony-tran@noniagriconomie@Valouleloup@error56@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp