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

Fix pattern for mobile phone number #6161

Closed
wants to merge 3 commits into from
Closed

Fix pattern for mobile phone number #6161

wants to merge 3 commits into from

Conversation

neilime
Copy link
Contributor

@neilime neilime commented Apr 18, 2014

The current pattern allows to much long mobile phone numbers, the new one fixes it.

The current pattern allows to much long mobile phone numbers, the new one fixes it.
@Martin-P
Copy link
Contributor

The current pattern allows to much long mobile phone numbers, the new one fixes it.

Can you explain what you are trying to fix? The alteration you made only adds a matching group and thus uses more memory. It does not however change anything about the length issue you are talking about, because the regex itself is still the same as before.

Edit: sorry, it seems it does fix the length issue

@@ -13,7 +13,7 @@
'national' => array(
'general' => '/^[124-9]\\d{8}|3\\d{3}(?:\\d{5})?$/',
'fixed' => '/^[1-5]\\d{8}$/',
'mobile' => '/^[6-7]\\d{8}|7[5-9]\\d{7}$/',
'mobile' => '/^([6-7]\\d{8}|7[5-9]\\d{7})$/',
Copy link
Contributor

Choose a reason for hiding this comment

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

You can make it a non-matching group and save memory:

'/^(?:[6-7]\\d{8}|7[5-9]\\d{7})$/'

@Ocramius
Copy link
Member

This needs an additional test case.

@neilime
Copy link
Contributor Author

neilime commented Apr 30, 2014

Done.

@@ -13,7 +13,7 @@
'national' => array(
'general' => '/^[124-9]\\d{8}|3\\d{3}(?:\\d{5})?$/',
'fixed' => '/^[1-5]\\d{8}$/',
'mobile' => '/^[6-7]\\d{8}|7[5-9]\\d{7}$/',
'mobile' => '/^(?:[6-7]\\d{8}|7[5-9]\\d{7})$/',
'tollfree' => '/^80\\d{7}$/',
'premium' => '/^3\\d{3}|89[1-37-9]\\d{6}$/',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what is a premium number, but the regex may have the same issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some failing cases in tests would have detected the problem.

@Ocramius Ocramius added this to the 2.4.0 milestone Jul 27, 2014
@Ocramius Ocramius self-assigned this Jul 27, 2014
@Ocramius
Copy link
Member

@neilime manually merged @db18d96964440840b20e6f9510e52c370092b4ce, thanks!

@Ocramius Ocramius closed this Jul 27, 2014
gianarb pushed a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015
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