GHOP #52: text captcha port to 6

I don't currently have a chance to give a proper review, but one thing I note right away is that hook_menu and hook_init are not the same thing. Check the hook_menus on some of the core modules to get an idea of how the code in here should change.

Also, the patch doesn't apply for me. How are you generating it? http://drupal.org/patch/create has some good instructions, including a video showing how this is done. But if you need further help, please ping back here or come find us in #drupal-ghop on irc.freenode.net. :)

The warning you're getting might have something to do with the base CAPTCHA module. The module looks pretty good! Now all we need is that patch! If you're using WinMerge, I assume you're on a Windows machine. I strongly suggest you try out TortoiseCVS as it provides a very clean way to check out sources, use your favourite programs to make changes, and then create a patch of all the changes. Good luck! Great work!

Test the reCAPTCHA module and see if you get the same warning. I've seen similar issues pop up with it. If you follow the TortoiseCVS instructions, you should be fine.

Very nice work! It's getting there..... Couple things:

• 'callback arguments' should be 'page arguments'.... This will help ;-) .
• return $output; causes a warning: notice: Undefined variable: output in captcha\text_captcha\text_captcha.module on line 21..... Removing that line should fix it!
• It's best to create that patch from the CAPTCHA base module directory. This means that when we apply the patch, we can just stick the patch in the module's directory, and then apply it without having to realize that we have to move it to text_captcha folder.
• Awesome work! Keep it up.

One more...

+
+; Information added by drupal.org packaging script on 2007-12-03
+version = "HEAD"
+project = "captcha"
+datestamp = "1196640110"
+

That stuff should be removed from the .info file. The packaging script (the thing that bundles up the module .tar.gz files) adds that automatically.

binodc, sorry this is taking so long for us to review. :-(

Just two really small things:
1) The package info added by the packaging script in the .info file needs to be removed:

+
+; Information added by drupal.org packaging script on 2007-12-03
+version = "HEAD"
+project = "captcha"
+datestamp = "1196640110"
+

2) The array items in the hook_menu need to be outdented (as in, they are currently too far indented.) The comment and 'title' => 'Text CAPTCHA', and friends should all be at a two space indent under $items.

I ran it through functionality and everything worked great! Just fix those two things and load another patch and it is RTBC.

The validation of the settings form still has some issues:
the validate function is not fired (I can submit out of range data), see http://drupal.org/node/144132#base
the siganture of the validate function also isn't right: it should be foo_validate($form, &$form_state), see http://drupal.org/node/144132#process-params

If the admin enables user defined words, he should enter at least 20 words in the text area. In your current version (patch from #13) it is however possible to enter less words without error message.
It is also possible to set "Number of words in phrase" outside the range of possible values (4 to 10) without error message.

You're so close.....

1. You're missing $form['#validate'][] ='text_captcha_settings_form_validate'; in function text_captcha_settings_form (I miss this all the time)
2. $form_values = $form_state['values']; might help you in text_captcha_settings_form_validate, as you're using the $form_values function a lot.
3. You don't have to check the $form_id in the validate, as you already know that it's being called from text_captcha_settings_form.

If you fix those three things doing some debugging with Devel's dvm function, you should be good! Keep it up! You're so close.

Another thing to watch out for is the Windows line breaks. Drupal CVS standard uses UNIX line breaks. You can fix those using a good text editing program like PSPad.

I'm sorry, but this has been ActionNeeded for 6 days now and we have not heard from the student during that time. I know you're really close to finishing this up, so if you are still interested please re-claim this task (if it's still open) so you can finish it up and get credit for the work you've done so far.

I just claimed this task on the Google issue tracker. I'm attaching my patch, which is based off of binodc's patch and which hopefully addresses the issues raised here. However, as people have pointed out, binodc's patch was just about complete before the task was reopened. This leads to the question of whether this task is still suitable for earning credit (since my work involved addressing the few issues raised here and then indenting some code properly based on my changes).
In any case, here is my patch.

The patch from #19 looks ok to me.
One minor thing is to put an extra space after the '=' in

$form['#validate'][] ='text_captcha_settings_form_validate';

but that's not a show stopper.

If you want to do more work on it, you could separate off the administration page functions to a separate include file 'text_captcha.admin.inc' as explained in http://drupal.org/node/146172 .

Additionally you could also separate off the CAPTCHA generating functions (_text_captcha_generate_nonsense_word(), _text_captcha_generate_words(), _text_captcha_ordinal()) to for example 'text_captcha.user.inc', and only load (require_once) this include file in the 'generate' part of text_captcha_captcha().

This will make your work more credit-worthy ;)

PS: look under "Adding/Deleting files" of http://drupal.org/patch/create for more information on how to create a patch that adds files

Alright, fair enough :)
Here's a patch with the changes, I hope I did them correctly. :P

That was fast!
patch looks very good
I did some final tweaks (added "$Id$" CVS keywords to new files)
and committed it: http://drupal.org/cvs?commit=94082
thanks a lot