[feedback] Patch: big rewrite op Captcha module + additional patches

Also, in the future, try to create a patch/diff when submitting code

I know, but I added a file (captcha.install) and I couldn't add it to my CVS working copy because I have no commit permission (with SVN this would be possible though). Consequently I couldn't get a diff patch with the new file included.
But on that CVS info page you mentioned (http://drupal.org/patch/create) there is a workaround for it.
In attachment: my work as a diff patch.

thanks for testing

in attachment: a small additional (incremental) patch for my module rewrite that solves minor bugs

in attachment an additional (incremental) patch with some cosmetic changes: changed some UI strings, updated queries to drupal code convention, minor t() changes.
No functionality was changed

In my previous additional patch (captcha-r12.patch) there is a stupid syntax error, do not use it.
Use the patch from this reply (captcha-r13.patch)

I did some further research and this patch should also solve issues http://drupal.org/node/148220 (added an option for this) and http://drupal.org/node/148220

Yet another additional patch: solved minor bug in installation procedure and added some default form_ids

Would be better if you add some comments to your code. ;)

Well, the original captcha.module does not contain much comments either ;), but that's not a reason to neglect it myself.

And I'd prefer if you do just one fixes/feauture by patch and if you break issues like that in many parts :)

I understand, but because I had to change so much (going from set_variables/get_variables to a dedicated table is not that trivial), I decided to do a big rewrite.

Sorry, but I prefer keeping form store dependency, isn't captcha function to discover forms.

I understand. It isn't indeed captcha function to collect forms, but because I introduced the table captcha_points the form_store functionality had no use anymore (only administration overhead).
But I'll look more into this, and look if I can't "outsource" some functionality to form_store again.

Thanks for your time

Also this simply doesn't work for me. Once I'd changed the SQL error and got things installed, I could do administrative tasks, but the CAPTCHA never appeared on the contact form (standard drupal contact module) I wanted it to.

note that captcha's are only presented to anonymous users and not authenticated users, so be sure to log out to test it (or use another browser/client). When "Add captcha adminstration links to forms" is enabled, users with the "administer site configuration" role should see some adminstrative message on the place of the captcha though.

thanks for testing

Sorry, but I prefer keeping form store dependency, isn't captcha function to discover forms.

I looked into this and reworked my version of the captcha module. I slimmed down the captcha_points table to only two fields: form_id and captcha_type. These are the bare essentials for the baseline captcha functionality. Extra attributes (like a description or visibility) should be implemented by the form_store module. I reworked the module code to fit the reduced table. I kept the "captcha administration links" functionality, because it is not that much code but offers a lot more usability (aka "less clicking") for the captcha module than the form_store can/will provide.

During testing my module, I observed that I don't need the form_store module anymore. So I think we can drop the form_store module as a dependency (it is not a real dependency, but for the original module it is needed if the default captcha points are not enough). I think it is a good thing to have less dependencies.
Consequently I did not implement calls to form_store functionality in my version. It is possible though to do it, but I don't see real benefits worth the hassle.

In attachment is the diff patch between CVS version 1.42 of captcha.module and my last version (revision 15, I keep my code in a bazaar repository btw). This patch overrules all previous patches in this thread/issue (it is not incremental).

bye

Ok, it's late, I should go to sleep
ignore the patch file from my previous post (#16)
and use the one from this post
(which does not contain a stupid syntax typo) ;)

Hi again,

I worked further on my rewrite of the captcha module. I'm at revision 30 now (I branched from CVS version 1.41). Because there changed so much, it does not make really sense to post a diff between CVS1.41 and my version (the diff is bigger than the new version on its own). On the other hand, I understand that the module maintainer(s) do not want to overwrite the existing version with my version just like that.

In attachment i've put a tar.gz with the best of all worlds:

• Plain files captcha.module, captcha.install and captcha.info for people that want to test, but don't want to do the patching and have no problem with overwriting the existing module. It's possible that it's needed to do an install-uninstall-install (or something) of the module, because the existing version does not use a install file and my version does (I think). I guess that's the problem monkeybeach mentions in #15. I had the problem too the first time after i added the install file.
• 29 patchfiles in the directory patches for the incremental changes between my 30 revisions (starting from CVS1.41).
  The file captcha-rewrite.log contains the revision log of the incremental changes.
  1 patchfile for the diff between CVS1.41 and my last revision (30).

The most important changes since my previous post are roughly:

• placement of captcha just above submit button works in more scenario's (e.g. comments with required preview or when other hook_form_alter modules are in play)
• lots of code cleanup/restructuring, code documentation
• usability stuff with captcha adminstration links: added redirection to original page after enabling captcha or editing captcha type
• made captcha persistence on by default

I hope most of my work will make it to the official version
sincerely

soxofaan

I realize I am a day late and way short.... but if we are having that many issues with this module that we are re-writng it frm scratch

Well, I didn't start writing it from scratch, I just started rewriting parts of it. But in the end, nothing much has been left over from the original module :)

- is it worth merging/converting to Heine's MyCaptcha module http://heine.familiedeelstra.com/mycaptcha-download

Will MyCaptcha be hosted on drupal.org? If so, I'm in favour of merging/unifying/streamlining things (captcha, recaptcha, captcha riddler, textimage, mycaptcha, my patches,...). I have not much experience with mycaptcha, but at first sight it looks a bit more advanced that what I tried to make because of the image captcha generation.
With this in mind, we could maybe create a captcha_light (or captcha_basic or wathever) out of my rewrite? It's light because there are no dependencies, but it's very functional and usable however. Heine's MyCaptcha could then depend on captcha_light and implement the image captcha stuff and replace or use the textimage functionality?

I finally hit the wall with captcha - where too many users were complaining that it didn't work and tried it.

it is indeed a frustrating situation. Certainly in this era of spam.

just seems like we are duplicating effort here.

I hope we don't. That's why I report as much as possible in this thread (and put shameless self-advertisements in other captcha issue threads :) ). I just wanted to create a simple but usable (only captcha's for anonymous visitors, simple text challenges, no dependencies, no form_store, no images) captcha module. MyCaptcha looks a bit more ambitious.

I hope this gets cleared out soon.

A) didn't get a captcha added to my "Webform"

Did you enable "Add captcha adminstration links to forms" in the captcha settings?
Then, did a "Captcha administration" link show up on the webform?
Then, did you click on that link? What was the highlighted form_id?
After the administration phase, did you try to visit the webform as anonymous user?
Authenticated users wont see any captchas, only users with administrative priviliges will see an indication that there will be a captcha.

B) have this error on the top of that page now: Notice: Undefined property: theme in /usr/local/www/drupal/includes/theme.inc on line 45

weird, I don't see anything related to captcha on that line.

Also... tried to revert back to my previous version of captcha module and now get a page not found at /admin/settings/captcha

clear your menu cache (e.g. use the devel module http://drupal.org/project/devel)
sometimes visiting the menu administration page also helps: e.g. example.com/?q=admin/build/menu

thanks for testing

hello phillipadsmith (in #28):

Yes to all of the above.

I tried the webform module and apparently there are two ways to enable a captcha:

1. choose a captcha component during the set up of the webform (under "Add a new component"). This indeed does not work, because webform want to use some non existing function (hook_captchachallenge) from captcha API (which is not stable/decided upon yet). This does not work of course.
2. Create your webform ignoring any captcha settings. After your webform is build, go to the new webform node as administrator and click on the "Place a captcha challenge here for anonymous visitors." under " Captcha administration:". Choose a captcha type and submit. This should work (it works for me).

Many thanks for the follow-up

happy to help

Here is a new version (revision 37) of my rewrite. I think it doesn't make sense anymore to provide all the (37) patches (like I did before), so I just attached a tarball as if it would be a normal module.

changes since revision 30:

• removed captcha administration links from administration pages (?q=admin/foo/bar), anonymous users shouldn't get there after all. This removes some unneeded clutter from these administration pages
• added a MENU_DEFAULT_LOCAL_TASK entry in captcha_menu() for being usable when other captcha modules are enabled
• worked on captcha positioning. solved the bug of the captcha ending up between buttons (e.g. comment form with "preview" and "submit" buttons). The code is cleaner too.
• Made spamming more difficult: now also after an unsuccessful validation the captcha solution is unset, to prevent retrying without re-rendering a form first.
• added a hidden captcha_token to the form to make spamming harder
• added source code documentation, solved minor bugs, tweaked some UI strings

bye

For some reason this file doesn't work for me after I patch it. Could you please submit a pre-patched version? Thanks!

it is possible that you need to do an "install" after "uninstall" after "install" of the rewrite if you had the official captcha module already installed.

Here is another version of my rewrite (revision 52)

Main changes:

• I added a new captcha type: text_captcha (separate module) which offers a text based captcha. It presents a phrase of random words and the user is asked to enter the n'th word. For humans this should be simpler than the math captcha, but for spam bots I guess it is harder because they have to parse words like "second", "third", "seventh", which can, moreover, be localized.
• I also ported the image captcha from the MyCaptcha module and implemented is as a separate module image_captcha (included in the tarball). Differences(/advantages) compared with the original MyCaptcha: no form_store dependency, no cache_captcha table needed, better integration with base captcha module.

please try it
and give feedback

bye

Here I am again with a new version (revision 56)

Main difference: added documentation about the API for developers that want to implement their own captcha challenge and integrate nicely with the base captcha module

Again a new version of my rewrite (revision 57)

Main change (beside a UI text typo) is that I worked on the upgrade path for users of the original captcha module. Upgrading (from the original captcha module, not from previous version of my rewrite) is now how it should be: replace old version with new version, run update.php script and done. So, no install-uninstall-install cycle needed, no error messages to ignore, like it was with previous version of my rewrite.

Upgrading from previous versions of my rewrite will possibly generate errors during the update, I guess, but i't safe to ignore them. Uninstall and reinstall if you want to be sure to have a working version.

complete module tarball in attachment (now with gzip compression ;) )

jnm, Farrerres, Skor,
thanks for your feedback

On the captcha settings page, I enabled "Add captcha adminstration links to forms", but I didn't get a link on any forms.

Am I missing something?

I even emptied the cache and reset menus, just to be sure. Still no luck.

Captcha administration links are only visible to users with the "administer site configuration" permission. Moreover you won't see captcha administration links on forms in the administrative section (?q=admin/foo/bar). Could it be related to this?

Do the default installed captcha points work? You don't need the captcha administration links for working with these default captcha points.
Also note that captcha's will only be visible to anonymous visitors. So to test, log out or use another browser/client/computer.

You forgot the install hook in captcha.install :)

Maybe this is related to Heine's #41 comment?

yep, stupid me. should have read the documentation on hook_install better.

revision 58 in attachment should solve this issue

sorry guys

Using one session variable per form_id won't do. This breaks requesting multiple comment submission forms in tabs, then submitting or previewing them.

I know, it's on the todo list (see top of captcha.module file).

Using one session variable per form_id won't do. This breaks requesting multiple comment submission forms in tabs, then submitting or previewing them.

Well, you could store the solution of each requested captcha in the $_SESSION variable (with a different random captcha_token as key), so anonymous users can post multiple instances of the same form at the same time. I'm working on a patch for this.
But the problem is that a malicious user could keep requesting forms (and captcha's) without solving them. Doing so, he can spam his session variable which leads to pollution of the database and maybe unneeded overhead. A solution to this could be to flush the session variable if too many (e.g. 20) captcha's are unsolved.

Using one session variable per form_id won't do. This breaks requesting multiple comment submission forms in tabs, then submitting or previewing them.

Here you are, a new version (revision 59). Anonymous users now can request up to 10 instances of the same form before submitting them. If more than 10 instances are requested, all previous challenges for that form are flushed.

The threshold 10 seems a reasonable default to me. Plenty for normal human users, but small enough to a be some sort of annoyance for spammers. I decided to not make this threshold configurable in the interface, because it doesn't seem worth it. It's a "php define" constant at the top of captcha.module however, so not too hard for power users to tweak it. Any toughts?

Doesn't uninstall correctly....

I'll add this, thanks.

Updating from previous versions of Captcha doesn't work. It's probably safest to uninstall current Captcha, and reinstall this one.

Of what revision of my rewrite are you talking? In my latest revision (59) I implemented the upgrade path from the previous captcha version. Or doesn't that work for you?

Where are the permissions? You should be able to state what roles the captcha is presented to

In my opion, captchas should only be used on anonymous users. There exists much better/more efficient ways to control the behavior of authenticated users: user permissions, roles, tracking, blocking, etc. It doesn't make sense to have an option to offer a captcha to site administrators for example. Also, I think offering a captcha selection for each form and for each role (like the currently official captcha module) adds too much bloat to the captcha configuration page and administrative overhead without sensible value. There are bug reports out there complaining about having to handle thirty-something selection boxes (e.g. six user roles x five forms). That's why I removed the user roles stuff from my implementation.

Thanks for your feedback
Soxofaan

Yet another new release (revision 68). Changes:

• added cleanup stuff of Rob (comment #50, http://drupal.org/node/153395#comment-271596)

to base and sub modules.

• solved issue of David (comment #54, http://drupal.org/node/153395#comment-272096)
• made the text captcha a bit harder for spam bots: now the question can also be like: "what's the third last word of ...."
• Added a "random captcha type" captcha (more a meta captcha type actually), which will randomly select a captcha type out of the available types.
• minor cleanups and fixes

thanks for testing guys.

new release (revision 75). Changes:

• renamed text_captcha to phrase_catcha
• added two new word selection challenges to phrase_captcha: find the word that is alphabetically misplaced, find the word that occurs twice
• made captcha play nice with page caching (see http://groups.drupal.org/node/5064)

As per the discussions with Heine and Wundo, this has been moved to the DRUPAL-5--3 branch...
http://cvs.drupal.org/viewcvs/drupal/contributions/modules/captcha/?only...

It seems that my revision 59 (or maybe older) is in that CVS branch currently, while in robloach's sandbox it is revision 75. Why?

in attachment: revision 77 of my rewrite, which adds the 'skip captcha' permission stuff and related changes in code/documentation/UI. Individual patch against revision 75 is at http://drupal.org/node/158305#comment-272823

thanks for committing it to CVS

We do not completely agree with some feautures that were implemented after r58.
For example the 10 different captchas limit, etc.

There is an discussion in capcha group about that.

I don't find that captcha group discussion, or do you mean the discussion about the role/permission stuff?
Considering the "10 different captchas limit": is it to limited? unneeded? I'd like to have your opinion. I'd also like to know the other 'disagreements'.

Once we test a bit more with what's in there, we'll push a release. Should be within the next few days.

Don't hurry, I'd like to push some features from after r58 (e.g. playing nice with caching, cleanup on uninstall, the 'skip captcha' permission).

cheers

Please submit futher changes as patches and please open new issues. (And always try to keep just one change by issue)

I'd like to push some features from after r58 (e.g. playing nice with caching, cleanup on uninstall, the 'skip captcha' permission).

Here are already some patches (bugs, small features) that shouldn't be a problem: http://drupal.org/node/158734 (about page caching), http://drupal.org/node/158740 (bug in text_captcha settings valdation), http://drupal.org/node/158747 (minor issue with image placement in image_captcha), http://drupal.org/node/158736 (cleanup on uninstall as suggested by Rob Loach), http://drupal.org/node/158305 (basic implementation of 'skip captcha' permission).