Skip to content
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

[Notifier] Add notifier for Clickatell #38922

Merged
merged 1 commit into from Jan 25, 2021
Merged

[Notifier] Add notifier for Clickatell #38922

merged 1 commit into from Jan 25, 2021

Conversation

ke20
Copy link

@ke20 ke20 commented Oct 31, 2020

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
License MIT

Add notifier bridge for Clickatell

@Nyholm
Copy link
Member

Nyholm commented Oct 31, 2020

Thank you for this PR.

Could you see why the fabbot.io is upset? They will give you some curl commands so it will fix things automatically.

@jderusse jderusse added this to the 5.x milestone Oct 31, 2020
@B-Galati
Copy link
Contributor

B-Galati commented Nov 2, 2020

A new recipe will be needed no?

@ke20
Copy link
Author

ke20 commented Nov 2, 2020

A new recipe will be needed no?

Yes thx, I created a PR for the recipe here : symfony/recipes#838

@jschaedl
Copy link
Contributor

jschaedl commented Nov 4, 2020

@ke20 As every bridge has its own LICENSE file could you please add one here as well?

@OskarStark
Copy link
Contributor

Typo in the PR title:
CleanShot 2020-11-20 at 12 16 34

@ke20 ke20 changed the title [Notifier] Add notifier for Clikatell [Notifier] Add notifier for Clickatell Nov 20, 2020
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Can you add a .gitignore file like done in other bridges?

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.

Some small things, I think using null for from is more common

@wouterj wouterj removed their request for review January 20, 2021 10:35
@OskarStark
Copy link
Contributor

Hi @ke20 👋

Can you please rebase and apply my comments? Thanks

@ke20
Copy link
Author

ke20 commented Jan 22, 2021

Hi @ke20 wave

Can you please rebase and apply my comments? Thanks

Done @OskarStark

But Travis fails for PHP 7.4, there are flaky tests ?

I don't know if it's "normal".

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.

Test failures unrelated

@OskarStark
Copy link
Contributor

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

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.

None yet