option for case insensitive validation of image CAPTCHA

Hi Dave,

Thanks for your patch. Some remarks:

• patches should be in unified format, you should add the option -up to your cvs diff command (see under readability in http://drupal.org/patch/create). It's also common to add the extension .patch instead of .txt, but that's a detail.
• you have both variable_get('captcha_ignore_case_sensitivity', TRUE) and variable_get('captcha_ignore_case_sensitivity', FALSE). There should be only one default for settings variable, and it should be FALSE in this case so that the feature is disabled by default.
• You marked this feature for CAPTCHA 5.x-3.x-dev, but it seems that the you patched the version from HEAD, which is CAPTCHA 6.x-2.x-dev. For CAPTCHA 5.x-3.x-dev you should checkout the branch DRUPAL-5--3 (add the option -r DRUPAL-5--3 to your cvs checkout).
• The feature spans two modules captcha.module and image_captcha.module, but note that there are many other CAPTCHA types (e.g. see http://drupal.org/project/captcha_pack), so there could be conflicts. It's better to isolate the feature to one module.
  • The simplest option is to isolate it in image_captcha and implement a preprocess hook in image_captcha_captcha(). See the CAPTCHA api docs under "=== Optional: preprocess the response ===" for more information
  • A lot more difficult is to add the feature in captcha.module and to make it available to all CAPTCHA types in a generic way.

  I would pick the first option for now

• I think "unsophisticated users" is a bit disrespectful to all the grandma's in the world ;) I would make the description more neutral.
• Something to keep in mind for this feature is that it also should work for non Latin languages (e.g. Hebrew, Greek, ...). I don't know if strtoupper/strtolower handle non Latin languages well. This needs some research, but you can start with using strtoupper/strtolower anyway.

Don't be discouraged by this list, most items are minor issues.
If this is your first Drupal patch, you already did very well.
Thanks again

no problem, there is no rush

typically a feature is implemented in one branch and after it's finished, it gets ported to the other maintained branches. No need for a separate issue, there is a "patch (to be ported)" status

Getting closer.

Some remarks however ;)

• you use the variable captcha_ignore_case_sensitivity to store the option, it's better to use the prefix image_captcha_ for image captcha settings
• I think the expression "Ignore the user's case sensitivity in their answer" is a bit weird. I think the module has to be case sensitive or case insensitive, it's not about the users
• in image_captcha_captcha you use "variable_get('captcha_ignore_case_sensitivity')", without a default value, this should be variable_get('captcha_ignore_case_sensitivity', FALSE)
• you also change the code to lowercase before storing it for rendering, I think it's better to render the code as-is and only change the case in the expected answer.
• also try to adhere to the Drupal coding standards, your patch seems to contain tabs for indentation, the proper way is 2 spaces.

in attachment you'll find an adapted version of your patch.

committed to head in http://drupal.org/cvs?commit=116700

at#8: if you backported it, then there is a backport ;)
be welcome to post it
I'm not very active on Drupal5 anymore, but a small patch as this one wouldn't be a problem

http://drupal.org/cvs?commit=195408