This module implements an interface for processing Webform-created forms with the the vetting service E-Hawk. E-Hawk is like an anti-spam service, but more sophisticated; rather than being geared toward spam per se, it's designed to evaluate provided personal information to assess the likelihood of fraud, rather than evaluating the content of a message.

This module provides controls for mapping fields in your Webform form to fields that E-Hawk uses to compute a risk score. It also provides an interface for mapping values returned by E-Hawk into hidden webform fields so E-Hawk's computed results are readily available.

This module attempts to invoke its E-Hawk processing prior to normal webform submission handling, so you should be able to setup rules, actions or whatever else you might normally do to handle submissions based on the values provided by E-Hawk.

Project page: https://www.drupal.org/sandbox/drywall/2421673

Git clone: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/drywall/2421673.git

CommentFileSizeAuthor
#9 ehawk-module_review-2430497-9.patch5.87 KBmarkie
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxdrywall2421673git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

drywall’s picture

I've addressed the issues found by automated review tools that were in need of fixing.

drywall’s picture

Status: Needs work » Needs review
nabil.sadki’s picture

Status: Needs review » Needs work

Automatic review

Some errors are remaining. Please fix them before applying for a new review

Review of the 7.x-1.x branch (commit 1e448a1):

  • PHP Parse error: syntax error, unexpected ')' in ./ehawk.module on line 147
    Errors parsing ./ehawk.module
  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
    FILE: /var/www/drupal-7-pareview/pareview_temp/ehawk.module
    ---------------------------------------------------------------------------
    FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
    ---------------------------------------------------------------------------
     525 | WARNING | [ ] Only string literals should be passed to t() where
         |         |     possible
     804 | ERROR   | [x] Inline control structures are not allowed
    ---------------------------------------------------------------------------
    PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    ---------------------------------------------------------------------------
    
    Time: 353ms; Memory: 15.5Mb
    
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Source: http://pareview.sh/ - PAReview.sh online service

drywall’s picture

Status: Needs work » Needs review

Okay, I've updated the code so that CodeSniffer is happy now.

I have to admit that I don't really understand such deep reliance on automated reviews, though. My old code looked like this:

function _ehawk_encode_proxy() {
  $proxy_encode = 0;

  if (isset($_SERVER['FORWARDED']))                  $proxy_encode = 1;
  elseif (isset($_SERVER['FORWARDED_FOR_IP']))       $proxy_encode = 2;
  elseif (isset($_SERVER['HTTP_CLIENT_IP']))         $proxy_encode = 3;
  elseif (isset($_SERVER['HTTP_FORWARDED_FOR']))     $proxy_encode = 4;
  elseif (isset($_SERVER['HTTP_FORWARDED_FOR_IP']))  $proxy_encode = 5;
  elseif (isset($_SERVER['HTTP_PROXY_CONNECTION']))  $proxy_encode = 6;
  elseif (isset($_SERVER['HTTP_VIA']))               $proxy_encode = 7;
  elseif (isset($_SERVER['HTTP_X_FORWARDED']))       $proxy_encode = 8;
  elseif (isset($_SERVER['HTTP_X_FORWARDED_FOR']))   $proxy_encode = 9;
  elseif (isset($_SERVER['HTTP_MT_PROXY_ID']))       $proxy_encode = 10;
  elseif (isset($_SERVER['HTTP_X_PROXY_ID']))        $proxy_encode = 11;
  elseif (isset($_SERVER['HTTP_Z_FORWARDED_FOR']))   $proxy_encode = 12;
  elseif (isset($_SERVER['HTTP_X_FORWARDED_PROTO'])) $proxy_encode = 13;
  elseif (isset($_SERVER['HTTP_CLIENT_IP']))         $proxy_encode = 14;

  return $proxy_encode;
}

whereas the new, passed-review code looks like this:

function _ehawk_encode_proxy() {
  $proxy_encode = 0;

  if (isset($_SERVER['FORWARDED'])) {
    $proxy_encode = 1;
  }
  elseif (isset($_SERVER['FORWARDED_FOR_IP']))       $proxy_encode = 2;
  elseif (isset($_SERVER['HTTP_CLIENT_IP']))         $proxy_encode = 3;
  elseif (isset($_SERVER['HTTP_FORWARDED_FOR']))     $proxy_encode = 4;
  elseif (isset($_SERVER['HTTP_FORWARDED_FOR_IP']))  $proxy_encode = 5;
  elseif (isset($_SERVER['HTTP_PROXY_CONNECTION']))  $proxy_encode = 6;
  elseif (isset($_SERVER['HTTP_VIA']))               $proxy_encode = 7;
  elseif (isset($_SERVER['HTTP_X_FORWARDED']))       $proxy_encode = 8;
  elseif (isset($_SERVER['HTTP_X_FORWARDED_FOR']))   $proxy_encode = 9;
  elseif (isset($_SERVER['HTTP_MT_PROXY_ID']))       $proxy_encode = 10;
  elseif (isset($_SERVER['HTTP_X_PROXY_ID']))        $proxy_encode = 11;
  elseif (isset($_SERVER['HTTP_Z_FORWARDED_FOR']))   $proxy_encode = 12;
  elseif (isset($_SERVER['HTTP_X_FORWARDED_PROTO'])) $proxy_encode = 13;
  elseif (isset($_SERVER['HTTP_CLIENT_IP']))         $proxy_encode = 14;

  return $proxy_encode;
}

And that's considered better? I'd love to understand why...

markie’s picture

Status: Needs review » Needs work

Odd that the PHPCS didn't pick up the elseif's as well. They should be on a separate line as well. Doesn't make it "better" per say, but does make it to standards. Personally I dislike a bunch of elseifs just to set a single variable that gets returned and would (again personally, not a standard or even something you have to do) rewrote the function as:

/**
 * Encode the client's perceiveable proxy settings for E-Hawk consumption.
 *
 * @return int
 *   Proxy encodeing value.
 */
function _ehawk_encode_proxy() {
  if (isset($_SERVER['FORWARDED'])) {
    return 1;
  }
  if (isset($_SERVER['FORWARDED_FOR_IP'])) {
    return 2;
  }
  if (isset($_SERVER['HTTP_CLIENT_IP'])) {
    return 3;
  }
  if (isset($_SERVER['HTTP_FORWARDED_FOR'])) {
    return 4;
  }
  if (isset($_SERVER['HTTP_FORWARDED_FOR_IP'])) {
    return 5;
  }
  if (isset($_SERVER['HTTP_PROXY_CONNECTION'])) {
    return 6;
  }
  if (isset($_SERVER['HTTP_VIA'])) {
    return 7;
  }
  if (isset($_SERVER['HTTP_X_FORWARDED'])) {
    return 8;
  }
  if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
    return 9;
  }
  if (isset($_SERVER['HTTP_MT_PROXY_ID'])) {
    return 10;
  }
  if (isset($_SERVER['HTTP_X_PROXY_ID'])) {
    return 11;
  }
  if (isset($_SERVER['HTTP_Z_FORWARDED_FOR'])) {
    return 12;
  }
  if (isset($_SERVER['HTTP_X_FORWARDED_PROTO'])) {
    return 13;
  }
  if (isset($_SERVER['HTTP_CLIENT_IP'])) {
    return 14;
  }
  return 0;
}

Speaking of which, your argument and return parameters are missing in a good portion of your functions.

/**
 * Helper function for referencing various E-Hawk fields.
 *
 * @param string $type
 *   The type of field. Defaults to 'collection'.
 *
 * @return array
 *   An array of data that you do stuffs with.
 */
function _ehawk_fields($type = 'collection') {

Finally you will probably want to sanitize any users input with check_plain or filter_xss depending.

function _ehawk_config() {
  $form = array();
  $api_key = variable_get('ehawk_api_key', '');
  $description = t('API key is provided by E-Hawk. Need one? <a href="@url" target="_blank">Get a free E-Hawk API key</a>.',
    array('@url' => 'http://www.e-hawk.net/signup/'));

  $form['ehawk_api_key'] = array(
    '#type' => 'textfield',
    '#title' => t('E-Hawk API Key'),
    '#description' => $description,
    '#required' => TRUE,
    '#default_value' => check_plain($api_key),
  );
drywall’s picture

Thanks for your help.

I've now pushed a new version that has extensive docblocks and reformatted the conditional that we'd been discussing.

Now CodeSniffer is complaining about "Missing parameter types" in my docblocks. Which I'm a little confused by, as Drupal standards don't generally seem to include them. I've spot-checked source in Core as well as a few popular modules (e.g. ctools), and the actual variable types never seem to be spelled out in the @param declarations. False positive from CodeSniffer, maybe?

drywall’s picture

Status: Needs work » Needs review
markie’s picture

Status: Needs review » Needs work
FileSize
5.87 KB

I made a few more alterations off of the latest dev build

1. Parameter types need to be defined, but not if you are using "Implements hook_xxx()". I think it's a weird standard too, but since hook_xx isn't an object, those parameters don't get picked up by PHPCS,

2) Changed TO DO to standardized "@todo" which is picked up by most IDE's and easier to read.

c: Added unset variable '$talon_fingerprint' which would pass NULL if missing. I assume the API is expecting an empty string.

IV - Updated db calls to use the database API instead of throwing things into a db_select() which sanitizes for you. I would test this to make sure I didn't do something silly.

I also ran these changes through the coder module and received no errors. I can't speak to "why doesn't XXXX module or core adhere to Coding standard XXX?", but it's something we all should be doing.

drywall’s picture

Status: Needs work » Needs review

Thanks, Markie!

I've reviewed, applied and tested your patch, and didn't see any problems. Now pushed and ready for further review.

markie’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed code and tested functionality.

DamienMcKenna’s picture

One small thing - the project's short name and module name really should match, so please either rename the module to e_hawk_integration or rename the project to "ehawk".

drywall’s picture

Done. Project name is now EHawk.

drywall’s picture

So..... now what?

drywall’s picture

Okay, I'm at a bit of a loss at this point as to how to get this project moved from Sandbox to blessed project status. It's been 5 months since I first submitted it. It's gone through community review, and changes have been made — almost all at the code-styling level — and approved. Yet despite community approval it's been 3 months now and nothing's happened.

Am I doing something wrong?

cweagans’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your contribution!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.

drywall’s picture

Thanks, @cweagans!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.