Skip to content

[Notifier] Add bridge for smsc.ru #42180

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
Jul 19, 2021
Merged

Conversation

kozlice
Copy link

@kozlice kozlice commented Jul 18, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR symfony/symfony-docs#15536

Hi,

This is a notifier bridge for Russian SMS provider smsc.

This PR contains the new bridge and updates to framework bundle in all the places where all the other notifiers are listed.

@Nyholm
Copy link
Member

Nyholm commented Jul 18, 2021

Great. Thank you for this PR.

It looks like a few CI jobs are complaining. Could you make sure that at least "fabbot.io" and "Package / Verify" are green?

@kozlice kozlice force-pushed the smsc-notifier branch 2 times, most recently from 8901968 to a7cfda2 Compare July 18, 2021 22:57
@derrabus derrabus self-requested a review July 18, 2021 23:00
@derrabus derrabus removed their request for review July 18, 2021 23:00
@kozlice
Copy link
Author

kozlice commented Jul 18, 2021

Sure, fixed all the CI issues I could.

There still is a failed test, but it seems to be unrelated.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Cool cool cool.

Just a few minor things an I'll be happy.

--

Correct. That test failure is unrelated. Thank you for checking.

@kozlice kozlice force-pushed the smsc-notifier branch 2 times, most recently from 2d53fe7 to 0863f6d Compare July 18, 2021 23:46
@kozlice kozlice force-pushed the smsc-notifier branch 2 times, most recently from 6b70d93 to 0a69dc2 Compare July 19, 2021 00:04
@derrabus derrabus added this to the 5.4 milestone Jul 19, 2021
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

Im happy with this PR. The psalm issue is addressed in #42183.

I think @OskarStark will be happy to review this PR later.

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.

Just some minor comments.

  • Can you please rebase to get rid of the Psalm errors
  • Please create a recipe at symfony/recipes-contrib repo, thanks

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.

Please create a recipe for this bridge

@OskarStark
Copy link
Contributor

Thanks Valentin for working on this feature, this is much appreciated.

@OskarStark OskarStark merged commit d46125a into symfony:5.4 Jul 19, 2021
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Jul 19, 2021
This PR was merged into the 5.4 branch.

Discussion
----------

[Notifier] Add bridge for smsc.ru

Add Notifier brdige for smsc.ru

symfony/symfony#42180

Commits
-------

5f443b9 [Notifier] Add bridge for smsc.ru
@kozlice
Copy link
Author

kozlice commented Jul 19, 2021

Thanks for the fast and detailed review, guys

Here's the recipe PR: symfony/recipes#975

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.

5 participants