[Notifier] smsapi-notifier `fast` option to sending message with the highest priority #45139

marphi · 2022-01-23T21:12:32Z

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	---
License	MIT
Doc PR	---

I have used custom solution to send sms via smsapi for many years. Time to rewrite it to notifier component. Found 2 issues from my point of view:

THIS part is splitted into separate PR [Notifier] Use the UTF-8 encoding in smsapi-notifier #45174. Encoding. Currently smsapi-notifier does not specify encoding in payload request to provider api which generates such errors.

(sms message with invalid encoding)
api.smsapi.pl use windows-1250 as a default value. I propose to set explicitly utf-8.
After changes:
I'm using smsapi to sending auth codes to my clients. In this case is really important to send SMS as fast as possible, fast options allow me to do this otherwise messages might have some delays.

src/Symfony/Component/Notifier/Bridge/Smsapi/README.md

src/Symfony/Component/Notifier/Bridge/Smsapi/SmsapiTransport.php

OskarStark

Please add a changelog to the transport changelog file, thanks

src/Symfony/Component/Notifier/Bridge/Smsapi/SmsapiTransportFactory.php

src/Symfony/Component/Notifier/Bridge/Smsapi/Tests/SmsapiTransportTest.php

OskarStark

As this is a new feature it must target 6.1 branch

stof · 2022-01-25T14:52:48Z

For the encoding one being considered as windows-1250 by default in the API, I would treat it as a bugfix, given that Symfony assumes UTF-8 by default in most places.

OskarStark · 2022-01-25T14:57:38Z

For the encoding one being considered as windows-1250 by default in the API, I would treat it as a bugfix, given that Symfony assumes UTF-8 by default in most places.

You are right, but in this case this PR should be slit into two PRs 🤷‍♂️

stof · 2022-01-25T15:09:49Z

@OskarStark indeed

stof · 2022-01-25T15:56:46Z

src/Symfony/Component/Notifier/Bridge/Smsapi/README.md

 ```

 where:
 - `TOKEN` is your API Token (OAuth)
 - `FROM` is the sender name
+ - `FAST` setting this parameter to "1" (default "0") will result in sending message with the highest priority which ensures the quickest possible time of delivery. Attention! Fast messages costs 50% more than normal message.


I suggest saying Fast messages cost more than normal messages, without mentioning the actual increase of 50%. this way, this readme does not become outdated if they change their pricing to modify the ratio.

@stof You are right. Done.

marphi · 2022-01-25T16:01:04Z

For the encoding one being considered as windows-1250 by default in the API, I would treat it as a bugfix, given that Symfony assumes UTF-8 by default in most places.

You are right, but in this case this PR should be slit into two PRs 🤷‍♂️

@stof @OskarStark We can leave this PR as new feature to 6.1 and I can prepare separate PR only with bugfix for encoding. If you prefer.

OskarStark · 2022-01-25T17:50:46Z

Please do so, we will merge the bugfix first then 👍🏻 thank you for your patience

marphi · 2022-01-25T19:38:27Z

@OskarStark PR only with bugfix #45174. This PR I've rebased to 6.1

This PR was merged into the 5.3 branch. Discussion ---------- [Notifier] Use the UTF-8 encoding in smsapi-notifier | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Tickets | --- | License | MIT | Doc PR | --- THIS PR is splitted version of PR #45139 (see discussion) Currently smsapi-notifier does not specify encoding in payload request to provider api which generates such errors. ![image](https://user-images.githubusercontent.com/57309/151045363-a5b97e9a-94b5-4fba-b93a-76ab72a8bd5f.png) (sms message with invalid encoding) api.smsapi.pl use windows-1250 as a default value. I propose to set explicitly `utf-8`. After changes: ![Zrzut ekranu 2022-01-23 o 21 52 28](https://user-images.githubusercontent.com/57309/151045277-2c11b5c5-8b0f-4f1d-b749-2661a6443a4c.png) Commits ------- 7ad382d [Notifier] smsapi-notifier - correct encoding

OskarStark · 2022-01-26T19:47:12Z

PR title should be adjusted, the encoding was fixed in a different PR

marphi · 2022-01-26T19:49:25Z

PR title should be adjusted, the encoding was fixed in a different PR

done

fabpot · 2022-01-27T07:51:41Z

Thank you @marphi.

…ssage with the highest priority (marphi) This PR was squashed before being merged into the 6.1 branch. Discussion ---------- [Notifier] smsapi-notifier `fast` option to sending message with the highest priority | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | --- | License | MIT | Doc PR | --- I have used custom solution to send sms via smsapi for many years. Time to rewrite it to notifier component. Found 2 issues from my point of view: 1. THIS part is splitted into separate PR #45174. ~~Encoding. Currently `smsapi-notifier` does not specify `encoding` in payload request to provider api which generates such errors. ![image](https://user-images.githubusercontent.com/57309/150697496-76998a85-43e7-418e-a4ab-b9efe69da506.png) (sms message with invalid encoding) api.smsapi.pl use `windows-1250` as a default value. I propose to set explicitly `utf-8`. After changes:~~ ![image](https://user-images.githubusercontent.com/57309/150697554-e0d1a8ee-d4d5-4644-b28e-ce17e7a0a320.png) 2. I'm using smsapi to sending auth codes to my clients. In this case is really important to send SMS as fast as possible, `fast` options allow me to do this otherwise messages might have some delays. Commits ------- cd266a0 [Notifier] smsapi-notifier `fast` option to sending message with the highest priority

marphi requested a review from OskarStark as a code owner January 23, 2022 21:12

carsonbot added Status: Needs Review Bug Feature Notifier labels Jan 23, 2022

carsonbot added this to the 5.4 milestone Jan 23, 2022

marphi changed the base branch from 5.4 to 5.3 January 23, 2022 21:12

OskarStark reviewed Jan 24, 2022

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Smsapi/README.md Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Bridge/Smsapi/SmsapiTransport.php Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Bridge/Smsapi/SmsapiTransport.php Outdated Show resolved Hide resolved

OskarStark reviewed Jan 25, 2022

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Smsapi/SmsapiTransportFactory.php Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Bridge/Smsapi/Tests/SmsapiTransportTest.php Outdated Show resolved Hide resolved

OskarStark approved these changes Jan 25, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 25, 2022

OskarStark modified the milestones: 5.4, 5.3 Jan 25, 2022

stof reviewed Jan 25, 2022

View reviewed changes

marphi mentioned this pull request Jan 25, 2022

[Notifier] Use the UTF-8 encoding in smsapi-notifier #45174

Merged

marphi force-pushed the notifier_smsapi_encoding_and_fast_options branch from ea2a6fe to 3583e0d Compare January 25, 2022 19:33

marphi requested review from chalasr, dunglas, jderusse, lyrixx, wouterj, xabbuh and yceruto as code owners January 25, 2022 19:33

carsonbot removed the Status: Reviewed label Jan 25, 2022

carsonbot added Status: Needs Review and removed Status: Reviewed labels Jan 25, 2022

marphi changed the base branch from 5.3 to 6.1 January 25, 2022 19:33

nicolas-grekas modified the milestones: 5.3, 6.1 Jan 26, 2022

marphi changed the title ~~[Notifier] smsapi-notifier correct encoding, optional fast option~~ [Notifier] smsapi-notifier fast option to sending message with the highest priority Jan 26, 2022

OskarStark approved these changes Jan 26, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 26, 2022

OskarStark requested a review from fabpot January 26, 2022 19:53

carsonbot removed the Status: Reviewed label Jan 26, 2022

OskarStark requested a review from stof January 26, 2022 19:53

carsonbot added the Status: Needs Review label Jan 26, 2022

fabpot approved these changes Jan 27, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 27, 2022

fabpot closed this Jan 27, 2022

fabpot force-pushed the notifier_smsapi_encoding_and_fast_options branch from 8e049a5 to cd266a0 Compare January 27, 2022 07:51

fabpot mentioned this pull request Jan 27, 2022

[Notifier] smsapi-notifier fast option to sending message with the hi… symfony/symfony-docs#16453

Closed

fabpot mentioned this pull request Apr 15, 2022

Release v6.1.0-BETA1 #46049

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Notifier] smsapi-notifier `fast` option to sending message with the highest priority #45139

[Notifier] smsapi-notifier `fast` option to sending message with the highest priority #45139

marphi commented Jan 23, 2022 •

edited

Loading

OskarStark left a comment

OskarStark left a comment

stof commented Jan 25, 2022

OskarStark commented Jan 25, 2022 •

edited

Loading

stof commented Jan 25, 2022

stof Jan 25, 2022

marphi Jan 25, 2022

marphi commented Jan 25, 2022

OskarStark commented Jan 25, 2022

marphi commented Jan 25, 2022

OskarStark commented Jan 26, 2022

marphi commented Jan 26, 2022

fabpot commented Jan 27, 2022

[Notifier] smsapi-notifier fast option to sending message with the highest priority #45139

[Notifier] smsapi-notifier fast option to sending message with the highest priority #45139

Conversation

marphi commented Jan 23, 2022 • edited Loading

OskarStark left a comment

Choose a reason for hiding this comment

OskarStark left a comment

Choose a reason for hiding this comment

stof commented Jan 25, 2022

OskarStark commented Jan 25, 2022 • edited Loading

stof commented Jan 25, 2022

stof Jan 25, 2022

Choose a reason for hiding this comment

marphi Jan 25, 2022

Choose a reason for hiding this comment

marphi commented Jan 25, 2022

OskarStark commented Jan 25, 2022

marphi commented Jan 25, 2022

OskarStark commented Jan 26, 2022

marphi commented Jan 26, 2022

fabpot commented Jan 27, 2022

[Notifier] smsapi-notifier `fast` option to sending message with the highest priority #45139

[Notifier] smsapi-notifier `fast` option to sending message with the highest priority #45139

marphi commented Jan 23, 2022 •

edited

Loading

OskarStark commented Jan 25, 2022 •

edited

Loading