perl-modules-Number-Phone icon indicating copy to clipboard operation
perl-modules-Number-Phone copied to clipboard

Incorrect parsing behaviour if country 2 letter code is passed to the constructor.

Open mschout opened this issue 8 years ago • 5 comments

I've discovered an inconsistency between Number::Phone and google libphonenumber in the case where both the country code and the number are passed to the constructor. Here is how Google's libphonenumber works (using the javascript version):

 var libphone = require('libphonenumber');
 var number = libphone.phoneUtil.parse("+377 45111111", "MC");

 console.log('Country Code: %s Number %s',
     number.getCountryCode(),
     number.getNationalNumber());

This outputs: Country Code: 377 Number 45111111

Which seems reasonable. (note that if I pass "ZZ" as the country code it does the same thing).

Number::Phone however, does not seem to get this right:

 $phone = Number::Phone->new("MC", "+377 45111111");
 say $phone->country_code;
 say $phone->format_using("Raw")

This prints: 37745 111111

And in addition, it is treating this number as a number from country "KOS".

Digging into why this happens, it seems that Number::Phone::_new_args() clobbers the passed in country by calling Number::Phone::Country::phone2country($number)

A quick and dirty workaround for this was to wrap this line in _new_args with a unless (defined $country) { ... } block.

That results in the expected output (country_code=377, Raw format=45111111)

A better fix might be to only do this if the passed in country is one that Number::Phone recognizes.

mschout avatar Aug 23 '16 19:08 mschout

Also to note, using Number::Phone::Lib (which docs say uses libphonenumber derived data only) makes no difference.

mschout avatar Aug 23 '16 22:08 mschout

Ugh, Kosovo. A special and very annoying case because it's a country that isn't fully recognized. It's not a UN member state and so doesn't have its own two letter country code, but I needed some country code and made up KOS. Nor does it have an IDD code allocated by the ITU, although it is apparently going to get +383 once the bureaucrats pull their fingers out of their arses.

Instead services there use bits of three different countries' number ranges - +377 44/45 (Monaco), +386 43/49 (Slovenia), and +381 28/29/38/39 (Serbia).

I don't consider this to be a bug. Also, it's not the only area where Number::Phone disagrees with libphonenumber. For example, libphonenumber assumes that all +1800 numbers are in the US because (according to the developers) that's where they're most likely to be. Number::Phone instead treats them as being generic NANP numbers.

NB I haven't bothered to do similar for weird half-arsed states like Abkhazia because very few people care about them and they are unlikely to become real countries any time soon. Kosovo is considerably more significant.

As for the constructor ... the two argument form is supposed to be invoked thus: ...->new("UK", "01234567890") - that is, a country code and then a number in local form as opposed to E.123 form. Invoking it like ...->new("UK", "+44 ...") should also work, but something like ...->new("FR", "+44...") which is effectively what you're doing - providing two different country codes - should, I suppose, be an error. But then I think you'd have legitimate reason to complain given the bloody irritating pain in the arse that is one country using the number range of another. So yeah, there's a bug here, but I'm loath to fix it by making it an error.

I'm not really sure what's the best thing to do here.

DrHyde avatar Sep 07 '16 19:09 DrHyde

Fair enough.

I actually ended up migrating to an XS wrapper around libphonenumber itself as the upstream vendor we send phone numbers to is quite strict about country codes. What libphonenumber does works better for us, but I completely understand your frustration :).

mschout avatar Sep 07 '16 22:09 mschout

What I've done was to test for starting with a + first, and call differently if so: https://github.com/mysociety/fixmystreet/blob/8e5853830f0fb65985881272b3b0178b37ac947b/perllib/FixMyStreet/SMS.pm#L91-L99 I think I would on balance prefer it if it did act like libphonenumber, and the country code is a hint but is ignored if a +... number is provided. But it is at least not hard to behave differently :)

dracos avatar Oct 01 '17 13:10 dracos

Kosovo is no longer squatting on other countries' number ranges, so this problem has at least partially gone away of its own accord.

DrHyde avatar Aug 24 '21 22:08 DrHyde