Problem/Motivation

#2486081: Cannot save captcha point as form id field is marked as required has been closed but the problem that it was trying to fix remains, it was just the wrong fix.

The original problem that's reported there is that the form can't be saved due to the validation. The fix that was proposed is to optionally remove the required flag.

That's not the right direction, because the label there *is* required. The problem is that the default captcha points have no label and you're forced to enter one when we create it.

Proposed resolution

The first, most important problem is that the form ID is not the form ID, the machine name is relevant. Therefore, as commented over there, the machine name should not be auto-created, the user has to manually define it. It has to be the first, primary element of the form. Yes, that's not good UX but that's not relevant. That's how captcha works, you have to enter the correct form ID or it won't do anything.

The question then is, what to do with the current label form element that's labeled as Form ID. I can see 3 options:

a) Keep it, rename to Label, move below the form ID name. But what exactly is the point of having this as a required thing? Users won't know what to enter IMHO.
b) Drop it, use the formID as the label key (additionally to using it as the ID key)
c) Make the form ID the label key, rename the current required label field to an optional description field.

Alternatively, I've been thinking about an even bigger change in the opposite direction. That we'd stop using the ID of the config entity as the form ID and match on that, but introduce a new field for the form ID. Then we could keep label/machine name more or less as it is. One interesting advantage of this would be that we could support multiple form ID's and even wildcards (e.g. node_*_form would protect *all* node forms, it would also be trivial to have a default configuration for all the forms, there's an issue for that). That does have quite some challenges (e.g. performance) and would require pretty major changes, but it could also be a huge improvement for users.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#24 improve_captcha_point-2504401-24.patch15.45 KByogeshmpawar
#24 interdiff-2504401-22-24.txt663 bytesyogeshmpawar
#22 improve_captcha_point-2504401-22.patch16.1 KByogeshmpawar
#22 interdiff-2504401-20-22.txt757 bytesyogeshmpawar
#20 improve_captcha_point-2504401-20.patch16.02 KByogeshmpawar
#13 CAPTCHA configuration Drupal 8.1.x.png81.73 KBprashant.c
#13 Add CAPTCHA point Drupal 8.1.x.png36.4 KBprashant.c
#10 captcha-point-form-2504401-9.patch16.17 KBddrozdik
#8 captcha-point-form-2504401-8.patch13.5 KBddrozdik
#6 captcha-point-form-2504401-6.patch4.54 KBddrozdik
#5 improve_captcha_point-2504401-5.patch819 bytesginovski
#1 Improve-captcha-point-form-and-properties-2504401-2.patch945 bytesprashant.c
captcha_point_form.png42.59 KBberdir

Comments

prashant.c’s picture

prashant.c
he/him
Primary language English
Location Dharamshala
commented
Version: 8.x-1.x-dev » 8.x-1.0-alpha0
Status: Active » Needs review
StatusFileSize
new Improve-captcha-point-form-and-properties-2504401-2.patch945 bytes

This issue is in alpha version also so changing the version for this issue.

Adding patch to fill machine name as FORM ID name for default forms to fix blank text field for Form Ids.

Status: Needs review » Needs work

The last submitted patch, 1: Improve-captcha-point-form-and-properties-2504401-2.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 8.x-1.0-alpha0 » 8.x-1.x-dev

Patches are always tested and committed against the dev release. I don't even know why the version lists releases, they make no sense except maybe for support questions or so.

nidhi.badani’s picture

nidhi.badani commented
Assigned: Unassigned » nidhi.badani
ginovski’s picture

ginovski commented
Status: Needs work » Needs review
StatusFileSize
new improve_captcha_point-2504401-5.patch819 bytes

b) Removed the label, renamed the machine name to Form ID.

ddrozdik’s picture

ddrozdik commented
Assigned: nidhi.badani » ddrozdik
StatusFileSize
new captcha-point-form-2504401-6.patch4.54 KB

Since we are talking about improving captcha point form, I have prepared a patch with refactored form and changes to fix an issue described above.

prashant.c’s picture

prashant.c
he/him
Primary language English
Location Dharamshala
commented
ddrozdik’s picture

ddrozdik commented
StatusFileSize
new captcha-point-form-2504401-8.patch13.5 KB

I have updated the patch and tests related to this form, to pass web tests. Please use this patch.

Status: Needs review » Needs work

The last submitted patch, 8: captcha-point-form-2504401-8.patch, failed testing.

ddrozdik’s picture

ddrozdik commented
StatusFileSize
new captcha-point-form-2504401-9.patch16.17 KB

I found some more changes related to my modifications. Here is updated patch.

ddrozdik’s picture

ddrozdik commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: captcha-point-form-2504401-9.patch, failed testing.

prashant.c’s picture

prashant.c
he/him
Primary language English
Location Dharamshala
commented
StatusFileSize
new Add CAPTCHA point Drupal 8.1.x.png36.4 KB
new CAPTCHA configuration Drupal 8.1.x.png81.73 KB

Patch submitted in #10 applies successfully.

I found another use case scenario:

1. I added new form id in admin/config/people/captcha/captcha-points/add page.
2. After adding new form this is by default enabled.
3. Instead of by default enabling the captcha on newly added form id it, Shouldn't it first display the form in listing admin/config/people/captcha/captcha-points with "Enable" button ?.

Attaching screenshots for the same.

ddrozdik’s picture

ddrozdik commented
Status: Needs work » Needs review

Patch has been failed because of this issue https://www.drupal.org/node/2783351#comment-11624043 , I guess we can review it or fix that issue and then return to this one.

@Prashant.c, regarding your comments - by default there are some points, which are available after installation the module, but they are not enabled, because module should not do any unpredictable actions before configuring it by site administrator. Every new added point will be enabled after adding, and that is fine, because administrator will take responsibility for its actions in admin panel. Not need to change this behavior.

The last submitted patch, 5: improve_captcha_point-2504401-5.patch, failed testing.

The last submitted patch, 6: captcha-point-form-2504401-6.patch, failed testing.

ddrozdik’s picture

ddrozdik commented

Patch is ready to review, I have fixed all issues related to test with cron.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs review » Needs work
Issue tags: +Novice, +Needs reroll

Could you reroll the patch please. Its not applying anymore.
error: patch failed: src/Form/CaptchaPointForm.php:11
error: src/Form/CaptchaPointForm.php: patch does not apply
Checking patch src/Tests/CaptchaAdminTestCase.php...

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: ddrozdik » yogeshmpawar
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new improve_captcha_point-2504401-20.patch16.02 KB

Rerolled the patch against branch 8.x-1.x

Status: Needs review » Needs work

The last submitted patch, 20: improve_captcha_point-2504401-20.patch, failed testing.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2504401-20-22.txt757 bytes
new improve_captcha_point-2504401-22.patch16.1 KB

Made changes against test failures. also added interdiff.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs review » Needs work
Issue tags: -Novice, -Needs reroll 
  1. +++ b/src/CaptchaPointInterface.php
@@ -27,22 +27,6 @@ interface CaptchaPointInterface extends ConfigEntityInterface {
-   * Getter for label property.
-   *
-   * @return string
-   *   Label string.
-   */
-  public function getLabel();
-
-  /**
-   * Setter for label property.
-   *
-   * @param string $label
-   *   Label string.
-   */
-  public function setLabel($label);
-

    We need an upgrade path if you are removing a property from the entity. 

  2. +++ b/src/CaptchaPointInterface.php
@@ -27,22 +27,6 @@ interface CaptchaPointInterface extends ConfigEntityInterface {
-   * Getter for label property.
-   *
-   * @return string
-   *   Label string.
-   */
-  public function getLabel();
-
-  /**
-   * Setter for label property.
-   *
-   * @param string $label
-   *   Label string.
-   */
-  public function setLabel($label);
-
-  /**

    Why you have removed these functions?

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2504401-22-24.txt663 bytes
new improve_captcha_point-2504401-24.patch15.45 KB

@naveenvalecha - i have added updated patch as per your comment #23. these two functions are unused that's why i removed these. but in updated patch i have added these functions.

Status: Needs review » Needs work

The last submitted patch, 24: improve_captcha_point-2504401-24.patch, failed testing.

couturier’s picture

couturier commented
Related issues: +#2982140: Captcha is unusable with current beta and DEV state
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

This is set as major task, but no activity since 4 years. Could someone check the status of this issue and post an update, if the issue still exists, please?

If yes, let's proceed with a review of #24.

grevil’s picture

grevil commented
Version: 8.x-1.x-dev » 2.x-dev
Priority: Major » Normal

Yes, this issue is still relevant, but I don't think it is major and can be moved to 2.x.

Haven't looked into patch #24 yet, but my suggestion based on this issue's description would be, to let the user enter the machine_name (proper form id) and generate the label from the machine name, if no label was given. This way, if the form ID is very unspecific, he can optionally provide a label for better recognition.

The extra features, @berdir mentioned, could be added in another issue.

Although if all of this is implemented in #24 and works perfectly we can easily add it here! :)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Agree with @Grevil in #28. Would someone like to work on this again and prepare a fresh MR for 2.x?

A lot of code changed since the last patch was created here.