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

[Notifier] Add Orange SMS bridge#44884

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

Closed

enigma972 wants to merge21 commits intosymfony:5.4fromenigma972:5.4

Closed

[Notifier] Add Orange SMS bridge#44884

enigma972 wants to merge21 commits intosymfony:5.4fromenigma972:5.4

Conversation

Copy link

Contributor

enigma972 commentedJan 1, 2022

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT

Add Orange SMS bridge

[Notifier] Add Orange SMS bridge

4bc6ee7

enigma972 requested a review fromOskarStark as acode owner

January 1, 2022 10:10

carsonbot added Status: Needs Review Feature Notifier labels

Jan 1, 2022

carsonbot added this to the5.4 milestone

Jan 1, 2022

Copy link

carsonbot commentedJan 1, 2022

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 6.1 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

enigma972 added8 commits

January 1, 2022 11:22

[Notifier] Fix Tests classes namespaces

d7f88de

[Notifier][Bridge][OrangeSms] Add somes git files

dad3317

[Notifier][Bridge][OrangeSms] Fix tests

e65759e

[Notifier][Bridge][OrangeSms] Fix code standards

e2f73f8

[Notifier][Bridge][OrangeSms] Fix tests

bba4192

[Notifier][Bridge][OrangeSms] Fix dsn parameter name

666d342

[Notifier][Bridge][OrangeSms] Fix dsn parameter name

174b8a9

[Notifier][Bridge][OrangeSms] Fix tests

e1ab434

Copy link

Contributor

OskarStark commentedJan 1, 2022

Please target 6.1 branch

OskarStark reviewed

Jan 1, 2022

View reviewed changes

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

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransportFactory.php OutdatedShow resolvedHide resolved

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

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransportFactory.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/OrangeSms/Tests/OrangeSmsTransportTest.php Outdated


		public function toStringProvider(): iterable
		{
		yield ['orangesms://default?from=FROM&sender_name=SENDER_NAME', $this->createTransport()];

Copy link

Contributor

OskarStarkJan 1, 2022

Choose a reason for hiding this comment

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

Test missing, where Sender name is not set

src/Symfony/Component/Notifier/Bridge/OrangeSms/composer.json OutdatedShow resolvedHide resolved

Kocal reviewed

Jan 2, 2022

View reviewed changes

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php Outdated

Comment on lines 28 to 41

		private $clientID;
		private $clientSecret;
		private $from;
		private $senderName;

		public function __construct(string $clientID, string $clientSecret, string $from, ?string $senderName, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
		{
		$this->clientID = $clientID;
		$this->clientSecret = $clientSecret;
		$this->from = $from;
		$this->senderName = $senderName;

		parent::__construct($client, $dispatcher);
		}

Copy link

Member

KocalJan 2, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Since 6.1 is PHP 8 only, why not just use constructor property promotion?

Suggested change

	private$clientID;
	private$clientSecret;
	private$from;
	private$senderName;

	publicfunction __construct(string$clientID,string$clientSecret,string$from, ?string$senderName,HttpClientInterface$client =null,EventDispatcherInterface$dispatcher =null)
	{
	$this->clientID =$clientID;
	$this->clientSecret =$clientSecret;
	$this->from =$from;
	$this->senderName =$senderName;

	parent::__construct($client,$dispatcher);
	}
	publicfunction __construct(privatestring$clientID,privatestring$clientSecret,privatestring$from,private ?string$senderName,HttpClientInterface$client =null,EventDispatcherInterface$dispatcher =null)
	{
	parent::__construct($client,$dispatcher);
	}

Copy link

Contributor

OskarStarkJan 3, 2022

Choose a reason for hiding this comment

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

Not sure Symfony uses it

Cc @symfony/mergers

Copy link

Member

GromNaNJan 3, 2022

Choose a reason for hiding this comment

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

I don't see any reason not to use it in new code. But before, we have to be sure that PHP-CS-Fixer/fabbot is compatible.

enigma972 added2 commits

January 2, 2022 18:40

[Notifier][Bridge][OrangeSms] Fix CS

ad7cc8d

[Notifier][Bridge][OrangeSms] Fix CS

fff38ec

GromNaN requested changes

Jan 2, 2022

View reviewed changes

Copy link

Member

GromNaN left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Nice addition.

After a rebase on branch 6.1, you can update the target of this PR.

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php


		$url = 'https://'.$this->getEndpoint().'/smsmessaging/v1/outbound/'.urlencode('tel:'.$this->from).'/requests';
		$headers = [
		'Authorization' => 'Bearer '.$this->getAccessToken(),

Copy link

Member

GromNaNJan 2, 2022

Choose a reason for hiding this comment

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

The access token should be cached and reused if several messages are sent. But that would require some expiration management for long running processes.
This is not blocking and could be handled in a future PR.

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

Jan 2, 2022

enigma972 added2 commits

January 3, 2022 11:18

[Notifier][Bridge][OrangeSms] Fix scheme

66991ce

[Notifier][Bridge][OrangeSms] change scheme test host

c222672

GromNaN reviewed

Jan 3, 2022

View reviewed changes

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php

		$url = 'https://'.$this->getEndpoint().'/oauth/v3/token';
		$credentials = $this->clientID.':'.$this->clientSecret;
		$headers = [
		'Authorization' => 'Basic '.base64_encode($credentials),

Copy link

Member

GromNaNJan 3, 2022

Choose a reason for hiding this comment

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

The HTTP client has theauth_basic option for that.
https://symfony.com/doc/current/http_client.html#authentication

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php OutdatedShow resolvedHide resolved

enigma972 added4 commits

January 3, 2022 12:01

[Notifier][Bridge][OrangeSms] Fix missing transport parameters

9b0658f

[Notifier][Bridge][OrangeSms] Prevent null sender name on __toString()

6deb236

[Notifier][Bridge][OrangeSms] Format LICENCE file

a272b6d

[Notifier][Bridge][OrangeSms] Fix reviewer request

c68be45

[Notifier][Bridge][OrangeSms] Fix type return

cc3adfb

enigma972 requested a review fromGromNaN

January 3, 2022 12:33

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

Jan 3, 2022

stof reviewed

Jan 4, 2022

View reviewed changes

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php Outdated

		private string $clientID;
		private string $clientSecret;
		private string $from;
		private string $senderName;

Copy link

Member

stofJan 4, 2022

Choose a reason for hiding this comment

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

this is wrong. The constructor will assignstring|null in this property

src/Symfony/Component/Notifier/Bridge/OrangeSms/OrangeSmsTransport.php Outdated

		$errorMessage = $content['requestError']['serviceException']['messageId'] ?? '';
		$errorInfo = $content['requestError']['serviceException']['text'] ?? '';

		throw new TransportException(sprintf('Unable to send the SMS: '.$errorMessage.' (%s).', $errorInfo), $response);

Copy link

Member

stofJan 4, 2022

Choose a reason for hiding this comment

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

concatenating the error message into the format string ofsprintf is a no-go, as it would require escaping% in the error message to avoid breaking it.

Copy link

Member

stof commentedJan 4, 2022

please also check the CI failures

enigma972 added2 commits

January 4, 2022 17:26

[Notifier][Bridge][OrangeSms] Fix property type

1c3a4c8

[Notifier][Bridge][OrangeSms] Fix CS

090acd7

enigma972 requested review fromOskarStark andstof

January 4, 2022 16:57

Copy link

Contributor

OskarStark commentedJan 4, 2022

Please rebase on 6.1 branch and target 6.1 afterwards, thanks

Merge branch 'symfony:5.4' into 5.4

193a34c

enigma972 mentioned this pull request

Jan 5, 2022

[Notifier] Add Orange SMS bridge#44913

Merged

Copy link

ContributorAuthor

enigma972 commentedJan 5, 2022

Please rebase on 6.1 branch and target 6.1 afterwards, thanks

By bad handling I deleted the fork, so I had to open another pull request
At#44913

GromNaN closed this

Jan 5, 2022

fabpot added a commit that referenced this pull request

Jan 17, 2022

feature#44913[Notifier] Add Orange SMS bridge (enigma972)

a4d6d4d

This PR was squashed before being merged into the 6.1 branch.Discussion----------[Notifier] Add Orange SMS bridge| Q             | A| ------------- | ---| Branch?       | 6.1| Bug fix?      | no| New feature?  | yes| Deprecations? | no| License       | MITAdd Orange SMS bridgePreviously [#44884](#44884)Commits-------1aebeaf [Notifier] Add Orange SMS bridge

symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request

Jan 17, 2022

feature #44913 [Notifier] Add Orange SMS bridge (enigma972)

f0fc83e

This PR was squashed before being merged into the 6.1 branch.Discussion----------[Notifier] Add Orange SMS bridge| Q             | A| ------------- | ---| Branch?       | 6.1| Bug fix?      | no| New feature?  | yes| Deprecations? | no| License       | MITAdd Orange SMS bridgePreviously [#44884](symfony/symfony#44884)Commits-------1aebeaf77c [Notifier] Add Orange SMS bridge

Labels

Feature Notifier Status: Needs Review

Movatterモバイル変換

Uh oh!

[Notifier] Add Orange SMS bridge#44884

[Notifier] Add Orange SMS bridge#44884

Uh oh!

Conversation

enigma972 commentedJan 1, 2022

Uh oh!

carsonbot commentedJan 1, 2022

Uh oh!

OskarStark commentedJan 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OskarStarkJan 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KocalJan 2, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OskarStarkJan 3, 2022

Choose a reason for hiding this comment

Uh oh!

GromNaNJan 3, 2022

Choose a reason for hiding this comment

Uh oh!

GromNaN left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GromNaNJan 2, 2022

Choose a reason for hiding this comment

Uh oh!

GromNaNJan 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stofJan 4, 2022

Choose a reason for hiding this comment

Uh oh!

stofJan 4, 2022

Choose a reason for hiding this comment

Uh oh!

stof commentedJan 4, 2022

Uh oh!

OskarStark commentedJan 4, 2022

Uh oh!

enigma972 commentedJan 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

KocalJan 2, 2022•
edited
Loading

GromNaN left a comment•
edited
Loading