Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the Mailer component #30741

Merged
merged 1 commit into from Mar 30, 2019

Conversation

@fabpot
Copy link
Member

commented Mar 28, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR upcoming

https://speakerdeck.com/fabpot/mailer

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 28, 2019

@fabpot fabpot force-pushed the fabpot:mailer branch 2 times, most recently from 6c4dbb0 to 9ef1fd9 Mar 29, 2019

@fabpot fabpot force-pushed the fabpot:mailer branch 2 times, most recently from 5dcc1ed to 32380c8 Mar 29, 2019

@fabpot fabpot force-pushed the fabpot:mailer branch from 32380c8 to e37ebca Mar 29, 2019

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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

@fancyweb

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

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

@fabpot fabpot force-pushed the fabpot:mailer branch 2 times, most recently from 51f739e to 299b53b Mar 29, 2019

@fabpot

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

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

@fabpot fabpot force-pushed the fabpot:mailer branch from 299b53b to 9f94a21 Mar 29, 2019

@fabpot fabpot force-pushed the fabpot:mailer branch 2 times, most recently from bec8e85 to 6436f75 Mar 29, 2019

@fabpot

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

Tests are green now.

@Taluu

Taluu approved these changes Mar 29, 2019

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

This comment has been minimized.

Copy link
@Taluu

Taluu Mar 29, 2019

Contributor

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 as symfony/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. :}

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 30, 2019

Member

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.

This comment has been minimized.

Copy link
@Taluu

Taluu Mar 30, 2019

Contributor

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 register any transport and use it through its DSN.

E.g for the mailjet thing I linked, have the possibility to use mailjet://... or api://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. :}

This comment has been minimized.

Copy link
@sstok

sstok Mar 30, 2019

Contributor

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. Like https://github.com/rollerworks/search/blob/master/lib/Core/Loader/InputProcessorLoader.php#L46

This comment has been minimized.

Copy link
@Taluu

Taluu Mar 30, 2019

Contributor

Yep, sorry I meant the factory strategy indeed.

@fabpot fabpot force-pushed the fabpot:mailer branch 5 times, most recently from 061332e to 0c4a417 Mar 30, 2019

@nicolas-grekas
Copy link
Member

left a comment

Let's go and iterate!

@fabpot fabpot force-pushed the fabpot:mailer branch from 0c4a417 to 69b9ee7 Mar 30, 2019

@fabpot fabpot merged commit 69b9ee7 into symfony:master Mar 30, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 30, 2019

feature #30741 Add the Mailer component (fabpot)
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     <!-- see https://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        | upcoming

https://speakerdeck.com/fabpot/mailer

Commits
-------

69b9ee7 added the Mailer component

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot deleted the fabpot:mailer branch May 8, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.