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

[Notifier] Add Brevo bridge (formerly Sendinblue) #50296

Merged
merged 1 commit into from Jul 9, 2023

Conversation

PEtanguy
Copy link
Contributor

@PEtanguy PEtanguy commented May 10, 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 notifier.
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 mailer PR: #50302

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@PEtanguy PEtanguy marked this pull request as ready for review May 10, 2023 17:10
@PEtanguy PEtanguy requested a review from OskarStark as a code owner May 10, 2023 17:10
@carsonbot carsonbot added this to the 6.3 milestone May 10, 2023
@PEtanguy PEtanguy force-pushed the new_notifier_bridge_brevo branch 2 times, most recently from 9a467e7 to 9622d22 Compare May 10, 2023 17:33
@OskarStark OskarStark changed the title New notifier bridge brevo [Notifier] Add brevo bridge (formerly Sendinblue) May 10, 2023
@OskarStark OskarStark changed the title [Notifier] Add brevo bridge (formerly Sendinblue) [Notifier] Add Brevo bridge (formerly Sendinblue) May 10, 2023
@stof
Copy link
Member

stof commented May 11, 2023

@PEtanguy are you also working on the mailer bridge ?

@PEtanguy
Copy link
Contributor Author

@stof yes i am waiting to merge this PR to open the Mailer one.
Do yo know how I can skip the failing tests by any chance?

The tests are failing because of the deprecation trigger

@stof
Copy link
Member

stof commented May 11, 2023

You need to mark tests as @group legacy when they test deprecated classes.

And maybe the deprecation need to be triggered only if the factory is actually used instead of triggering it as soon as it is autoloaded, in case FrameworkBundle registers all installed factories.

6.3
---

* Added the bridge
Copy link
Contributor

Choose a reason for hiding this comment

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

should you mentioned that this is the replacement of https://packagist.org/packages/symfony/sendinblue-notifier?
and also in the other composer, mark it as deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely!

@PEtanguy PEtanguy force-pushed the new_notifier_bridge_brevo branch 2 times, most recently from 3adf0d1 to ae9867b Compare May 11, 2023 16:54
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@PEtanguy PEtanguy force-pushed the new_notifier_bridge_brevo branch 2 times, most recently from c90b1cf to b4dcd18 Compare May 24, 2023 20:55
private string $apiKey;
private string $sender;

public function __construct(#[\SensitiveParameter] string $apiKey, string $sender, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use constructor property promotion for apiKey and sender

@PEtanguy PEtanguy force-pushed the new_notifier_bridge_brevo branch 3 times, most recently from f079a58 to 6746d3c Compare May 24, 2023 21:47
6.4
---

* Added the bridge
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
* Add the bridge

We can add an info about a replacement in the README; no need for it in the changeling file. From a code POV this is simply a new bridge

@@ -0,0 +1,25 @@
Brevo Notifier
===================
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
===================
==============

@@ -0,0 +1,25 @@
Brevo Notifier
===================

Copy link
Contributor

Choose a reason for hiding this comment

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

A note about the renaming can be put here

{
"name": "symfony/brevo-notifier",
"type": "symfony-notifier-bridge",
"description": "Symfony Brevo Notifier Bridge - formerly Sendinblue",
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
"description": "Symfony Brevo Notifier Bridge - formerly Sendinblue",
"description": "Symfony Brevo Notifier Bridge",

"require": {
"php": ">=8.1",
"symfony/http-client": "^5.4|^6.0",
"symfony/notifier": "^6.3"
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
"symfony/notifier": "^6.3"
"symfony/notifier": "^6.4"

Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

LGTM after my comments and the revert of src/Symfony/Component/Notifier/Bridge/Sendinblue/*

@fabpot fabpot force-pushed the new_notifier_bridge_brevo branch from 3053f74 to 731f9b0 Compare July 9, 2023 07:00
@fabpot
Copy link
Member

fabpot commented Jul 9, 2023

Thank you @PEtanguy.

@fabpot fabpot merged commit ce9bbd3 into symfony:6.4 Jul 9, 2023
5 of 9 checks passed
fabpot added a commit that referenced this pull request Jul 9, 2023
… (PEtanguy)

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

Discussion
----------

[Mailer] New Brevo mailer 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 mailer.
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 notifier PR: #50296

Commits
-------

f537358 [Mailer] New Brevo mailer bridge (formerly Sendinblue)
symfony-splitter pushed a commit to symfony/mailer that referenced this pull request Jul 9, 2023
… (PEtanguy)

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

Discussion
----------

[Mailer] New Brevo mailer 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 mailer.
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 notifier PR: symfony/symfony#50296

Commits
-------

f537358e5a [Mailer] New Brevo mailer bridge (formerly Sendinblue)
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jul 9, 2023
… (PEtanguy)

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

Discussion
----------

[Mailer] New Brevo mailer 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 mailer.
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 notifier PR: symfony/symfony#50296

Commits
-------

f537358e5a [Mailer] New Brevo mailer bridge (formerly Sendinblue)
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jul 28, 2023
… (PEtanguy)

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

Discussion
----------

[Mailer] New Brevo mailer 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 mailer.
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 notifier PR: symfony/symfony#50296

Commits
-------

f537358e5a [Mailer] New Brevo mailer bridge (formerly Sendinblue)
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

7 participants