Skip to content

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

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 30, 2019
Merged

Add the Mailer component #30741

merged 1 commit into from
Mar 30, 2019

Conversation

fabpot
Copy link
Member

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

@fabpot fabpot force-pushed the mailer branch 2 times, most recently from 6c4dbb0 to 9ef1fd9 Compare March 29, 2019 08:16
@fabpot fabpot force-pushed the mailer branch 2 times, most recently from 5dcc1ed to 32380c8 Compare March 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

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

@fabpot fabpot force-pushed the mailer branch 2 times, most recently from 51f739e to 299b53b Compare March 29, 2019 15:24
@fabpot
Copy link
Member Author

fabpot 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 mailer branch 2 times, most recently from bec8e85 to 6436f75 Compare March 29, 2019 15:56
@fabpot
Copy link
Member Author

fabpot commented Mar 29, 2019

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 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. :}

Copy link
Member

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

@Taluu Taluu Mar 30, 2019

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 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. :}

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

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.

@fabpot fabpot force-pushed the mailer branch 5 times, most recently from 061332e to 0c4a417 Compare March 30, 2019 07:58
Copy link
Member

@nicolas-grekas nicolas-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!

@fabpot fabpot merged commit 69b9ee7 into symfony:master Mar 30, 2019
fabpot added a commit that referenced this pull request Mar 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     <!-- 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 mailer branch May 8, 2019 07:56
@fabpot fabpot mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.