Skip to content
This repository was archived by the owner on Aug 17, 2021. It is now read-only.

Conversation

reduardo7
Copy link
Contributor

@reduardo7 reduardo7 commented Nov 30, 2016

Fixes #167, #141

@TheSharpieOne
Copy link
Contributor

Very interesting. This trigged recaptcha to fetch the language specific version of the script. (Previously, people have been trying to swap the scripts) I am not sure if that is a new capability or not.
Looking at how you accomplished this, you just set hl in the widget's internal recaptcha config. Doing this from the start (passing hl along with the config to render) appears to have the same affect... meaning there is an undocumented feature to set the language baked right into reCaptcha (not too sure if that is new or not).
The method you are using is very fragile though, it depends on internal script variables and those are not guaranteed to stay the same version to version. In fact, b/c the script is minified and mangled, I would be will to bet that this would break when there is an update to reCaptcha.
Basically, remove all of the hackiness which dives into window.___grecaptcha_cfg and it will still work but without being able to change an existing widget's language. You could watch for the lang to change and remove the existing widget and create a new one to have the ability to change the language of an 'existing widget' without going into the internal workings o recaptcha itself.

src/service.js Outdated
* Set reCaptcha language
*/
setLang: function (widgetId, lang) {
var cli = window.___grecaptcha_cfg.clients[widgetId];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can find this object at: https://www.google.com/recaptcha/api.js

Copy link
Contributor

@TheSharpieOne TheSharpieOne Nov 30, 2016

Choose a reason for hiding this comment

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

things that start with _ and especially __ and even more so ___ are internal methods and should not be relied upon by third parties as they can change or disappear. window.grecaptcha is the official api.
Furthermore, variable names like W and wk should not be replied upon because those are generated by the build process and will change build to build, version to version.

@iambrosi
Copy link
Contributor

@reduardo7 First, I want to thank you for the PR. I subscribe to what @TheSharpieOne mentioned, and I would like to add that all your changes must be performed on the files in the src dir. The files located in the release dir are generated during the release process that. For this same reason, you must not commit the files in the release dir.

Lastly, it would be awesome to have some tests included in the PR for the new feature.

@reduardo7
Copy link
Contributor Author

@iambrosi and @TheSharpieOne : I have made the suggested changes, but I notice that the automatic integration test is failing for something that does not seem to be related to my work.
If you notice anything else that needs adjusting, do not hesitate to contact me.

@TheSharpieOne
Copy link
Contributor

The issue is with the CI, travis.yml is set to use the latest stable version of node. That combined with out-dated devDep makes it not work.
I put up #171 to address that. If you want, once that gets merged, rebase this PR to include it so that travis CI will pass.

Also, I did test the merge result of this PR locally with the updated devDeps; everything passed.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a5a1f25 on reduardo7:master into ** on VividCortex:master**.

Copy link
Contributor

@TheSharpieOne TheSharpieOne left a comment

Choose a reason for hiding this comment

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

LGTM

@iambrosi iambrosi merged commit eb89acd into VividCortex:master Dec 2, 2016
@iambrosi
Copy link
Contributor

iambrosi commented Dec 2, 2016

Thanks @reduardo7 !

@reduardo7
Copy link
Contributor Author

reduardo7 commented Dec 7, 2016

@iambrosi I agree with #170 (comment), but now the release dir don't contain the minified code. Should I make this missing commit? Or you should do it?

Issue: #173

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants