Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] Add Mobyt bridge#36648
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a4d1cf1 to512440fCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
512440f to3b5cdbbCompare3b5cdbb to59acfa2CompareDeamon commentedJun 12, 2020
Rebased few minutes ago on master. |
| return$message; | ||
| } | ||
| publicfunctiongetNotification(): ?Notification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hi@fabpot, you mean this function only or the entire modification of the file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The whole change in the SmsMessage class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Question about that, I need to know the notification level inside the Moby Transport class to select a "type_quality" parameter (which implies cost differences).
Is there an other way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would use the less costly by default and let's talk about this issue in another PR. That would allow to merge this one and think about the issue is a more broader sense instead of just for this specific provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank for you feedback. Fix is on the way.
fabpot commentedAug 10, 2020
@Deamon Can you submit a PR on symfony/recipes (you can have a look at other notifier bridge recipes to get some inspiration)? |
Deamon commentedAug 10, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hello@fabpot, thanks for your comment, There is also a documentation pr waiting. |
fabpot commentedAug 10, 2020
@daemon Can you also rebase this PR on current master and take my comment into account? Thank you. |
| return$messageinstanceof SmsMessage; | ||
| } | ||
| protectedfunctiondoSend(MessageInterface$message):void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This must return aSentMessage now.
Uh oh!
There was an error while loading.Please reload this page.
| "ext-json":"*", | ||
| "php":"^7.2.5", | ||
| "symfony/http-client":"^4.3|^5.0", | ||
| "symfony/notifier":"^5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be ^5.2
59acfa2 tocee47a1CompareDeamon commentedAug 12, 2020
@fabpot, all changes have been pushed |
| { | ||
| $options =$this->options; | ||
| unset($options['message']); | ||
| unset($options['recipient']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could be merged with the previous line.
| @@ -0,0 +1,4 @@ | |||
| /Testsexport-ignore | |||
| /phpunit.xml.distexport-ignore | |||
| /.gitattributesexport-ignore | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can be removed to be consistent with the other similar files in core.
| thrownewTransportException(sprintf('Unable to send the SMS: "%s".',$error['result']),$response); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You must return aSentMessage instance. Can you also test with a real account to be sure that everything works as expected?
cee47a1 tobf594b7CompareDeamon commentedAug 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@fabpot your last feedback have been resolved. the code is teste in v5.1 only, I couldn't get rid of the following error using dev-master branch: tried several ways but none worked for me. and yes, the sentMessage part isn't tested because class doesn't exists in 5.1 |
fabpot commentedAug 13, 2020
Thank you@Deamon. |
This PR was merged into the master branch.Discussion----------[Notifier] Add Mobyt NotifierHi all,here is the doc forsymfony/symfony#36648Commits-------63ea8b7 Add Mobyt Notifier doc
Uh oh!
There was an error while loading.Please reload this page.
Add Mobyt notifier bridge.
In this SMS Provider, you can choose a sort of "quality service" to send the message.
I updated
src/Symfony/Component/Notifier/Message/SmsMessage.phpto add the notification in order to be able to use the notification importance when creating options.