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

The code seems to use the I18n::locale() if the param lang is empty, but the ConfigValidator doesn't allow us to set an empty string #2

Open
Xety opened this issue Dec 15, 2015 · 3 comments
Labels

Comments

@Xety
Copy link
Member

Xety commented Dec 15, 2015

The code seems to use the I18n::locale() https://github.com/cakephp-fr/recaptcha/blob/master/src/View/Helper/RecaptchaHelper.php#L64-L67 if the param lang is empty (btw it's really a good idea), but the ConfigValidator doesn't like when we don't specify any lang option : https://github.com/cakephp-fr/recaptcha/blob/master/src/Validation/ConfigValidator.php#L93-L98

Also, maybe set the lang param to be empty by default or to use the I18n::locale()? https://github.com/cakephp-fr/recaptcha/blob/master/src/View/Helper/RecaptchaHelper.php#L40

Also, we probably should do a substr(I18n::locale(), 0, 2)) to get the I18n::locale() value because the locale value with I18n::locale() can also be full named such as fr_FR, en_US etc.

Edit :
After some more tests, by removing the lang option in the config, the validator pass, but the option is defined to en : https://github.com/cakephp-fr/recaptcha/blob/master/src/View/Helper/RecaptchaHelper.php#L40 so the condition :

$lang = $this->config('lang');
if (empty($lang)) {
        $this->config('lang', I18n::locale());
}

is always false.

@Xety Xety added the bug label Dec 15, 2015
@cake17
Copy link
Member

cake17 commented Dec 20, 2015

@Xety you're right, maybe we could define the lang to be I18n::locale() by default.
If no accepted lang is provided (with I18n::locale or through the helper), do we define the lang to en, or do we throw an exception?

EDIT
Google Recaptcha also accepts lang like en-GB so maybe substr would remove these possibilities ? We should for sure transform the - into a _

I started to reorder the ConfigValidation and divide it into 2 separate validation:

  • one globally for the Plugin : we could use it in bootstrap to check secret and all other params 'lang', 'type', ...
  • one for the recaptcha helper : only the params 'lang', 'type', ...

Also maybe the recaptcha.php config file is not needed, and we could just add the config to app.php ?

@Xety
Copy link
Member Author

Xety commented Dec 21, 2015

For the lang option like en-GB, yeah like you said, the best solution would be to transform them into en_GB, so we will follow the CakePHP conventions :

Translation folders can either be the two letter ISO code of the language or the full locale name such as fr_FR, es_AR, da_DK which contains both the language and the country where it is spoken.

Well, actually, it would be the inverse, let me show you step by step how to check it :
I18n::locale() result : (These tests should be the same for the lang option)

  • en_GB :
      1. en_GB has a _
      1. Transform en_GB into en-GB
      1. Matching en-GB in the allowed array
  • en-GB
      1. en-GB has a -
      1. Matching en-GB in the allowed array
  • en
      1. en has not a _ or a -
      1. Matching en in the allowed array
  • fr_FR :
      1. fr_FR has a _
      1. Transform fr_FR into fr-FR
      1. Not matching fr-FR in the allowed array
      1. Doing a substr => fr
      1. Matching fr in the allowed array
  • fr-FR :
      1. fr-FR has a -
      1. Not matching fr-FR in the allowed array
      1. Doing a substr => fr
      1. Matching fr in the allowed array
  • fr
      1. fr has not a _ or a -
      1. Matching fr in the allowed array
  • zz
      1. zz has not a _ or a -
      1. Not matching zz in the allowed array
      1. Use the default en

For the fr-FR, it's like you want, we can do a substr like in my example, or we can refusing it and use the en. It's like you want.

For the default locale, i think your solution is the best solution. For me, it would be better to use en as default locale if the I18n::locale() & the lang option has a bad locale.

@anhtuank7c
Copy link

You all should try this plugin: recaptcha

You can define your own lang or just leave blank to use your default application locale

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

No branches or pull requests

3 participants