DrHyde / perl-modules-Number-Phone

Number::Phone and friends
24 stars 31 forks source link

Matching of prefix-less UK numbers to Jersey #101

Closed dracos closed 3 years ago

dracos commented 3 years ago

I was not expecting the following:

print Number::Phone::Lib->new("GB", "406092");
Number::Phone::StubCountry::JE=HASH(0x55a192563540)

What appears to be happening is that Number::Phone::Lib calls _new_args, which calls phone2country with +44406092. The idd code checking matches on 44 => ['GB', 'GG', 'IM', 'JE'] line, and the number is passed to each StubCountry in turn. All except GB have a nationalPrefixTransformRule check (created by build-data.stubs at https://github.com/DrHyde/perl-modules-Number-Phone/blob/62210a8df4ab45d6cbbb0104a830d2d495933db5/build-data.stubs#L157) that checks for 6-digit prefix-less numbers - GG has to start with [25-9], IM [5-8] and JE [0-24-8] - the last of these passes and so it comes back as a valid Jersey number. Which I mean, I guess it is, but it wasn't really what I was going for, if that's the case it's also a valid GB number in various places :)

Not sure if there's an easy solution, or if I've missed something obvious. I generally want to assume a full UK phone number including area code has been given.

DrHyde commented 3 years ago

Well this is definitely a bug - if you've asked for country XX then you ought to get country XX or nothing, not some related guesstimate!

DrHyde commented 3 years ago

I believe I have a fix in 08c0c27. Unless problems show up in testing that will be in the next release. It will still return an object if you ask for ...::Lib->new('JE', '406092') but not if you ask it for something ambiguous (ie that number for 'GB') or invalid (for 'IM').

DrHyde commented 3 years ago

And all the CI tests passed. I'll close this ticket shortly unless you have any comments.

dracos commented 3 years ago

Looks good, thanks :-)

dracos commented 3 years ago

Couple of additional thoughts only just come to me, sorry:

  1. Number::Phone::Lib->new("+44402609") still works and returns a JE number;
  2. As JE/IM/GG are in the UK numbering plan, I wonder where it is fair to expect Number::Phone::Lib->new("GB", "01534406092"); to work, which it now no longer does.

It was really only the fact it supported "402609" on its own that was my issue, not that it worked within the same numbering plan. If my code uses Number::Phone::Lib->new("GB", $user_entered_number), it will no longer work if some gives an 01534... number, which I guess they might expect it to. I can loop through GB/JE/IM/GG myself, I imagine, but just wanted to check what the expected behaviour was.

I can't currently switch to "+44" as a first argument because of point 1, it will say 402609 is valid again. I guess if that was resolved, I could then switch to +44 as my first argument instead of GB and that would work.

DrHyde commented 3 years ago

The first of those is unfortunate, but it's a limitation of the libphonenumber data. I could hard-code information about the UK numbering plan in there, but that's really icky.

The second ... hmmm. I'm not sure what is correct there. I think I'm less unhappy with the current behaviour, but I agree it's a nasty mess.

Unfortunately I can't just get rid of the nationalPrefixTransformRule completely, as it really is required for some other countries. Do you think it would be acceptable to drop it for GG/IM/JE only?

DrHyde commented 3 years ago

I've pushed c34782d which I think improves things. It's still a bit confusing though, so in particular please let me know any ways I can improve the doco.

dracos commented 3 years ago

I think that makes sense, thanks! Documentation explains it as I understand it too.

DrHyde commented 3 years ago

version 3.8000 with this fix in is now on its way to your friendly local CPAN mirror