Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wundo’s picture

Status: Needs review » Postponed (maintainer needs more info)

How does this relates with the patch I've just committed for the 7.x branch?

elachlan’s picture

Status: Postponed (maintainer needs more info) » Needs work

It still needs work I think. I will get around to it eventually.

The patch will need to be re-rolled to the 8.x branch.

elachlan’s picture

Port from git, see issue: #255795: Add CAPTCHA to all forms by default.

Will roll patch soon.

naveenvalecha’s picture

ddrozdik’s picture

Assigned: Unassigned » ddrozdik
ddrozdik’s picture

Status: Needs work » Needs review
FileSize
4.27 KB

Here is a patch which adds needed feature. This patch does not have any tests, since all tests for the settings form will be added here https://www.drupal.org/node/2802617

nirvana_u’s picture

Status: Needs review » Needs work

Hi @ddrozdik,

Thanks for looking into this. I have applied the patch on my d8 environment with captcha (8.x-1.x-dev) version. But unable to get positive result. This needs to be re-work.

Please let me know if more clarity needed and we can discuss further.

Hope this helps.

Thanks & Regards!!!

Ivan Berezhnov’s picture

Issue tags: +CSKyiv18
fnuppy’s picture

Assigned: ddrozdik » fnuppy
Status: Needs work » Needs review
FileSize
2.67 KB

Status: Needs review » Needs work

The last submitted patch, 9: add-captcha-to-all-forms-2025181-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

fnuppy’s picture

fnuppy’s picture

Status: Needs work » Needs review
fnuppy’s picture

ApacheEx’s picture

Status: Needs review » Needs work

Here is some coding standard issues:

  1. +++ b/captcha.module
    @@ -162,6 +162,11 @@ function captcha_form_alter(array &$form, FormStateInterface $form_state, $form_
    +    if(!$captcha_point && $config->get('enabled_default')){
    +        $captcha_point = captcha_set_form_id_setting($form_id, $config->get('default_challenge'));
    +        $captcha_point->enable();
    +    }
    

    1.1. There should be space before (
    1.2. Wrong indent (should be 2 spaces, but I see 4)

  2. +++ b/src/Form/CaptchaSettingsForm.php
    @@ -92,6 +92,13 @@ class CaptchaSettingsForm extends ConfigFormBase {
    +      //Option for enabling CAPTCHA for all forms.
    

    2.1. Unnecessary spaces before //
    2.2. There is not space after //

fnuppy’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
ApacheEx’s picture

Status: Needs review » Needs work

I've reviewed more in details (hope it's clear):

  1. +++ b/captcha.module
    @@ -162,6 +162,11 @@ function captcha_form_alter(array &$form, FormStateInterface $form_state, $form_
    +    if (!$captcha_point && $config->get('enabled_default')) {
    +      $captcha_point = captcha_set_form_id_setting($form_id, $config->get('default_challenge'));
    +      $captcha_point->enable();
    +    }
    

    I think it should be done in another approach. Here you create an entity for each form which stores in db.
    If I disable "enabled_default" option, all entities (CaptchaPoint) which were created before won't be removed.
    That's a problem. You do not need to create a CaptchaPoint.

  2. +++ b/config/install/captcha.settings.yml
    @@ -1,3 +1,4 @@
    +enabled_default: 0
    

    can be just `false`

  3. +++ b/config/schema/captcha.settings.yml
    @@ -5,6 +5,9 @@ captcha.settings:
    +      type: integer
    

    better to use `boolean`

  4. +++ b/config/schema/captcha.settings.yml
    @@ -5,6 +5,9 @@ captcha.settings:
    +      label: 'Add captcha to all forms.'
    

    captcha => CAPTCHA

fnuppy’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

Thx, I fixed this problem, I just use fake instance without saving.

ApacheEx’s picture

Assigned: fnuppy » Unassigned
Status: Needs review » Reviewed & tested by the community

Many thanks, looks good for me and do not see any other issues so far.

  • elachlan committed 0d47a03 on 8.x-1.x authored by fnuppy
    Issue #2025181 by fnuppy, ddrozdik: Add CAPTCHA to all forms by default
    
elachlan’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.