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

Use libphonenumber to properly format phone numbers #216

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hanzi
Copy link

@hanzi hanzi commented Aug 11, 2020

This replaces the phone number formatting method using strtr() with one that utilises the libphonenumber library (which is PHP port of the Google library with the same name.)

That allows a user to just define the country code where the FRITZ!Box will be used and libphonenumber will handle the rest.

It will automatically apply appropriate formatting for phone numbers and detect whether it's a national or international phone number. See the docblock for the method used:
https://github.com/giggsey/libphonenumber-for-php/blob/master/src/PhoneNumberUtil.php#L2643

Fixes #193

@andig
Copy link
Owner

andig commented Aug 12, 2020

/cc @blacksenator

@andig
Copy link
Owner

andig commented Aug 12, 2020

LGTM on quick glance. Would appreciate confirmation by @blacksenator

@blacksenator
Copy link
Collaborator

I'll do this the next couple of days

return $number;
$numberUtil = PhoneNumberUtil::getInstance();
try {
$countryCode = $this->config['defaultCountry'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

$countryCode = $this->config['country'] ?? 'DE';

'@' => '',
'-' => ''
],
'defaultCountry' => 'DE',
Copy link
Collaborator

@blacksenator blacksenator Aug 14, 2020

Choose a reason for hiding this comment

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

analog config.example.php: 'country' => 'DE',

'/' => '',
'-' => '',
],
'defaultCountry' => 'DE'
Copy link
Collaborator

Choose a reason for hiding this comment

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

'country' => 'DE'

Copy link
Collaborator

@blacksenator blacksenator left a comment

Choose a reason for hiding this comment

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

@hanzi
Just to be sure:
The conversion may be in accordance with the ITU specifications - but can we also be sure that the FRITZ devices will process defined separators in foreign numbers?
This may not even happen for the vast majority of users - but using the library there is no longer any possibility for individual fine-tuning...
Listen to the FRITZ!Box Callmonitor only digits (apart from SIP) without any separators are displayed. Beside of this: unfortunately there is no AVM documentation which separators can be processed.

Have you tested with AVM devices whether the converted foreign numbers are initiating a dial out? For instance: test calls from the telephone book of a FRITZ!Fon or with a 'Wählhilfe' from the FRITZ!Box GUI?
Conversely, the central convenience function of identifying the caller using a stored telephone book in the FRITZ!Fon should not be lost when a (foreign) call is received.

As I said - possibly a marginal problem - but you never know ...

@blacksenator
Copy link
Collaborator

Contributer never updated reviewed topics. Beside of this: Apart from that, the implementation of the extensive library does not bring any real improvement over the manual solution in the config. I suggest to close this topic.

@hanzi
Copy link
Author

hanzi commented Dec 28, 2020

Apologies, I must have missed the notification for the review in August.

I've renamed the option from defaultCountry to country as requested.

The conversion may be in accordance with the ITU specifications - but can we also be sure that the FRITZ devices will process defined separators in foreign numbers?

I've been using this fork for half a year now, with various international numbers in my address book (from several European countries, India, Taiwan, USA, Vietnam) and never ran into any issues.

Caller ID also works without issues, regardless of formatting in the address book.

As far as I can tell, the Fritzbox happily ignores any whitespace/separators. But as you said, there doesn't seem to be any documentation for that.

Apart from that, the implementation of the extensive library does not bring any real improvement over the manual solution in the config.

I understand the hesitation for including another dependency. That's of course your decision. For me it works quite well and I've also been using it in some customer projects without any issues.

My personal benefits with this solution:

  1. It gives me nicely formatted phone numbers that honour the formatting of each country, separate area codes for easier readability etc.
  2. It worked better with my existing address book which often prefixes international numbers with 0046 instead of +46. I know that the latter would be more correct anyway, but that's what I've got.
  3. There were some other issues with the strtr solution and my address book, but I can't remember them exactly.
  4. Configuration feels a bit more straight forward to me (setting a country code instead of having to think about the more technical replacement tables) but that's of course highly subjective.

@blacksenator
Copy link
Collaborator

@andig if @hanzi says that it doesn't cause any problems in the interaction of the devices, then I don't see any reasons why it shouldn't be part of the solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

International area code ignored
3 participants