NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Notifier] add Mailjet SMS bridge#41705

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:5.4fromjnadaud:mailjet_notifier

Jul 4, 2021

Merged

[Notifier] add Mailjet SMS bridge#41705

fabpot merged 1 commit intosymfony:5.4fromjnadaud:mailjet_notifier

Jul 4, 2021

Conversation

Copy link

Contributor

jnadaud commentedJun 14, 2021•
edited by OskarStark
Loading

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT
Doc	symfony/symfony-docs#15436

Adding Mailjet SMS bridge for Notifier component

Copy link

carsonbot commentedJun 14, 2021

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

jnadaud mentioned this pull request

Jun 14, 2021

[Notifier] Add Mailjet Bridgesymfony/symfony-docs#15436

Closed

jnadaud force-pushed themailjet_notifier branch from4929c4d tof9fc518Compare

June 14, 2021 17:51

jnadaud marked this pull request as ready for review

June 14, 2021 18:06

jnadaud requested a review fromOskarStark as acode owner

June 14, 2021 18:06

carsonbot added Status: Needs Review Feature Notifier labels

Jun 14, 2021

Copy link

carsonbot commentedJun 14, 2021

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

jnadaud force-pushed themailjet_notifier branch 2 times, most recently from47803d2 to555a2d5Compare

June 14, 2021 18:53

94noni reviewed

Jun 14, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Mailjet/README.md OutdatedShow resolvedHide resolved

OskarStark reviewed

Jun 14, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.phpShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/Mailjet/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/Mailjet/LICENSE OutdatedShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/Mailjet/MailjetTransport.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/Mailjet/MailjetTransportFactory.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/Mailjet/README.md OutdatedShow resolvedHide resolved

jnadaud force-pushed themailjet_notifier branch from555a2d5 to62687b5Compare

June 15, 2021 02:16

nicolas-grekas modified the milestones:6.0,5.4

Jun 15, 2021

Copy link

Contributor

OskarStark commentedJun 15, 2021

It looks, likefrom is required anduser/password is not used, I would usefrom asuser andauthtoken aspass for the DSN, like explained here:#41522 (comment)

jnadaud force-pushed themailjet_notifier branch from1a0cc4a toca85797Compare

June 16, 2021 16:08

Copy link

ContributorAuthor

jnadaud commentedJun 16, 2021

@OskarStark Done!!!

jnadaud force-pushed themailjet_notifier branch fromca85797 to9e616c7Compare

June 16, 2021 16:14

OskarStark approved these changes

Jun 17, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jun 17, 2021

OskarStark reviewed

Jun 17, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Mailjet/MailjetTransport.php Outdated


		publicfunction__toString():string
		{
		returnsprintf('mailjet://%s:%s@%s',$this->from,$this->authToken,$this->getEndpoint());

Copy link

Contributor

OskarStarkJun 17, 2021•
edited
Loading

Choose a reason for hiding this comment

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

The auth-token should not be exposed, please remove it and fix the tests, thanks

Copy link

Member

fabpot commentedJun 23, 2021

@jnadaud Can you make the last change asked by Oskar? Thank you.

fabpot approved these changes

Jul 4, 2021

View reviewed changes

[Notifier] add Mailjet SMS bridge

ee744e7

fabpot force-pushed themailjet_notifier branch from9e616c7 toee744e7Compare

July 4, 2021 09:06

Copy link

Member

fabpot commentedJul 4, 2021

Thank you@jnadaud.

fabpot merged commit42790be intosymfony:5.4

Jul 4, 2021

Copy link

Member

fabpot commentedJul 4, 2021

@jnadaud Can you create a PR on symfony/recipes for this new notifier?

Copy link

Member

nicolas-grekas commentedJul 4, 2021

Tests fail also after this merge. Could you please have a look@jnadaud?

OskarStark mentioned this pull request

Jul 5, 2021

Fix: Mailjet bridge tests#41985

Merged

Copy link

Contributor

OskarStark commentedJul 5, 2021

I am on it 👍

#41985

derrabus added a commit that referenced this pull request

Jul 5, 2021

bug#41985Fix: Mailjet bridge tests (OskarStark)

b19200c

This PR was merged into the 5.4 branch.Discussion----------Fix: Mailjet bridge tests| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | Follows#41705| License       | MIT| Doc PR        | -Fix the buildCommits-------690e4ed Fix: Mailjet bridge tests

This was referencedNov 5, 2021

Release v5.4.0-BETA1#43937

Merged

Release v6.0.0-BETA1#43939

Merged