Enmask Captcha

http://git.drupal.org/sandbox/enmasksteve/1275582.git enmask_captcha

could not be downloaded.
The requested URL /sandbox/enmasksteve/1275582.git was not found on this server.

sorry my bad.. its not a direct dowload link... we will have to use ( as suggested by zocee in irc) git to download it.

git clone http://git.drupal.org/sandbox/enmasksteve/1275582.git enmask_captcha

Needs work to comply with coding standards. See http://drupal.org/coding-standards
Please install the Coder module and run it on your contribution. Coder does not catch everything mentioned in the URL above, but is a good start.

In particular, indents should be two spaces for each level and there should be no tabs at all. Also, function declarations like

function enmask_permission()
{

should be

function enmask_permission() {

license.txt does not belong in your repository.

In your .info file, you only need to declare files[] = for files that contain class definitions. In this case, I believe that only one line is needed: files[] = enmask.captcha.php

when i click on submit.. i get this error on a white screen.

Fatal error: Call to undefined function curl_init() in D:\server\xampp\htdocs\acc\D7\sites\all\modules\modules\custom\enmask_captcha\enmask.captcha.php on line 40

If you require curl then you should implement hook_requirements() to check for curl, so that your module can't be installed without it. That way you won't encounter problems like #5. Put this in a file called enmask.install:

/**
 * Implements hook_requirements().
 */
function enmask_requirements($phase) {
  $t = get_t();

  $has_curl = function_exists('curl_init');
  $requirements['enmask_curl'] = array(
    'title' => $t('cURL'),
    'value' => $has_curl ? $t('Enabled') : $t('Not found'),
  );
  if (!$has_curl) {
    $requirements['enmask_curl']['severity'] = REQUIREMENT_ERROR;
    $requirements['enmask_curl']['description'] = $t("Enmask requires the PHP <a href='!curl_url'>cURL</a> library.", array('!curl_url' => 'http://php.net/manual/en/curl.setup.php'));
  }

  return $requirements;
}

Also in enmask.install, you should implement hook_uninstall() to remove your module's system table variables when your module is uninstalled.

The Captcha text displayed should (preferably) always fit inside the display field when at no zoom.

Optionally i feel the width of the captcha field should be little more.

This would be a nice UX improvement.

I am looking forward to use this module once its released :)

I'm curious how this works for users who have low vision, or who have no vision and use a screen-reader?

still tons of coder violations. Also, I do not readily understand why there is no integration with the captcha module, instead of re-implements form alter, admin etc in yet another way. I know captcha is alpha but it wont stay alpha forever but you on the other hand would need to carry this burden forever.

* Why don't you integrate your module with the existing CAPTCHA module?
* info file contains line ending errors ('\r'), they should be all unix style '\n', see http://drupal.org/node/318#indenting
* Rename "readme.txt" to "README.txt"
* "Captcha you entered was not valid." always wrap text in t() for translation.
* wrong indentation in module file enmask_form_alter(), use 2 spaces for each level. Also in other places.

I'm still seeing line ending problems in README.txt and enmask.info, both linked to the drupalcode.org view where you can see the bad \r characters.

You don't seem to be doing anything with curl you can't do with drupal_http_request, so you should switch that implementation and remove the unnecessary dependency.

$this->protocol appears to be set up to switch between HTTP and HTTPS, but it's hard-coded everywhere to HTTP. That should change, so this won't throw errors on secure sites.

urlencode('response') This isn't actually doing anything, since the text is already URL-encoded.

$wp_lang = 'en-US';
if ($wp_lang != '') {

There's no point in checking the value of a variable you just gave a specific value in the previous line.

Also, this looks like a WordPress variable. If you're trying to make this a generic file you can use in any system, should ignore what I said about using drupal_http_request(), but also remove references to WordPress.

Indentation is still bad. For example:

  if (trim(check_plain($_POST['myCaptcha']))!='') {
  //start for validating captcha
  require_once('enmask.captcha.php');
  $enmask_captcha = new EnmaskCaptcha();

Everything within the if () { block needs further indentation.

  );
    return $items;
} 

Why is return indented so far here?

I'm not trying to nitpick here; it's important the code on Drupal.org follow standards so everyone can contribute improvements.

Finally, I think you're making a mistake in not integrating with the Captcha module, which has thousands of users already in Drupal 7, despite the alpha label. That said, it's your mistake to make.

I believe this access argument will make it so only uid 1 can administer the module. I suggest updating it to match your hook_permissions which uses "administer Enmask Captcha" as the key for the permission.

70 'access arguments' => array('Activation options Enmask Captcha'),

Looks RTBC to me.

• It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
• lines in README.txt should not exceed 80 characters
• enmask_settings(): indentation errors, always use 2 spaces per level. Did coder not detect this?
• "//only valid for drupal": all comments should start capitalized and end with a ".". And there should be a space after "//".
• "$result=json_decode($buffer->data);": there should be a space before and after "="
• "//return message will be from the enmask server.": comments should be on a separate line.
• "form_set_error('', check_plain( t($message)));": there should be no space after "("
• module file: still indentation errors in many functions, enmask_admin_submit(), enmask_help(), enmask_permission().
• "require_once('enmask.captcha.php');" remove this line, your class has been registered in the info file, so the autoloader will take care of it.
• "$form['enmask_captcha'] = array('#type' => 'item',": the first array element should also be on a new line.
• "if (trim(check_plain($_POST['myCaptcha']))!='') {": there should be a space before and after "!="

I'm sorry, but there are still too many coding standards violations.

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

This line in enmask.admin.inc t('EnMask is founded with the ideas of helping people control their online content, privacy and freedom. For the detail information please') . '<a href="http://www.enmask.com" title="enmask.com" target="_blank">' . t('click here') . '</a>' should be written to use placeholders. See http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/t/7. You should also make use of the l() function for the link.

Why do you make it so your captcha can only be used on 6 specific pages? Why don't you allow the admin to specify *any* form?

enmask_submit() should be in enmask.admin.inc

The constructor for class EnmaskCaptcha should be called "__construct()", not "EnmaskCaptcha()" (you're using the old, semi-deprecated PHP 4 convention). In fact, since you don't have any constructor arguments and your instance variables are private and you don't supply accessor or mutator functions for those variables, there's no need at all to even have a constructor. Simply initialize the variables when they're declared, e.g. private protocol = "http://";

One of the methods in that class does not have a function documentation comment, and the other has an improper comment - the comment must start with /** to be recognized as a Doxygen comment.

updated git url (with new branch name)

Regarding the '6 specific pages', why only these pages, we not let the user decide on what pages to use this? And if you go that route, you're back a why not integrating with captcha. I know they are still alpha/dev but it's working.

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/enmask.captcha.php:
   +15: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 4 files, 1 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• enmask_form_validate(): why are you using check_plain() for thw $_POST variables here? You are not outputting them to the user, so no need to do so.
• enmask_admin_submit(): why do you need that function? system_settings_form() will save the form values automatically, just use "enmask_form_ids" as $form key and it should work.
• enmask_form_alter(): you don't use the $path variable, so remove it.

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/enmask.module:
   +66: [minor] There should be no trailing spaces
   +73: [minor] There should be no trailing spaces
   +81: [minor] There should be no trailing spaces
  
  sites/all/modules/pareview_temp/test_candidate/enmask.captcha.php:
   +15: [minor] There should be no trailing spaces
   +46: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 4 files, 5 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• enmask.module in enmask.info: It's only necessary to declare files[] if they declare a class or interface.
• enmask.install in enmask.info: It's only necessary to declare files[] if they declare a class or interface.
• ./enmask.install: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function hook_uninstall() {
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so I can get back to yours sooner.

manual review:

• "'access arguments' => array('Administer Enmask Captcha'),": permission should start lower cased.
• "trim(filter_xss($_POST['mycaptcha' . $captcha_sid]))": why do you use filter_xss() here? you are not outputting anything to the user, so you do not need the sanitization. Also elsewhere.
• "if ($isValid) {": you could just use "return (bool) $isValid;" here.
• enmask_captcha(): the function is more readable if you define the parameters in the function signature.
• "$mycaptcha = $enmaskCaptcha->getHtml('mycaptcha' . $captcha_sid);": why the name "mycaptcha"? "enmask" or something would be more clear.

Review of the 7.x-1.x branch:

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./README.txt ./enmask.info ./enmask.captcha.php ./enmask.admin.inc ./enmask.install ./enmask.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• "'access arguments' => array('administer enmask captcha'),": does not match the permission that you define,
• "EnMask Captcha is a free cloud base service (Service as a Service, SaaS)": whut?
• enmask_settings_form(): missing doc block
• enmask_settings_form(): what is this function for? does the pseudo-embedded javascript even work? I don't think you need such a testing page, so remove it + the menu hook + the permission.
• Please try to deliver a module that is nearly ready for relase and does not contain half-baked functionality.

Hi,

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/enmask.module:
   +61: [minor] Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), !f
  ilter_xss() or similar.
   +65: [minor] Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), !f
  ilter_xss() or similar.
   +66: [minor] Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), !f
  ilter_xss() or similar.
  
  Status Messages:
   Coder found 1 projects, 1 files, 3 minor warnings, 0 warnings were flagged to be ignored
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./enmask.install ./enmask.captcha.php ./README.txt ./enmask.module ./enmask.info
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:
For the $_POST issue I find no dangerous use (displaying, function injection) but perhaps either someone might take an additional look at that.

OK, looks fine for me. Note to git approvers: $_POST issue does not appear to be an issue here, but please judge again. Thx.

folder name changed

Thanks for your contribution, Steve! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Please also review these additional notes.

.install file

Seems to be empty. Remove it unless you're going to add something useful to it, like a message telling the admin that the module has been installed.

release tags

You will need to tag one of your branches to be able to make a release. Please review: Release naming conventions

project page

Your project page should make it clear that this module requires the CAPTCHA module to function, and some links to the other CAPTCHA modules that are available. You might also wish to contact the other maintainers of CAPTCHA modules to get them to include this new module on their project pages too to allow people a greater choice at their finger tips. Please take a moment to make your project page follow tips for a great project page.