Closed (fixed)
Project:
Mollom
Version:
6.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
3 May 2010 at 22:53 UTC
Updated:
24 Apr 2014 at 17:13 UTC
Jump to comment: Most recent, Most recent file
There's a fair amount of duplication in mollom.js, I wanted to see if I could simplify things down. Patch to follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 788968-mollom-simplify-js-captcha-D6.patch | 11.42 KB | dave reid |
| #2 | 788968-mollom-simplify-js-captcha-D7.patch | 13.91 KB | dave reid |
Comments
Comment #1
dave reidSo we're also duplicating fetching captchas three different times too. That can and should be reduced down to one function.
Comment #2
dave reidComment #3
dave reidSummary of the patch:
1. Combines the JS function getAudioCaptcha() and getImageCaptcha() into getMollomCaptcha() which can automatically determine which CSS class is on the link (that has CSS class .mollom-switch-captcha). This should probably actually be abstracted out into a mollomSwitchCaptcha() and mollomGetCaptcha() functions.
2. Simplifies the PHP code for fetching a CAPTCHA into one function. Currently it was being done in three different places, so if you wanted to change the help text, you had to change it three times. Its now available as mollom_get_captcha($type, array $data = array()).
Comment #4
dave reidWe should really consider using the AHAH system in D7 for the captcha replacement as we really should provide different help text for each captcha type as well. That can probably be a follow-up issue.
Comment #5
dries commentedThis looks like a reasonable clean-up to me.
Comment #6
dave reidDries, did you set this to active because of #4? I'd love that to be a separate follow-up issue since I haven't actually played too much with AHAH.
Comment #7
dries commentedI incorrectly changed the status. Back to 'needs review'.
Comment #8
dries commentedReviewed and looks good. Committed to CVS HEAD.
Should we backport this to D6?
Comment #9
dave reidYep I can work on re-rolling for D6.
Comment #10
dave reidPatch re-rolled for D6, working on testing manually to confirm the JS changes.
Comment #11
dave reidManual testing worked great and passes locally for me.
Comment #12
dave reidCommitted to DRUPAL-6--1 as well.
http://drupal.org/cvs?commit=363692
Comment #13
sunTo be able to use the AJAX framework for this in D7, #647228: Links are needlessly unable to fully participate in D7 AJAX framework features needs to be fixed.