-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Add the Mailer component #30741
Conversation
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Mailer/Transport/Http/Api/AbstractApiTransport.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Mailer/Transport/Smtp/Auth/AuthenticatorInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Mailer/Bridge/Amazon/Http/SesTransport.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Mailer/Bridge/Sendgrid/Http/Api/SendgridTransport.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Mailer/Exception/HttpTransportException.php
Outdated
Show resolved
Hide resolved
6c4dbb0
to
9ef1fd9
Compare
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Mailer/Bridge/Amazon/Http/Api/SesTransport.php
Outdated
Show resolved
Hide resolved
5dcc1ed
to
32380c8
Compare
src/Symfony/Component/Mailer/Bridge/Mailgun/Http/Api/MailgunTransport.php
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
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:
Read this for all the tech details: https://www.ampproject.org/docs/interaction_dynamic/amp-email-format |
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. |
51f739e
to
299b53b
Compare
@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. |
bec8e85
to
6436f75
Compare
Tests are green now. |
$pass = \urldecode($parsedDsn['pass'] ?? ''); | ||
\parse_str($parsedDsn['query'] ?? '', $query); | ||
|
||
switch ($parsedDsn['host']) { |
There was a problem hiding this comment.
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. :}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
061332e
to
0c4a417
Compare
There was a problem hiding this 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!
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
https://speakerdeck.com/fabpot/mailer