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

[Mailer] New Brevo mailer bridge (formerly Sendinblue) #50302

Merged
merged 1 commit into from Jul 9, 2023

Conversation

PEtanguy
Copy link
Contributor

@PEtanguy PEtanguy commented May 11, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR TODO

Hello,

This PR is aimed at updating the config for the sendinblue mailer.
As you might have seen, Sendinblue has rebranded to Brevo and also rewrote their apis.
This change ensure compatibility with the new endpoints and removes any reference to Sendinblue.

This is the notifier PR: #50296

@carsonbot carsonbot added this to the 6.3 milestone May 11, 2023
@PEtanguy PEtanguy changed the title New Brevo mailer bridge (formerly Sendinblue) [Mailer] New Brevo mailer bridge (formerly Sendinblue) May 11, 2023
}

/**
* @return array<int, string>
Copy link
Member

Choose a reason for hiding this comment

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

the return type is actually list<array{email: string, name?: string}>

/**
* @return array<int, string>
*/
protected function stringifyAddresses(array $addresses): array
Copy link
Member

Choose a reason for hiding this comment

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

However the protected method of the AbstractTransport with a different return type is a bad idea. This should use a different method name for the needs of this class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatAddresses() ?

Copy link
Member

Choose a reason for hiding this comment

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

this looks like a good name

Copy link
Member

Choose a reason for hiding this comment

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

@PEtanguy you haven't replaced this method

src/Symfony/Component/Mailer/Bridge/Brevo/composer.json Outdated Show resolved Hide resolved
src/Symfony/Component/Mailer/Bridge/Brevo/composer.json Outdated Show resolved Hide resolved
@carsonbot carsonbot changed the title [Mailer] New Brevo mailer bridge (formerly Sendinblue) New Brevo mailer bridge (formerly Sendinblue) May 12, 2023
@@ -76,7 +76,7 @@ protected function doSendApi(SentMessage $sentMessage, Email $email, Envelope $e
/**
* @return list<array{email: string, name?: string}>
*/
protected function stringifyAddresses(array $addresses): array
protected function formatAddresses(array $addresses): array
Copy link
Member

Choose a reason for hiding this comment

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

should be a private method as it does not try to overwrite the parent method anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@PEtanguy PEtanguy force-pushed the new_mailer_bridge_brevo branch 2 times, most recently from a3a2944 to 72a6287 Compare May 24, 2023 20:45
@PEtanguy
Copy link
Contributor Author

PEtanguy commented May 24, 2023

I will make the documentation once this is merged. I also noticed an error in the doc of another transport.

@OskarStark OskarStark changed the title New Brevo mailer bridge (formerly Sendinblue) [Mailer] New Brevo mailer bridge (formerly Sendinblue) Jun 14, 2023
6.3
---

* Added the bridge as a replacement of the deprecated Sendinblue one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Added the bridge as a replacement of the deprecated Sendinblue one.
* Add the bridge

Copy link
Member

Choose a reason for hiding this comment

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

@PEtanguy you forgot this change when fixing requested things

@OskarStark
Copy link
Contributor

OskarStark commented Jun 14, 2023

I would not touch the other bridge in this PR; let's just add this as a new bridge. No need to deprecate the code IMHO, we can abandon the whole package via packagist.

WDYT @fabpot ?

@stof
Copy link
Member

stof commented Jun 14, 2023

Well, triggering a deprecation when someone configures a mailer DSN using the sendinblue schemes is needed to help them migrate.

If we only abandon the package, projects removing the old bridge without updating their env variables in prod would get a broken project due to using an unsupported scheme.
Side note: I would even wait a bit before marking the package as abandoned as FrameworkBundle 5.4 and 6.3 won't support the Brevo bridge (it does not register it as a supported factory) while the SendinblueBridge technically still works thanks to the BC layer of the API itself (where the old domain still works). We cannot mark a package as abandoned only for some versions and not for others.

@PEtanguy
Copy link
Contributor Author

I agree with @stof , we can abandon Sendinblue later. I will keep the deprecations on this PR and on the one of the notifier (#50296)

fabpot added a commit that referenced this pull request Jul 9, 2023
…anguy)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Notifier] Add Brevo bridge (formerly Sendinblue)

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | TODO

Hello,

This PR is aimed at updating the config for the sendinblue notifier.
As you might have seen, Sendinblue has rebranded to [Brevo](https://developers.brevo.com/) and also rewrote their apis.
This change ensure compatibility with the new endpoints and removes any reference to Sendinblue.

This is the mailer PR: #50302

Commits
-------

731f9b0 [Notifier] Add Brevo bridge (formerly Sendinblue)
symfony-splitter pushed a commit to symfony/notifier that referenced this pull request Jul 9, 2023
…anguy)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Notifier] Add Brevo bridge (formerly Sendinblue)

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | TODO

Hello,

This PR is aimed at updating the config for the sendinblue notifier.
As you might have seen, Sendinblue has rebranded to [Brevo](https://developers.brevo.com/) and also rewrote their apis.
This change ensure compatibility with the new endpoints and removes any reference to Sendinblue.

This is the mailer PR: symfony/symfony#50302

Commits
-------

731f9b02ef [Notifier] Add Brevo bridge (formerly Sendinblue)
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jul 9, 2023
…anguy)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Notifier] Add Brevo bridge (formerly Sendinblue)

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | TODO

Hello,

This PR is aimed at updating the config for the sendinblue notifier.
As you might have seen, Sendinblue has rebranded to [Brevo](https://developers.brevo.com/) and also rewrote their apis.
This change ensure compatibility with the new endpoints and removes any reference to Sendinblue.

This is the mailer PR: symfony/symfony#50302

Commits
-------

731f9b02ef [Notifier] Add Brevo bridge (formerly Sendinblue)
@fabpot
Copy link
Member

fabpot commented Jul 9, 2023

Thank you @PEtanguy.

@fabpot fabpot merged commit 710e16c into symfony:6.4 Jul 9, 2023
6 of 9 checks passed
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 9, 2023
This PR was merged into the 6.4 branch.

Discussion
----------

[Mailer] Rename Sendinlue to Brevo

Sendinblue mailer transport was deprecated in Symfony 6.4, new transport Brevo was created due to the service name change.

https://www.brevo.com/fr/blog/pourquoi-brevo/

PR : symfony/symfony#50302

Commits
-------

10ef904 [Mailer] Rename Sendinlue to Brevo
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jul 28, 2023
…anguy)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Notifier] Add Brevo bridge (formerly Sendinblue)

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | TODO

Hello,

This PR is aimed at updating the config for the sendinblue notifier.
As you might have seen, Sendinblue has rebranded to [Brevo](https://developers.brevo.com/) and also rewrote their apis.
This change ensure compatibility with the new endpoints and removes any reference to Sendinblue.

This is the mailer PR: symfony/symfony#50302

Commits
-------

731f9b02ef [Notifier] Add Brevo bridge (formerly Sendinblue)
@xabbuh
Copy link
Member

xabbuh commented Aug 15, 2023

#51295 should be enough, right?

@thomas2411
Copy link
Contributor

#51295 should be enough, right?

Yes, thank you :)

This was referenced Oct 21, 2023
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.

None yet

8 participants