Skip to content

[Notifier] Add Free Mobile notifier #35690

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 21, 2020
Merged

[Notifier] Add Free Mobile notifier #35690

merged 1 commit into from
Apr 21, 2020

Conversation

noniagriconomie
Copy link
Contributor

@noniagriconomie noniagriconomie commented Feb 12, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Implements symfony/symfony-docs#13025 (review)
License MIT
Doc PR Will document if accepted (see Usage below)

Add a new notifier (SMS) with the French Free Mobile provider.

It is a special notifier as it only send the SMS to the self user,
but I think it can be useful for notification alerting purposes (the way I use it already, and plan to use it with the component)

Provider doc: (🇫🇷 sorry)

https://mobile.free.fr/moncompte/index.php?page=options

1

2

Usage:

// .env file
FREEMOBILE_DSN=freemobile://LOGIN:PASSWORD@default?phone=PHONE

where:

  • LOGIN is your Free Mobile login
  • PASSWORD is the token displayed in the config panel
  • PHONE is your Free Mobile phone number
// config/packages/notifiers.yaml file
framework:
    notifier:
        texter_transports:
            freemobile: '%env(FREEMOBILE_DSN)%'

Then you can then use it like documented here https://symfony.com/doc/current/notifier/texters.html

ℹ️ As this is a special notifier, the PHONE provided inside the DSN mut be the same used here for $phone value

Voilà!

@noniagriconomie noniagriconomie force-pushed the feature-notifier-free-mobile branch 2 times, most recently from 82e539a to 0a37acb Compare February 12, 2020 17:52
@nicolas-grekas nicolas-grekas added this to the next milestone Feb 12, 2020
Guikingone
Guikingone reviewed Feb 13, 2020
View reviewed changes
src/Symfony/Component/Notifier/Bridge/FreeMobile/FreeMobileTransport.php Outdated

public function supports(MessageInterface $message): bool
{
return $message instanceof SmsMessage;
Copy link
Contributor

@Guikingone Guikingone Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What about checking an additional information about the $message as the payload is not exactly the same as others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Guikingone what do you mean?

like checking the sms phone is empty as it will be not used?

Copy link
Contributor

Choose a reason for hiding this comment

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

By example but I'm more worried about the fact that the TwilioTransport use the same approach to detect if it can support SmsMessage: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Notifier/Bridge/Twilio/TwilioTransport.php

I don't know if it's possible but it could be a good idea to differentiate both transport when it comes to detect if it can support the message ? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the same implementation for Nexmo as well, so maybe there is another thing that differentiate the notifier event if it is « supported »
I did not look at the internal component code yet tbh

Copy link
Member

Choose a reason for hiding this comment

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

The idea is that we can choose between providers that support sending SMS messages.

Here, the provider is a bit special as you can only send to your number. I think we need to add this phone number in the DSN that is then used here in the supports method to trigger this Transport. It's then your responsibility to make this transport the first one. That way, if you are using the special number, the Free notifier is used, if not, it falls back to other SMS providers. And in any case, the SMS message can also be send by other providers if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I see, I will improve this one
thank you

Copy link
Contributor Author

@noniagriconomie noniagriconomie Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@fabpot @Guikingone I've tried what you suggested, but I do not know if it is what both of you have in mind in term of implementation

also I added some tests (based on other transports)

PRs doc updated as well

@noniagriconomie noniagriconomie force-pushed the feature-notifier-free-mobile branch from 0a37acb to 2b0a5b8 Compare February 19, 2020 22:08
@stephanvierkant
Copy link
Contributor

I don't think this should be added. There are thousands of providers like this and adding them would make it unmaintainable.

@Nek-
Copy link
Contributor

Nek- commented Apr 8, 2020 •
edited
Loading

@stephanvierkant there is no thousands of ISP (it is the 2nd biggest of France), no. I'm 👍 with this addition, it feels absolutely legit to support biggest notification providers to me.

@noniagriconomie noniagriconomie force-pushed the feature-notifier-free-mobile branch 2 times, most recently from 39b3b67 to edaef59 Compare April 20, 2020 13:34
@noniagriconomie noniagriconomie force-pushed the feature-notifier-free-mobile branch from edaef59 to 1b8709e Compare April 20, 2020 13:48
@noniagriconomie
Copy link
Contributor Author

test failures are not related

- Symfony\Component\Cache\Tests\Adapter\RedisClusterAdapterTest::testGetMetadata
- Symfony\Component\HttpClient\Tests\AmpHttpClientTest::testInformationalResponseStream
12091Trying to access array offset on value of type null

@fabpot
Copy link
Member

fabpot commented Apr 21, 2020

Thank you @noniagriconomie.

@fabpot fabpot merged commit 4cc6055 into symfony:master Apr 21, 2020
@noniagriconomie noniagriconomie deleted the feature-notifier-free-mobile branch April 21, 2020 13:44
@noniagriconomie noniagriconomie mentioned this pull request Apr 28, 2020
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 29, 2020
@javiereguiluz
minor #13600 [Notifier] Add FreeMobile SMS entry (noniagriconomie)
7a783d8 
This PR was squashed before being merged into the master branch.

Discussion
----------

[Notifier] Add FreeMobile SMS entry

Hi,

I was reading this [blog post](https://symfony.com/blog/new-in-symfony-5-1-new-and-improved-integrations#notifier-component)
and remind I have not yet added it to the doc [this feature](symfony/symfony#35690)

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

Commits
-------

259c1e2 [Notifier] Add FreeMobile SMS entry
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
@noniagriconomie noniagriconomie mentioned this pull request Aug 12, 2020
Merged
fabpot added a commit that referenced this pull request Aug 12, 2020
@fabpot
minor #37809 [Notifier] add doc for free mobile dsn (noniagriconomie)
fc8a1ac 
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
@94noni 94noni mentioned this pull request Jan 26, 2022
Merged
nicolas-grekas added a commit that referenced this pull request Jan 26, 2022
@nicolas-grekas
bug #45185 [Notifier] Fix encoding of messages with FreeMobileTranspo…
4565edb 
…rt (94noni)

This PR was submitted for the 5.4 branch but it was squashed and merged into the 5.3 branch instead.

Discussion
----------

[Notifier] Fix encoding of messages with FreeMobileTransport

| Q             | A
| ------------- | ---
| Branch?       | 5.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix sending accents chars
| License       | MIT

When I first [introduce this transport](#35690) I used to test it with basic text, now with some French accent I need this fix otherwise the accent chars are not sent

Commits
-------

5760694 [Notifier] Fix encoding of messages with FreeMobileTransport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Guikingone Guikingone Guikingone left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature Notifier Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
7 participants
@noniagriconomie @stephanvierkant @Nek- @fabpot @Guikingone @nicolas-grekas @carsonbot