cloudgears [cloudgears]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

We just review a module / theme per applicant; let us know which one you want reviewed.
As there is already a project to implement CAPTCHAs, why didn't you open a feature request for the existing project?

To clarify, the original code was also GPL, so that's cleared.

Utility : Demo provided (THANKS!) works well. It's not often I see a Captcha that counts as fun, or even interesting.
Non-js is an issue of course, but we'll leave that to the developers to decide. A warning about this on the proj page would be appropriate.

Documentation: Excellent. The readme even describes where the config page is!

Cross-over: This correctly extends, not duplicates anything in the base CAPTCHA module. captcha.module provides hooks for exactly this sort of add-on.

Security: No visible entry point for exploits. It does a lot of 'crypto' that I won;t evaluate, but all it's doing is hashing a captcha, no it's not exposing anything. (Could be overkill to require MCrypt, I've not seen that in standard PHP distros)
No code execution or XSS vulnerabilities. No access to Drupal content or profile data.

Code: Very tidy. Looks to conform to Drupal standards, well documented.

Not sure about the second half where it starts writing chunks of javascript that writes HTML directly to the screen. That's a bit freaky. if you were to continue developing like that, I'd suggest looking at the Drupal functions for javascript variable management drupal_add_js($data, 'setting') And better use of the jquery lib instead of all that document.write.
but there's nothing wrong with what you've got - that works. And is (compared to what it could be) entirely maintainable.

I can see WHY you found the need to inline your CSS into the module code, but would suggest 95% of it goes in its own notcapcha.css file and only the dynamic extras get written inline. This will affect developers, themers and page rendering later. That one file now contains 4 different languages inline!

The trackbar.js file would probably benefit by being written with jquery instead of the long way like it is, but again, that's a possible revision, not a problem.

I've not tried the actual code - this is visual review, But this gets an A, if not an A+ from me.

No problems here, looks like cloudgears will be a nice addition to the developer community.
.dan.

I am changing the status as per previous comment of dman. Drupal modules needs to be written using the function made available from Drupal, or following its coding standards; JavaScript code for Drupal should be using jQuery.

Drupal code must follow Drupal coding standards, and use Drupal functions; that is not optional.

From the current state of things, it's clear that the javascript in question was written for and inherited from another project. This is a port only. It works quite well as it is, and requiring cloudgears to write a new version from scratch instead of using the current routines just to be more jquery-like is unnecessary.

There are a bunch of other projects that may include some js or ported js that's not strictly jquery, many of the WYSIWYGs, Google Analytics, Various widgets ...
JQuery is a good stylistic choice when building from scratch for a 100% Drupal new project, but requiring working code to be completely refactored when doing a port like this is - I think - not something that should prevent an otherwise really nice contribution.

I think this application is good as-is.

1. The license file should be removed as it cannot be committed in Drupal.org repository.
2. Files available from third-party sites should not be committed in Drupal.org repository.
3. Menu titles, and descriptions should not be passed to t() as that is already done by Drupal core code.

4.   $fonts = array();
     $fontsdir_absolute = drupal_get_path('module', 'notcaptcha_captcha') . '/gallery';
     $handle = opendir($fontsdir_absolute);
     if ($handle) {
       while (false !== ($file = readdir($handle))) {
         if (preg_match('/\.png$/i', $file)) {
           $fonts[]=$fontsdir_absolute.'/'.$file;
         }
       }
       closedir($handle);
     }
   

   The code is duplicating the code of a Drupal function that should be used instead.

5.   if (function_exists("imagejpeg")) {
       header("Content-Type: image/jpeg");
       imagejpeg($img_res, null, $notcaptcha['notcaptcha_jpeg_quality']);
     } else if (function_exists("imagegif")) {
       header("Content-Type: image/gif");
       imagegif($img_res);
     } else if (function_exists("imagepng")) {
       header("Content-Type: image/x-png");
       imagepng($img_res);
     }
     
     exit;
   }
   

   The code is not correctly formatted as per Drupal coding standards (that is a minor issue); the code should then allow to the other modules to correctly exit before to invoke exit. The correct function to call is drupal_header().

6.   $output = <<<STYLE
   .imgunit {
     width: {$notcaptcha['notcaptcha_imagesize']}px;
     height: {$notcaptcha['notcaptcha_imagesize']}px;
     overflow: hidden;
     margin-left: {$img_margin_left}px;
     position: relative; /* IE fix */
   }
   .imgunit img {
     padding: 0;
     margin: 0;
     position: relative;
   }
   .captchablock {
     width: {$captcha_block_width}px;
     float: left;
     padding: 2px;
   }
   table.trackbar, table.trackbar div, table.trackbar td {
     margin: 0;
     padding: 0;
   }
   table.trackbar {
     border-collapse: collapse;
     border-spacing: 0;
     border: 0;
   }
   table.trackbar tbody {
     border: 0;
   }
   table.trackbar img{
     border: 0;
   }
   table.trackbar {
     width: {$track_bar_width}px;
     background: repeat-x url(/$module_path/imgtrackbar/b_bg_on.gif) top left;
   }
   table.trackbar .l {
     width: 1%; 
     text-align: right; 
     font-size: 1px; 
     background: repeat-x url(/$module_path/imgtrackbar/b_bg_off.gif) top left;
   }
   table.trackbar .l div {
     position: relative;
     width: 0;
     text-align: right;
     z-index: 500;
     white-space: nowrap;
   }
   table.trackbar .l div img {
     cursor: pointer;
   }
   table.trackbar .l div span {
     position: absolute;
     top: -12px; 
     right: 6px; 
     z-index: 1000; 
     font: 11px tahoma; 
     color: #000;
   }
   table.trackbar .l div span.limit {
     text-align: left; 
     position: absolute;
     top: -12px; 
     right: 100%; 
     z-index: 100; 
     font: 11px tahoma; 
     color: #D0D0D0;
   }
   table.trackbar .r {
     position: relative; 
     width: 1%; 
     text-align: left; 
     font-size: 1px; 
     background: repeat-x url(/$module_path/imgtrackbar/b_bg_off.gif) top right; 
     cursor: default;
   }
   table.trackbar .r div {
     position: relative; 
     width: 0; 
     text-align: left; 
     z-index: 500; 
     white-space: nowrap;
   }
   table.trackbar .r div img {
     cursor: pointer;
   }
   table.trackbar .r div span {
     position: absolute;
     top: -12px; 
     left: 6px; 
     z-index: 1000; 
     font: 11px tahoma; 
     color: #000;
   }
   table.trackbar .r div span.limit {
     position: absolute;
     top: -12px; 
     left: 100%; 
     z-index: 100; 
     font: 11px tahoma; 
     color: #D0D0D0;
   }
   table.trackbar .c {
     font-size: 1px; 
     width: 100%;
   }
   STYLE;
   

   Why aren't the CSS styles loaded from an external file?

7.         $angles1 = urlencode($notcaptcha['angles1']);
           $angles2 = urlencode($notcaptcha['angles2']);
           $angles3 = urlencode($notcaptcha['angles3']);
   

   The correct function to call is drupal_urlencode().

8.         $images_header = t('Place these icons') . ' <strong>' . t('vertically') . '</strong>';
           $images_footer = t('Move the sliders to change angle of the images');
           $images_copyright = t('Human test by') . ' <a href="http://cloudgears.com/notcaptcha-for-drupal">' . t('NotCaptcha for Drupal') . '</a>';
   

   It is better to avoid to concatenate strings that are translated, when possible. Avoiding to concatenate such strings gives more context to who translate the strings, and reduce the possibility of giving a not correct translation.

9.         $result['form']['captcha_body'] = array(
              '#type' => 'markup'
             ,'#value' => 
   <<<HTML
   <style type="text/css">
   {$style}
   </style>
   <noscript><div style="clear:both">{$noscript}</div></noscript>
   <script type="text/javascript">
   
     var im1val{$uniqid} = 0;
     var im2val{$uniqid} = 0;
     var im3val{$uniqid} = 0;
     
     function setCaptchaValue{$uniqid}(id, val) {
       var imgLeft = -val/10*{$notcaptcha['notcaptcha_imagesize']} - (val/10);
       document.getElementById(id+"{$uniqid}Pict").style.left = imgLeft.toString() + "px";
       document.getElementById("notcaptcha-result{$uniqid}").value = md5(im1val{$uniqid}.toString() + im2val{$uniqid}.toString() + im3val{$uniqid}.toString());
     }
   
     document.write('<div style="clear:both"><small>{$images_header}</small></div>');
     document.write('<div style="clear:both">');
     
     document.write('<div class="captchablock">');
     document.write('<div id="imgone{$uniqid}Unit" class="imgunit"><img id="imgone{$uniqid}Pict" src="/captcha/notcaptcha/{$angles1}"></div>');
     trackbar.getObject('imgone{$uniqid}').init({
       onMove : function() {
         im1val{$uniqid} = this.leftValue / 10;
         setCaptchaValue{$uniqid}('imgone', this.leftValue);
       },
       dual : false, // two intervals
       width : {$trackbar_size}, // px
       roundUp: 10,
       leftLimit : 0, // unit of value
       leftValue : 0, // unit of value
       rightLimit : {$trackbar_size}, // unit of value
       rightValue : {$trackbar_size}, // unit of value
       clearLimits: 1,
       clearValues: 1,
       imagesPath: "/{$module_path}/imgtrackbar"
     });
     document.write('</div>');
   
     document.write('<div class="captchablock">');
     document.write('<div id="imgtwo{$uniqid}Unit" class="imgunit"><img id="imgtwo{$uniqid}Pict" src="/captcha/notcaptcha/{$angles2}"></div>');
     trackbar.getObject('imgtwo{$uniqid}').init({
       onMove : function() {
         im2val{$uniqid} = this.leftValue / 10;
         setCaptchaValue{$uniqid}('imgtwo', this.leftValue);
       },
       dual : false, // two intervals
       width : {$trackbar_size}, // px
       roundUp: 10,
       leftLimit : 0, // unit of value
       leftValue : 0, // unit of value
       rightLimit : {$trackbar_size}, // unit of value
       rightValue : {$trackbar_size}, // unit of value
       clearLimits: 1,
       clearValues: 1,
       imagesPath: "/{$module_path}/imgtrackbar"
     });
     document.write('</div>');
   
     document.write('<div class="captchablock">');
     document.write('<div id="imgthree{$uniqid}Unit" class="imgunit"><img id="imgthree{$uniqid}Pict" src="/captcha/notcaptcha/{$angles3}"></div>');
     trackbar.getObject('imgthree{$uniqid}').init({
       onMove : function() {
         im3val{$uniqid} = this.leftValue / 10;
         setCaptchaValue{$uniqid}('imgthree', this.leftValue);
       },
       dual : false, // two intervals
       width : {$trackbar_size}, // px
       roundUp: 10,
       leftLimit : 0, // unit of value
       leftValue : 0, // unit of value
       rightLimit : {$trackbar_size}, // unit of value
       rightValue : {$trackbar_size}, // unit of value
       clearLimits: 1,
       clearValues: 1,
       imagesPath: "/{$module_path}/imgtrackbar"
     });
     document.write('</div>');
     
     document.write('</div>');
     document.write('<div style="clear:both"><small>{$images_footer}</small></div>');
   </script>
   <div><small>{$images_copyright}</small></div>
   HTML
   

   It would be better to load the JavaScript code from an external file.

10. The file .info contains lines that are already added from the packaging script; duplicating those lines confuse the Update manager, which could report the not correct version of the module being installed in a Drupal site.

Changing the code to use jQuery is not so much difficult; http://drupal.org/node/539608 reports that

Your module pulls in a lot of JavaScript code in-line without a hint of jQuery involvement. Try to use jQuery to play nice with all the other jQuery code running on end users sites.

If it is necessary to make the changes you're asking for and it's impossible to place the module on drupal.org without re-writing the code, such changes will be made in the future, but not now as we have more projects to do.

What you say sounds like you would not have time for either fix the bug reported from the users who is using your module. If that is the case, why don't you apply for a CVS account when you have time to dedicate to the projects you want to host on Drupal.org?

There have not been replies in more than a week.

Please read the following links as this is very important information about CVS applications.

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for these applications. Please read Migrating from CVS Applications to (Git) Full Project Applications and Applying for permission to opt into security advisory coverage on how this affects and benefits you and the application process. In short, every user has now the permissions necessary to create new projects, but they need to apply for opt into security advisory coverage. Without applying, the projects will have a warning on projects that says:

This project is not covered by Drupal’s security advisory policy.