cakephp / localized

I18n and L10n related CakePHP code
Other
213 stars 179 forks source link

Why don't you allow intl phone validation? #154

Closed 2ndGAB closed 7 years ago

2ndGAB commented 7 years ago

Hello,

According to the fr validation code, you gave up international validation and so entering phone number like +331234567890 or 0033 1234567890 fails. Isn't it possible to allow a + sign (and so 2 more numbers) in front of a phone number?

And in fact, I suppose that allowing + is available in all languages no?

antograssiot commented 7 years ago

Sure can you provide a PR ?

inoas commented 7 years ago

Maybe this better should be in cakephp/cakephp - e.g. one simple rule to validate international phone numbers, having one lookup table with all 00xx/+xx codes. Then you could utilize that as a validation rule where you say "if valid as either international-number or this local number". Else I fear that it would be duplicated here among many languages.

Edit: The international number validator could also have an optional list parameter which would take what kind of prefixes are allowed, as plus or double zero (e.g. 33, 41, with + or 00)

antograssiot commented 7 years ago

no, it is good in the plugin, it as been extracted of cakephp/cakephp already.

inoas commented 7 years ago

@antograssiot isn't international phone number validation e.g. just off the +xx / 00xx format very generic opposed to "localized"?

antograssiot commented 7 years ago

A generic rule will allow to many false positive. The number of digit would still vary and the first allowed digits would also vary a lot. Handling it by country on demand sounds a better plan to me. Ifor one needs something more general, some libs already try to provide that and you can use them in custom rules.

2ndGAB commented 7 years ago

So I propose this regular expression to match French and DOM/TOM phone number formats validation, based on wikipedia.

2ndGAB commented 7 years ago

I've updated code and testcode and so I wanted to test it. I've never used phpunit test cases until now. So I installed phpunit with composer and everything worked fine.

me@vps308391:/var/www/myExample$ php composer.phar require --dev phpunit/phpunit
Using version ^6.0 for phpunit/phpunit
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 24 installs, 0 updates, 0 removals
  - Installing sebastian .....

sebastian/global-state suggests installing ext-uopz (*)
phpunit/phpunit-mock-objects suggests installing ext-soap (*)
phpunit/php-code-coverage suggests installing ext-xdebug (^2.5.1)
phpunit/phpunit suggests installing phpunit/php-invoker (^1.1)
phpunit/phpunit suggests installing ext-xdebug (*)
Writing lock file
Generating autoload files
> Cake\Composer\Installer\PluginInstaller::postAutoloadDump

But when I try to start the tests, I get and error:

me@vps:/var/www/myExample$ vendor/bin/phpunit vendor/cakephp/localized/tests/TestCase/IntegrationTest.php
PHP Fatal error:  Class 'PHPUnit_Framework_TestCase' not found in /var/www/myExample/vendor/cakephp/cakephp/src/TestSuite/TestCase.php on line 32

Fatal error: Class 'PHPUnit_Framework_TestCase' not found in /var/www/myExample/vendor/cakephp/cakephp/src/TestSuite/TestCase.php on line 32

I saw this discussion but I think it's ok for me; in my composer.json, I see:

"require-dev": {
    "d11wtq/boris": "1.0.*",
    "phpunit/phpunit": "^6.0"
},

Is require-dev ok ?

Any idea?

antograssiot commented 7 years ago

You should require phpunit ^5.7 for now.

antograssiot commented 7 years ago

@2ndGAB Tell me if you need help with this, I'll happily guide you (even in french if required).

2ndGAB commented 7 years ago

@antograssiot Perfect thanks a lot.

So now I tried to create branch 'issue-154' and then commit my changes but it seems I've no permission to do it.

antograssiot commented 7 years ago

@2ndGAB if you are trying to push your branch directly on this repository, indeed you're not allowed. My bet is that you clone directly the cakephp/localized repository which is not the "correct" way of doing it, but it can be corrected pretty quickly.

What you need to do at this point is to fork the repository (see the button on the top right corner of this page

Then in your cloned folder, you need to update the origin. To do so you should run

cd somefolder/localized
git remote set-url origin https://github.com/2ndGAB/localized.git

then you can also add a the upstream with (to help keeping your fork up to date

git remote add upstream https://github.com/cakephp/localized.git

Then push your branch on your fork

git push origin issue-154

And open a PR from your fork to the main repository.

You can have a look to https://book.cakephp.org/3.0/en/contributing/code.html if you're struggling or just post another comment and we will help you going through this process

2ndGAB commented 7 years ago

@antograssiot Ok, thanks a lot so I suppose you should see my pull request. Please tell me if everything is ok.

antograssiot commented 7 years ago

Closing as #158 is open now