[2.x] Improve the CAPTCHA form markup and use twig files for more flexibility, where possible

I think the very first thing to improve, should be the formatter CSS class on the wrapper, so the module styles / theme styles are able to address the differences.

My 2 cent about the markup and the details element:

I have no idea why anyone would have the CAPTCHA in a collapsed state, so the Details element doesn't make any sense for me. I wouldn't even add this as an option. If someone needs this, its easy to override the Twig file.

In my opinion, every single CAPTCHA element should dropped as variable to the Twig template. All Markup should live inside this template file.

And for sure, the text "Answer this question to verify that you are not a spam robot." needs a wrapper then.

@thomas.frobieter please have a look at the changes once again.

These changes currently break the module with the following error:

Twig\Error\SyntaxError: Unknown "clean_class_array" filter. in Twig\ExpressionParser->getFilterNodeClass() (line 22 of modules/custom/captcha/templates/captcha.html.twig).

After a quick google search I found out, that we use the correct sanitization syntax, so I am not sure why the filter can not be found. 🤔

But I have no experience with templates, so maybe @thomas.frobieter can have a look!

Oh yeah, the use of the clean_class_array fiter was a mistake. This filter is n/a in Drupal Core, its a contrib module: https://www.drupal.org/docs/8/modules/twig-tools/sanitization-filters

I'll change this, give me a second.

Thx! I'll have a second look tomorrow, but can already tell from the tests and a quick peek at my local captcha login form, that this definitely still needs work!

@thomas.frobieter:

Allow to change the order / alignment of the captcha elements, for example beside or below each other by CSS

this and allowing to hide the descriptive label (which we had hidden in our custom css) would be a nice setting I guess?

Any ideas how we could add that and how the setting could be named and look like?

CAPTCHA alignment?
Hide label?

@anybody: TBH, this sounds very unnecessary to me. This would be a global setting? But the CAPTCHA appears in multiple forms, various widths .. so I don't know, seems useless if not configurable for each CAPTCHA instance.

I think we better should ensure a solid default layout, what is very flexible/responsive by default. So for example the image captcha: Image left, input & text right while theres enough width, Image above if not.

We can do this with CSS Grid or Flexbox very easily, so no need for setting, IMO.

OK, I couldn't finish this, as I had internal debugging problems with my dev environment.

The point is, that we need to additionally regard the "persistence" setting. This is considered in our FormElement, through "_captcha_required_for_user()" but not in our preprocess hook. So we need to get the required variables for the "_captcha_required_for_user()" function inside the preprocess hook and only render title and description if it returns true. Otherwise, if somebody already fulfilled the captcha but put in a wrong username, the captcha element and type is hidden, but the title and description are still being displayed.

OK all done! Please review!

@Anybody, especially check the update hook! For users which disabled the description, but still have a description string set, this update will delete that description. We could possibly notice the user about this, inside the update hook, but doubt anybody has important text inside the description worth noticing about.

Good thing this wasn't merged yet! There is a BIG mistake using "delete($param)" for the old config entry instead of "clear($param)" which leads to deleting the ENTIRE config object instead of the single object entry!! I am really sorry for not testing this update hook beforehand (won't happen again).

Fixed the issue, see attached interdiff.

Rebased the branch on 2.x, Tests are all green locally.

I'll take care about this when if have the time.

Rebased on current 2.x.

Okay I am ready with the image captcha styling. I've added a further wrapper in the form, to group the image and the reload link.

I've also done some cleanup, separated admin scripts & styles, created a libraries file + CSS for the captcha module itself.

The reload link now is a simple reload-icon instead text, to reduce clutter. Maybe this will need some accessibility love, but I think the image captcha itself is an accessibility nightmare, so ...

Rebased the branch! Twig file had a merge conflict 12 times, so I just fixed it afterwards :)

I'll check everything carefully now and see if anything broke!

Tests are green and code looks good!

Although, we should definitely split such big issues in several smaller issues. Trying to review so many changes at once, can lead to overseeing problematic code.

Furthermore, trying to rebase this issue branch was an utter pain and leads to time-consuming actions. Note that rebasing means, that for every single commit done in dev, we have to reverse ALL commits done here, add the commit from dev and then ADD all commits from this issue branch again. Those are 31 commits now and counting. Meaning worst case, we could have 31 merge conflicts for EVERY SINGLE commit done in dev, before committing this!

So let's break it apart next time!

OK, on to the manual functionality test! :)

Just a small reminder, as I couldn't remember, why 2.x is D10 only.

The reason behind this is, that besides the type hinting introduced in the newest Symfony release, used for D10, there were also method implementation changes (See https://git.drupalcode.org/project/captcha/-/merge_requests/47/diffs#1f6...).

That's why the newest release is D10 only! 👍