Skip to content

[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

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

fre5h
Copy link
Contributor

@fre5h fre5h commented Jun 2, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#15403
Recipe PR symfony/recipes#953

Add TurboSms bridge to Symfony Notifier

https://turbosms.ua/api.html This service is very popular in Ukraine

@fre5h
Copy link
Contributor Author

fre5h commented Jun 3, 2021

Travis build failed because of

  Problem 1
    - Root composer.json requires symfony/turbo-sms-notifier, it could not be found in any version, there may be a typo in the package name.

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.
I don't now how to fix CI correctly. Should I remove this package from composer.json? Or is there some other trick?

@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Jun 3, 2021
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

thanks for submitting

@chalasr
Copy link
Member

chalasr commented Jun 3, 2021

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).

Copy link
Contributor

@OskarStark OskarStark left a 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.

@chalasr
Copy link
Member

chalasr commented Jun 3, 2021

valid point but IMHO there should be no need for the docs unless it works like expected? WDYT?

If one day the implementation becomes outdated or no longer works as expected for whatever reason, who will be able to fix it?
Not providing an internationalized entry point for a product is either a sign of a lack of resources or it means the product targets a very specific, localized population.
Maybe this should just be released as a community bundle until the service has a universally accessible entry point?
I won't vote against this PR, but I don't feel safe regarding our ability to maintain this transport on the long run.

@OskarStark
Copy link
Contributor

IIRC we already have some bridges which have the same problem. This should not be an argument, just an info.

@StaffNowa
Copy link
Contributor

StaffNowa commented Jun 4, 2021

@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 :)

@StaffNowa
Copy link
Contributor

As I see TurboSMS is a UA company, but page loads Russian :) Too know Russian no problem 😊😁

@fabpot
Copy link
Member

fabpot commented Jun 4, 2021

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, ...).

@StaffNowa
Copy link
Contributor

Agree with @fabpot 👍

@chalasr
Copy link
Member

chalasr commented Jun 4, 2021

Works for me.

@StaffNowa
Copy link
Contributor

You will need to make fabbot happy (rebase) :)

@fre5h fre5h force-pushed the feature-notifier-turbosms branch from 9a25794 to 19ee1a4 Compare June 7, 2021 06:30
@fre5h
Copy link
Contributor Author

fre5h commented Jun 7, 2021

rebased

@OskarStark
Copy link
Contributor

Can you please rebase? Thanks

@fre5h fre5h force-pushed the feature-notifier-turbosms branch from 19ee1a4 to f2d965e Compare August 2, 2021 06:57
@fre5h
Copy link
Contributor Author

fre5h commented Aug 2, 2021

@OskarStark rebased

@OskarStark OskarStark self-requested a review August 2, 2021 12:45
@OskarStark OskarStark force-pushed the feature-notifier-turbosms branch from a0ce72c to 12bacf4 Compare August 4, 2021 14:40
@OskarStark
Copy link
Contributor

Thank you Artem.

@OskarStark OskarStark merged commit 52ce2af into symfony:5.4 Aug 4, 2021
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Aug 4, 2021
This PR was merged into the 5.4 branch.

Discussion
----------

[Notifier] Add TurboSms Bridge

TurboSms Bridge: symfony/symfony#41522

Commits
-------

10cced4 Add TurboSms notifier
@fre5h fre5h deleted the feature-notifier-turbosms branch August 5, 2021 06:22
This was referenced Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants