catamphetamine / libphonenumber-js

A simpler (and smaller) rewrite of Google Android's libphonenumber library in javascript
https://catamphetamine.gitlab.io/libphonenumber-js/
MIT License
2.77k stars 217 forks source link

Wrong parsing of number #245

Closed NunoLopes12 closed 6 years ago

NunoLopes12 commented 6 years ago

I was trying to parse number with Belgium as country default code. I got some wrong results using the parseNumber(value, 'BE'). For example value = 912123123. When trying on the demo page https://catamphetamine.github.io/libphonenumber-js/ it works as expected.

I tried used the current, 1.4.4, version and an older one, 1.2.22, and still got the wrong results.

I am using this on a vuejs project. Doing: import { parseNumber } from 'libphonenumber-js';

Am i doing something wrong?

catamphetamine commented 6 years ago

Post a pull request with a test case which you think behaves not as you expect.

NunoLopes12 commented 6 years ago

I will do that asap. In the meantime I made a fiddle that mimics the results I'm getting: https://jsfiddle.net/NunoLopes/eywraw8t/343424/ Using the number: 931231231 i get a positive result on the fiddle but on the demo page it's a negative result.

Thanks for looking into it.

catamphetamine commented 6 years ago

The number is invalid https://libphonenumber.appspot.com/phonenumberparser?number=931231231&country=BE

NunoLopes12 commented 6 years ago

That is the issue, the number is invalid but I'm getting it parsed as a valid one. Updated the fiddle: https://jsfiddle.net/NunoLopes/eywraw8t/343424/

catamphetamine commented 6 years ago

“Fiddles” don’t matter, demo shows it’s invalid.

On Fri, 7 Sep 2018 at 16:25, Nuno Lopes notifications@github.com wrote:

That is the issue, the number is invalid but I'm getting it parsed as a valid one. Updated the fiddle: https://jsfiddle.net/NunoLopes/eywraw8t/343424/

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/catamphetamine/libphonenumber-js/issues/245#issuecomment-419438264, or mute the thread https://github.com/notifications/unsubscribe-auth/AAdH7-e_EUht8URwRarCOUJKkVSStxyjks5uYnO_gaJpZM4WdKPq .

NunoLopes12 commented 6 years ago

But then why is the function parseNumber extended turned true is returning the key valid as true? Because that's my original question: getting a different result from the demo. Sorry about the earlier fiddle it wasn't updated: https://jsfiddle.net/NunoLopes/eywraw8t/343939/ I will submit a test case later. Again thanks for your time.

catamphetamine commented 6 years ago

Your fiddle shows that the number is valid. The demo shows that it is invalid. Update the library version in the fiddle and see if it fixes that.

catamphetamine commented 6 years ago

@NunoLopes12 Actually, I checked and the number is valid. So it is a valid number and your fiddle shows the expected result. What's your issue?

image

catamphetamine commented 6 years ago

@NunoLopes12

I was trying to parse number with Belgium as country default code. I got some wrong results using the parseNumber(value, 'BE'). For example value = 912123123. When trying on the demo page https://catamphetamine.github.io/libphonenumber-js/ it works as expected. I tried used the current, 1.4.4, version and an older one, 1.2.22, and still got the wrong results.

What exactly is the desired outcome and what is the observed outcome and what's the exact issue you're having.

NunoLopes12 commented 6 years ago

@catamphetamine What I'm trying to do is: use parse phonenumbers with a Belgium as the default. What I observed was a difference in the resulting outcome on the demo page and on my code. I think that the demo page is parsing it correctly, but as you can see on the added image the same number that returned for you as valid returned for me, just now, as invalid. I did test the numbers with the https://libphonenumber.appspot.com/ and keep gettign the same result as the demo page.

Finally: the version I am using is the 1.4.4 tag, tried also the 1.2.0 tag. On the fiddle its importing the 1.4.3 tag.

screen shot 2018-09-07 at 14 50 18

I'm hoping my explanation isn't too confusing.

catamphetamine commented 6 years ago

I'm hoping my explanation isn't too confusing.

I still don't understand. Describe it in simple words:

NunoLopes12 commented 6 years ago

Maybe I should add that I'm working on a Mac. I'm not sure if that would trigger different results for the demo page.

catamphetamine commented 6 years ago

So, you're

?

NunoLopes12 commented 6 years ago

Exactly. Though that number is just an example, some other numbers starting with 9 + 8 numbers are also returning valid.

I noticed it by comparing with the results on the two test pages.

catamphetamine commented 6 years ago

Ok, I'll look into the issue. Seems that Belgian numbers are either 8-digit ones or 4 + 8 digits for mobile numbers. This one is 9 digits and doesn't start with 4.

NunoLopes12 commented 6 years ago

What made me raise the issue was the different results that I was getting. Thanks for looking into it.

catamphetamine commented 6 years ago

@NunoLopes12 Ok, so you have found a bug in the core regexp matching function in this library. Funny that I previously had a bug in that function which I thought I fixed but actually there was a second bug which went undiscovered until you found it. Released libphonenumber-js@1.4.5. Thx.

NunoLopes12 commented 6 years ago

Thanks for the quick fix.