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

Matching of prefix-less UK numbers to Jersey #101

Closed
dracos opened this issue Aug 20, 2021 · 9 comments
Closed

Matching of prefix-less UK numbers to Jersey #101

dracos opened this issue Aug 20, 2021 · 9 comments
Projects

Comments

@dracos
Copy link
Contributor

dracos commented Aug 20, 2021

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

# There is a national prefix transform rule.
) 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
Copy link
Owner

DrHyde commented Aug 24, 2021

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 added a commit that referenced this issue Aug 24, 2021
@DrHyde
Copy link
Owner

DrHyde commented Aug 24, 2021

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
Copy link
Owner

DrHyde commented Aug 24, 2021

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

@DrHyde DrHyde added this to In progress in Maintenance Aug 24, 2021
@DrHyde DrHyde moved this from In progress to Testing/feedback in Maintenance Aug 24, 2021
@dracos
Copy link
Contributor Author

dracos commented Aug 25, 2021

Looks good, thanks :-)

@DrHyde DrHyde closed this as completed Aug 25, 2021
Maintenance automation moved this from Testing/feedback to Done Aug 25, 2021
@dracos
Copy link
Contributor Author

dracos commented Sep 1, 2021

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
Copy link
Owner

DrHyde commented Sep 2, 2021

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 DrHyde reopened this Sep 2, 2021
@DrHyde DrHyde moved this from Done to In progress in Maintenance Sep 2, 2021
@DrHyde
Copy link
Owner

DrHyde commented Sep 9, 2021

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
Copy link
Contributor Author

dracos commented Sep 10, 2021

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

dracos added a commit to mysociety/fixmystreet that referenced this issue Sep 15, 2021
There is an issue with Number::Phone that means it is matching e.g.
402609 as a Jersey phone number. Reported and behaviour improved at
DrHyde/perl-modules-Number-Phone#101, but
until a new release, move the searches around so a ref search takes
precedence.
dracos added a commit to mysociety/fixmystreet that referenced this issue Sep 16, 2021
There is an issue with Number::Phone that means it is matching e.g.
402609 as a Jersey phone number. Reported and behaviour improved at
DrHyde/perl-modules-Number-Phone#101, but
until a new release, move the searches around so a ref search takes
precedence.
@DrHyde
Copy link
Owner

DrHyde commented Sep 19, 2021

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

@DrHyde DrHyde closed this as completed Sep 19, 2021
Maintenance automation moved this from In progress to Done Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants