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

isValidNumber returns false for valid german numbers #235

Closed bittermanq closed 6 years ago

bittermanq commented 6 years ago

Starting from version 1.3.1 isValidNumber returns "false" for valid german numbers like +49 171 xxxxxxx +49 176 xxxxxxxx +49 1575 xxxxxxx

UPD: I tested it with following numbers: +491711434816 +4917657887287 +4915752084769 All are valid in 1.3.0 and invalid in 1.3.1

catamphetamine commented 6 years ago

Google's metadata for German numbers was updated in version 1.3.0 -> 1.3.1. https://github.com/googlei18n/libphonenumber/commit/80db0ee78a5580f1317acfb4df7c0362de69bace#diff-e918a2e051ae7dc86df09f1f07534667L6454 In Google's library it's part of "Metadata updates for release 8.9.11".

I don't know whether the new rules are wrong, I suppose they are. You can debug it if you want. In case you don't agree with the new rules for German numbers in metadata then I'd suggest creating an issue in Google's repo https://github.com/googlei18n/libphonenumber But they don't seem to have a way of reporting issues. Or you could investigate the issue yourself using the metadata for Germany.

I looked very briefly at that and before the regexp was something like

[1-35-9]\d{3,14}|...rest...

And after it became

(?:1|[358]\d{11})\d{3}|[1-35689]\d{13}|4(?:[0-8]\d{5,12}|9(?:[05]\d|44|6[1-8])\d{9})|[1-35-9]\d{6,12}|49(?:[0-357]\d|[46][1-8])\d{4,8}|49(?:[0-3579]\d|4[1-9]|6[0-8])\d{3}|[1-9]\d{5}|[13-68]\d{4}

I won't be looking at that because I suppose it's an issue in Google's metadata, or maybe you're just supplying wrong phone numbers. You can investigate if you want.

I'm leaving this open so that others could possibly share their ideas or whatever else.

cc @antonversal

catamphetamine commented 6 years ago

@bittermanq You should also provide a valid phone number that's valid for 1.3.0 and invalid for 1.3.1 and you haven't provided any.

bittermanq commented 6 years ago

@catamphetamine you mean providing full numbers, not templates from the first message?

catamphetamine commented 6 years ago

@bittermanq I meant you didn't supply a phone number for me to test against. If you're reporting an issue then report it with the proper details.

I used the phone @antonversal supplied — +4915784846111‬. It is valid for 1.3.0 and is invalid for 1.3.1. I did a test and found out that there is a bug: the new regexp broke my code by introducing a non-standard regexp matching case, so it's a combination of two things: my code and metadata update. I'll fix it.

bittermanq commented 6 years ago

@catamphetamine added numbers to initial message

catamphetamine commented 6 years ago

@bittermanq No need now.

Summary: this was a bug in my code — matches_entirely() function was called incorrectly throughout the code. Fixed it in the relevant commit. @antonversal 's bug report and @bittermanq 's bug report were correct.

Published libphonenumber-js@1.4.2.

adri commented 6 years ago

Thanks for fixing this so quickly! :)