DrHyde / perl-modules-Number-Phone

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

Russian numbers with the region code starting with 8 don't pass validation #28

Closed bigunyak closed 9 years ago

bigunyak commented 9 years ago

Hi,

Seems the recent changes in Number::Phone::StubCountry::RU module in version 1.20140904220737 made all Russian phone number with the region code starting with 8 being treated as invalid numbers. The problem is that after removing the country code, you also remove the leading 8 from the region code here:

$number =~ s/(^\+7|\D)//g;
$number =~ s/(^8)//g;

So, numbers like +7(812)315-98-83 transform into 123159883 and of course are invalid indeed.

aleksandrpanteleymonov commented 9 years ago

Actually, rebuilding didn't help. And as I see in Stub/RU.pm it should parse number like +7812..., but it doesn't.

aleksandrpanteleymonov commented 9 years ago

I found a root of the issue: RU.pm sub new { my $class = shift; my $number = shift; $number =~ s/(^+7|\D)//g; $number =~ s/(^8)//g; my $self = bless({ number => $number, formatters => $formatters, validators => $validators }, $class); return $self->is_valid() ? $self : undef; }

As you see, numbers like +7812... will be passed to validator as 12... So, the line $number =~ s/(^8)//g; should be changed, but I can't figure out how.

bigunyak commented 9 years ago

Aleksandr, have you ever read my original bug report? ;) The fix is simple, in the Number::Phone::StubCountry::RU module comment out this line:

$number =~ s/(^8)//g;

But this is gonna work right for international numbers only which start with +7.

aleksandrpanteleymonov commented 9 years ago

oh, sorry :). but we can't just comment it. we should fix Stub generator. Because this issue exists not only for Russian numbers. So we need to change generator to deal with leading 0 or 8.

For example it could generate code like this:

sub new {
  my $class = shift;
  my $number = shift;
  $number =~ s/(^\+7|\D)//g;
  my $self = bless({ number => $number, formatters => $formatters, validators => $validators }, $class);
  if ($self->is_valid()) {
    return $self;
  } else {
    $number =~ s/(^8)//g;
    $self = bless({ number => $number, formatters => $formatters, validators => $validators }, $class);
   return $self->is_valid() ? $self : undef;
  }
}
bigunyak commented 9 years ago

Well, I'm not aware of what is going under the hood and how these files get generated. But what I'm saying is that as recently as in version Number-Phone-3.0 there was no this second string which cuts out the leading 8. So something is wrong now :)

aleksandrpanteleymonov commented 9 years ago

it's for cases like +7 8 812 2132 12 23 (Russia) => +7 812 2132 12 23 or +380 0 44 444 44 44 (Ukraine) => +380 44 444 44 44. When you add leading 8/0 which you use for local numbers, but you don't need to use is for international format.

DrHyde commented 9 years ago

In "+7 8 812 2132 12 23" is the first 8 the national long distance dialling code? If it is then it shouldn't be present in the E.123 formatted number. The data in Google's libphonenumber (from which N::P::StubCountry::RU is derived) says that +7 88 is invalid, that +7 8 can only be followed by [1-7], and that +7 812 can only be followed by 7 digits.

aleksandrpanteleymonov commented 9 years ago

yes, you are right. for e.123 and e.164 it should be formated like +7 812 2132 12 23. root of the issue was that library tries to deal with cases like "+7 8 812 2132 12 23" or like "+31 06 11 22 33 44" and trying to remove leading digit for national calling system. both examples in previous sentence are incorrect, but library trying to "normalize" them to e164/whatever valid format. I fixed it with 48063f4a86d8aef4d02e9babf72518f96f6506ef. The same issue could be (or was, I don't know) for Italian numbers like +390.... Removing 0 in this case is mistake, becase 0 is a valid prefix for Italian numbers.

DrHyde commented 9 years ago

Ahh, ok, I see. The extra super long example 7 812 2132 12 23 was confusing me. Patch applied, and a test. Can you check that it's correct now?

DrHyde commented 9 years ago

Oh, and you both might be interested in joining the mailing list for Number::Phone. I intend to use it for announcements of new versions: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/number-phone

DrHyde commented 9 years ago

BTW, I think the bug was introduced when I fixed https://github.com/DrHyde/perl-modules-Number-Phone/issues/22

DrHyde commented 9 years ago

3.0004 uploaded to the CPAN with this fix

bigunyak commented 9 years ago

The fix works fine, thanks. But I've got another problem: #31 :)