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

Improve en_US area code generation #1792

Closed
wants to merge 6 commits into from
Closed

Improve en_US area code generation #1792

wants to merge 6 commits into from

Conversation

shawnlindstrom
Copy link
Contributor

The existing PhoneNumber provider does not fully implement the NANP rules for area codes. This can be problematic when running generated phone numbers against validation.

In concert with implementing the full NANP rules, this PR adds a new formatter: randomDigitNotIn(). The formatter is used here because the first and second digits of the area code cannot be 37 or 96. Since the formatter accepts an integer or array, it's convenient to pass [3, 7] or [9, 6] to get a digit exclusive of the passed digits.

This is a resubmission of #1473 which I closed asmy rebase did not not go smoothly. Sorry.

@shawnlindstrom
Copy link
Contributor Author

@pimjansen Here is my resubmission. Sorry again for not being able to make the original work. Let me know if it needs anything else.

@pimjansen
Copy link
Contributor

@localheinz @fzaninotto i think this could be useful in the randomDigitNot(x) method right? We can easiliy make it backwards compatible to accept an array as well as an int and validate it there?

Or do you guys prefer 2 different methods for it?

@shawnlindstrom
Copy link
Contributor Author

@localheinz @fzaninotto i think this could be useful in the randomDigitNot(x) method right? We can easiliy make it backwards compatible to accept an array as well as an int and validate it there?

Or do you guys prefer 2 different methods for it?

Hi Pim,

Actually, that's the approach I took in the original PR and asked the same question: #1473 (review)

And, that's why this PR includes randomDigitNotIn().

@pimjansen
Copy link
Contributor

@shawnlindstrom understand. Lets wait for the other 2 on their ideas. I do agree that a list could be very useful so. Now it is just on the implementation

if (!$pool) {
throw new \InvalidArgumentException('The argument is inclusive of the digits 0 to 9. A digit cannot be returned.');
}
$key = array_rand($pool);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prior to php 7.1, this does not use a Mersenne Twister randomizer. Use the randomElement formatter to choose an element in the array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change made as requested. The test failure is unrelated:

Faker\Test\Provider\ImageTest::testDownloadWithDefaults
getimagesize(): Read error!

@shawnlindstrom
Copy link
Contributor Author

Rebased and tests are passing. Please note this fixes #1837 and that there is an alternate PR started for the same issue open at #1838.

@pimjansen
Copy link
Contributor

We already released 1.9 last week so new features wont be added to the 1.x stream. We are now working on ideas of 2.0 so once that is clarified we can see what to do with this

@pimjansen
Copy link
Contributor

Please see #1851

@anobjectn
Copy link

Please advise: if someone wants the fix for this issue - what are the current options? Is there a patch available? Is this added to 1.9 or some other version, or do I have to wait till 2.0 for proper fake US phone numbers?

@shawnlindstrom
Copy link
Contributor Author

It's dead @anobjectn. It didn't make it into 1.9 and they froze features until they figure out what's going into 2.0. I tried hard. Sorry.

@anobjectn
Copy link

It's dead @anobjectn. It didn't make it into 1.9 and they froze features until they figure out what's going into 2.0. I tried hard. Sorry.

No need for apology sir! Info was appreciated. Thank you.

I could use a variation of the current phone number code but selecting from a given set of known valid area codes (e.g. 212,718,518) with the rest being the same (sometimes +1 country code, with and without extensions, different formatting). I may have to roll up my sleeve but more likely I'll just live with it for now (so if anyone wants to suggest something like that, share or dm).

Thanks!

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants