This repository has been archived by the owner on Aug 21, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 71
Parsing US number succeeds in tests fails in actual usage #4
Open
ajayrungta
wants to merge
16
commits into
davideme:master
Choose a base branch
from
ajayrungta:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Updates namespace from com\google\i18n\phonenumbers to libphonenumber * Updates comments & README for namespace change * Moves multiple classes in single file to individual classes * Adds use & require_once statements in tests, build & demo.php accordingly
Mobile pattern update to match 8551###### as valid indian mobile number
Mobile pattern update to match 8747###### as valid indian mobile number
Mobile pattern update to match 8740###### as valid indian mobile number
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
US number parsing fails because different data is used for countryCallingCodeToRegionCodeMap while testing and actual usage. The mapping for global networks (880 => 001) is present in countryCallingCodeToRegionCodeMapForTesting but not in countryCallingCodeToRegionCodeMap.
When the init() is called inside PhoneNumberUtil::getInstance(), it removes US from the supportedRegions array.
At line, http://git.io/CLseew#L235
unset($this->supportedRegions[array_search(self::REGION_CODE_FOR_NON_GEO_ENTITY, $this->supportedRegions)]) resolves to
unset($this->supportedRegions[array_search(001, $this->supportedRegions)])
Because the mapping doesn't exist for 001, array_search returns FALSE executing unset($this->supportedRegions[FALSE].
As unset(array[FALSE]) is equivalent to unset(array[0]), it unset the entry for US from the supportedRegions as index for US is 0 in the array.
Hence, parsing US number results into error.
I have attached a possible solution. Please validate and pull.