Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Fix U.S. Area Code Generation (Fixes #1837) #1838

Closed
wants to merge 2 commits into from
Closed

Fix U.S. Area Code Generation (Fixes #1837) #1838

wants to merge 2 commits into from

Conversation

hackel
Copy link

@hackel hackel commented Nov 7, 2019

Ensures phone numbers generated are valid.

Regular expressions extracted from the PHP port of Google's libphonenumber via the PHP port:
https://github.com/giggsey/libphonenumber-for-php/blob/f3a55dcb6fead9d4fb291c6dadda0564e5ae8d3f/src/data/PhoneNumberMetadata_US.php#L29

@hackel
Copy link
Author

hackel commented Nov 7, 2019

It doesn't look like the ImageTest is failing because of me: getimagesize(): Read error!

@pimjansen
Copy link
Contributor

It doesn't look like the ImageTest is failing because of me: getimagesize(): Read error!

Correct, fix for this is already pushed in a PR for the 1.9.0 release.

This fix will solve #1837

@pimjansen
Copy link
Contributor

Could we maybe add some tests?

@pimjansen pimjansen added this to the 1.9.1 milestone Nov 7, 2019
@pimjansen pimjansen added the bug label Nov 7, 2019
@hackel
Copy link
Author

hackel commented Nov 7, 2019

Yeah, I wasn't sure on the best approach for testing this beyond what is already there. I could check the output against the full regex, or I could pull in libphonenumber-php as a dev dependency and check it against that.

Actually that way we could easily test all of the different locales, though I suspect that could trigger even more failures. Specifically, CA and other +1 countries should use similar logic to this.

@pimjansen
Copy link
Contributor

Yeah, I wasn't sure on the best approach for testing this beyond what is already there. I could check the output against the full regex, or I could pull in libphonenumber-php as a dev dependency and check it against that.

Actually that way we could easily test all of the different locales, though I suspect that could trigger even more failures. Specifically, CA and other +1 countries should use similar logic to this.

Well we clearly have an issue with the 96 one which should not be there. So we can test that one right? Or even all the formats that can match

@shawnlindstrom
Copy link
Contributor

Please see #1792 which I submitted a while ago. It fixes #1837. I just rebased and tests are passing.

@pimjansen pimjansen modified the milestones: 1.9.1, 1.9.2 Dec 3, 2019
@hackel hackel closed this by deleting the head repository Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants