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

Initial #1

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Initial #1

wants to merge 21 commits into from

Conversation

andrasguseo
Copy link
Collaborator

The extension adds reCAPTCHA v3 support for Community Events with the help of some template overrides.

Docs: https://developers.google.com/recaptcha/docs/v3

Copy link

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Looking good! I left some questions and comments. Thank you Andras!

src/Tec/Plugin.php Outdated Show resolved Hide resolved
src/Tec/Plugin.php Outdated Show resolved Hide resolved
src/Tec/Plugin.php Outdated Show resolved Hide resolved
* @since 1.0.0
*/
function template_base_paths( array $paths ): array {
$slug = "tec-labs-" . PUE::get_slug();
Copy link

Choose a reason for hiding this comment

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

I'd try to centralize a place for the slug, for maintenance and clarity purposes. We currently have it in the Plugin and PUE classes (for the latter, with the extension- prefix). I think for the template path it would be more friendly to use the plugin slug, but that's a personal preference. PUE is oriented to the update engine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bordoni just FYI. What do you think about this? In general, for the extensions?

src/Tec/Plugin.php Outdated Show resolved Hide resolved
@@ -0,0 +1,113 @@
<?php
Copy link

Choose a reason for hiding this comment

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

Do we have a way only to inject the pieces that we need for the reCaptcha?

If we make changes to CE forms and this extension is enabled, then this template will be out of date and maybe buggy (assuming that it will take time to update the extension if we make changes in the plugin). This could be a problem for support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@juanfra I couldn't find a way to inject things that would be needed. However, it could be done if we made some slight modifications to the original template files.

As far as I remember:

  1. The <form> needs an id.
  2. The submit button needs an extra class. We could introduce a filter to be able to add that.
  3. The submit button needs four parameters, which could be done with another filter.
  4. We need to find a way to disable the captcha.php file (which is for v2) in case we're using v3. Again, a filter can do the trick.

I know that's a lot of filters. :) That would be a fast way to have a simpler extension without template overrides.

In general it would be best to do all this in core - I'm sure you agree - so we wouldn't need an extension. I'd be happy to work on it as time allows.

@@ -0,0 +1,64 @@
<?php
Copy link

Choose a reason for hiding this comment

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

Same question about only injecting the reCaptcha pieces rather than overriding the entire template.

andrasguseo and others added 4 commits January 4, 2024 21:37
Co-authored-by: Juan Aldasoro <juanfra@theeventscalendar.com>
Co-authored-by: Juan Aldasoro <juanfra@theeventscalendar.com>
Co-authored-by: Juan Aldasoro <juanfra@theeventscalendar.com>
Co-authored-by: Juan Aldasoro <juanfra@theeventscalendar.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants