-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Mailer] Added Sendinblue bridge #38277
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
Conversation
src/Symfony/Component/Mailer/Bridge/Sendinblue/Transport/SendinblueApiTransport.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Mailer/Bridge/Sendinblue/Transport/SendinblueTransportFactory.php
Outdated
Show resolved
Hide resolved
1d3c246
to
82e19b0
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.
You also need to register the transport in src/Symfony/Bundle/FrameworkBundle/Resources/config/mailer_transports.xml
, and add it to src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
, src/Symfony/Component/Mailer/Exception/UnsupportedSchemeException.php
, and src/Symfony/Component/Mailer/Transport.php
as well.
CHANGELOG | ||
========= | ||
|
||
5.3.0 |
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.
Could be 5.2 as I think we can add it in 5.2 in time.
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 have changed this also for symfony-docs and recipe.
], | ||
"require": { | ||
"php": ">=7.2.5", | ||
"symfony/mailer": "^4.4|^5.0" |
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.
Should be ^5.1 (as you are using MetadataHeader which is not part of earlier versions)
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.
Done.
self::SENDINBLUE_TLS_ENABLED, | ||
$dispatcher, | ||
$logger | ||
); |
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.
Can be on one line
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.
Done.
/** | ||
* @author Yann LUCAS | ||
*/ | ||
final class SendinblueSmtpsTransport extends EsmtpTransport |
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.
This class should not be needed. The EsmtpTransport is able to determine the port automatically.
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 have removed this class.
$this->getPassword($dsn), | ||
$this->dispatcher, | ||
$this->logger | ||
); |
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.
should be on one line
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.
Done.
39f4f03
to
5f87d50
Compare
*/ | ||
final class SendinblueSmtpTransport extends EsmtpTransport | ||
{ | ||
const SENDINBLUE_SMTP_HOST = 'smtp-relay.sendinblue.com'; |
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.
These 3 consts can be inlined IMHO or at least be marked private.
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.
Yes i changed for inline values. There is no other usages for those constants.
1c01e0b
to
836a203
Compare
Thank you @drixs6o9. |
Thank you too. And also @nicolas-grekas and @noniagriconomie |
@drixs6o9 👍 i saw that this provider do also have an SMS endpoint maybe it could be added (if PR can be reviewed/merged like this one) :) |
Yes, it's a good idea. I'll see this soon. |
N.B. I had a little help from Pierre TONDEREAU for the API.