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

dev/translation#50 Make recapture library locale aware #308

Closed
wants to merge 1 commit into from

Conversation

seamuslee001
Copy link
Contributor

@civibot
Copy link

civibot bot commented Sep 14, 2020

(Standard links)

@civibot civibot bot added the master label Sep 14, 2020
@adixon
Copy link

adixon commented Sep 14, 2020

Thank for this. I wasn't convinced this is the right approach - including civi-specific code within a library seems wrong - but least we've now got a conversation. The right solution would be to update the upstream library upstream with a patch that included language in the library call as a parameter, and then include that getLocale call in the civi code that calls the library function. Or maybe there's already a better upstream version? Yeah, that's a whole new can of worms.

@homotechsual
Copy link

It's the right can of worms imo. We should avoid patching third party libraries and opt for libraries that meet our needs or upstream patches.

@homotechsual
Copy link

I'm not sure what library we're using but the official PHP lib is supported and language aware... https://github.com/google/ReCAPTCHA

@homotechsual
Copy link

Definitely in favour of the effort going towards "modern recaptcha" library vs patch ancient unsupported library.

@adixon
Copy link

adixon commented Sep 14, 2020

So, that leaves us with the work of trying to update the places in civi code that are using this newer version of the library and update the calls to what is almost certainly a more modern and complicated library ...

@homotechsual
Copy link

Work we're going to have to do anyway eventually... either when PHP makes something in this 2007 code break or when Google turn off RECAPTHA v2.

I'm going to look at what it would take to switch - the invocation of both is pretty similar and this would give us scope to introduce v3 support which has been requested.

@homotechsual
Copy link

So the library I linked is more for server-side verification. There is no client side replacement, seems like we'd be better off dropping the library and moving the functions to generate recaptcha elements directly into core.

All the library seems to be doing is inserting the HTML for the recaptcha elements?

@seamuslee001
Copy link
Contributor Author

@MikeyMJCO the library also does the post to run the validation I think as well but this pr will need to be redone once #317 is merged

@seamuslee001
Copy link
Contributor Author

closing as this is now conflicted

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

Successfully merging this pull request may close these issues.

3 participants