Skip to content

[Notifier] Add FreeMobile SMS entry #13600

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
Apr 29, 2020

Conversation

noniagriconomie
Copy link
Contributor

Hi,

I was reading this blog post
and remind I have not yet added it to the doc this feature

Just a question as I am not sure where and how to document the fact that this sms notifier is a bit special, is it ok like this in the versionadded code block?

or maybe it is better in a note afterward?

Thank you

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Just a question as I am not sure where and how to document the fact that this sms notifier is a bit special, is it ok like this in the versionadded code block?

I always approach versionadded blocks as things that can be deleted without thinking about the contents. So we should not be loosing information when deleting them. I would say add a .. note:: after telling what makes FreeMobile so special (is there a reason btw to make it special? Can't it set the phone number dynamically?)

@94noni
Copy link
Contributor

94noni commented Apr 28, 2020

@wouterj ok lets add a note then

is there a reason btw to make it special? Can't it set the phone number dynamically?

This french phone provider allow to send free sms to your personal phone number

edit: personal account :s

@noniagriconomie noniagriconomie changed the title Update notifier.rst [Notifier] Add FreeMobile SMS entry Apr 29, 2020
@javiereguiluz
Copy link
Member

javiereguiluz commented Apr 29, 2020

I'm not sure about adding that note about Free Mobile. It's an internal detail of this service. Yes, I know it's important that you cannot send SMS but to yourself ... but what are the chances of being a client of that company and not knowing that?

@noniagriconomie
Copy link
Contributor Author

@javiereguiluz I see your point, maybe in the code itself then instead of the documentation?
(I will remove it from here)

@javiereguiluz
Copy link
Member

@noniagriconomie yes! Hopefully, in the code, we can return a perfect error message when someone tries to send a SMS to explain them that you can only send SMS to yourself.

@javiereguiluz
Copy link
Member

Thanks Antoine.

@javiereguiluz javiereguiluz merged commit 7a783d8 into symfony:master Apr 29, 2020
@noniagriconomie noniagriconomie deleted the patch-6 branch August 12, 2020 09:08
fabpot added a commit to symfony/symfony that referenced this pull request Aug 12, 2020
This PR was merged into the 5.2-dev branch.

Discussion
----------

[Notifier] add doc for free mobile dsn

| Q             | A
| ------------- | ---
| Branch?       | master (maybe 5.1 ? it is only code doc)
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix symfony/symfony-docs#13600 (comment)
| License       | MIT
| Doc PR        |

Improve readme according to the linked comment
I've taken info of my original PR #35690 (with some rewrite)

Also the package was renamed, fixing the doc here symfony/symfony-docs#14057

Commits
-------

eb067ed [Notifier] add doc for free mobile dsn
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