Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to en-US if locale is not found #488

Closed
wants to merge 1 commit into from

Conversation

oss92
Copy link
Contributor

@oss92 oss92 commented Feb 8, 2016

Fixed the isAlpha and isAlphanumeric methods to support locales that are not found by defaulting to en-US.

  • Old code: If locale is en-GB for example, alpha[locale] will be undefined and will output an error Cannot read property 'test' of undefined.
validator.isAlpha = function (str, locale) {
        locale = locale || 'en-US';
        return alpha[locale].test(str);
};
  • New code: If alpha[locale] is undefined, alpha['en-US'] will be used instead.
    validator.isAlpha = function (str, locale) {
        var regex = alpha[locale] || alpha['en-US'];
        return regex.test(str);
    };

@oss92 oss92 force-pushed the default_english branch 2 times, most recently from a52c5b1 to 6f72fb1 Compare February 8, 2016 12:23
@chriso
Copy link
Collaborator

chriso commented Feb 8, 2016

Passing an invalid locale should be an error, or should at least cause it to return false like it does in the isMobilePhone() validator. Silently replacing an invalid locale with en-US will cause unexpected results.

The problem is with sequelize, which should not be passing true as the second argument (sequelize/sequelize#5377).

@chriso chriso closed this Feb 8, 2016
@oss92
Copy link
Contributor Author

oss92 commented Feb 9, 2016

It was not really about sequelize. What about en-GB for example? Which will return an error even though it is the same character set as en-US.

The general case is that alphanumeric characters (POSIX/C locale) are 0-9A-Za-z. Also since it is already locale || 'en-US' which fallbacks to en-US is no locale was assigned then it makes sense that if the locale was not found to default to en-US.

Also your old code before localized alphanumerics and alphas was only en-US.

This will produce some problems with languages with unicode characters. (Chinese, Hebrew, Arabic etc..) but is performant on most other languages.

@chriso
Copy link
Collaborator

chriso commented Feb 9, 2016

What about en-GB for example?

en-GB (en-AU etc) should be an explicit alias for en-US. I'd be happy to accept a PR to add additional locales/aliases.

diff --git i/validator.js w/validator.js
index c48fa9e..2662961 100644
--- i/validator.js
+++ w/validator.js
@@ -79,6 +79,9 @@
       , decimal = /^[-+]?([0-9]+|\.[0-9]+|[0-9]+\.[0-9]+)$/
       , hexcolor = /^#?([0-9A-F]{3}|[0-9A-F]{6})$/i;

+    alpha['en-GB'] = alpha['en-US']
+    alphaNumeric['en-GB'] = alphaNumeric['en-US']
+
     var ascii = /^[\x00-\x7F]+$/
       , multibyte = /[^\x00-\x7F]/
       , fullWidth = /[^\u0020-\u007E\uFF61-\uFF9F\uFFA0-\uFFDC\uFFE8-\uFFEE0-9a-zA-Z]/

The general case is that alphanumeric characters (POSIX/C locale) are 0-9A-Za-z.

Agreed. This is why when you don't specify a locale, it defaults to en-US. This was a backwards-compatible change from #477. If you want the default locale, don't specify one! Sequelize is calling validator.isAlphanumeric(string, true) which is invalid: the interface is clearly defined and true is not a locale.

it makes sense that if the locale was not found to default to en-US.

Completely disagree. You're suggesting that validator.isAlpha('foobar', 'zh-CN') should silently change zh-CN to en-US and then return true.

@oss92
Copy link
Contributor Author

oss92 commented Feb 9, 2016

That was a fruitful conversation. Agreed 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants