-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Notifier] Add TurboSms Bridge #41522
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
Conversation
b936c71
to
486ac94
Compare
Travis build failed because of
I added a new package to the composer.json https://github.com/symfony/symfony/pull/41522/files#diff-08024b4d552345610ae41302c94fd454bae1875e86c8eb751409172aa249514cR84 But this package doesn't exist yet. |
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.
thanks for submitting
src/Symfony/Component/Notifier/Bridge/TurboSms/Tests/TurboSmsTransportFactoryTest.php
Outdated
Show resolved
Hide resolved
I'm not super comfortable with adding a bridge to a service whose documentation is not available in english, that limits the potential adoption quite a lot. (note that we don't have any policy yet, I'm just wondering). |
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.
@chalasr valid point but IMHO there should be no need for the docs unless it works like expected? WDYT? The nice thing is that I can use this provider while I cannot if there is no bridge available.
src/Symfony/Component/Notifier/Bridge/TurboSms/TurboSmsTransportFactory.php
Show resolved
Hide resolved
src/Symfony/Component/Notifier/Bridge/TurboSms/TurboSmsTransport.php
Outdated
Show resolved
Hide resolved
If one day the implementation becomes outdated or no longer works as expected for whatever reason, who will be able to fix it? |
IIRC we already have some bridges which have the same problem. This should not be an argument, just an info. |
@chalasr our community a big :) I am provided two companies from Lithuania and if needed to help read docs it is not a big issue :))) one company officially have English translation, another company have only LT :) |
As I see TurboSMS is a UA company, but page loads Russian :) Too know Russian no problem 😊😁 |
My 2cts: Providers (notifier, mailer, translation, ...) MUST be maintained by the community (the developers using them), the core team cannot maintain them except for mass changes (CS, new PHP version, ...). |
Agree with @fabpot 👍 |
Works for me. |
src/Symfony/Component/Notifier/Bridge/TurboSms/TurboSmsTransport.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Notifier/Bridge/TurboSms/Tests/TurboSmsTransportFactoryTest.php
Show resolved
Hide resolved
You will need to make fabbot happy (rebase) :) |
9a25794
to
19ee1a4
Compare
rebased |
Can you please rebase? Thanks |
19ee1a4
to
f2d965e
Compare
@OskarStark rebased |
a0ce72c
to
12bacf4
Compare
Thank you Artem. |
This PR was merged into the 5.4 branch. Discussion ---------- [Notifier] Add TurboSms Bridge TurboSms Bridge: symfony/symfony#41522 Commits ------- 10cced4 Add TurboSms notifier
Add TurboSms bridge to Symfony Notifier
https://turbosms.ua/api.html This service is very popular in Ukraine