[D7] Aspam

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

This issue is not fixed? See the workflow: https://www.drupal.org/node/532400

Git clone command and project page still missing in the issue summary, please add that.

Please don't set the status to fixed, this needs review.

Automated Review

Found various issues in pareview.sh. Follow this link : http://pareview.sh/pareview/httpgitdrupalorgsandboxmimedia2686391git

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[No: Does not follow] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements]

Coding style & Drupal API usage

[Various issues found:

1. In aspam.module file:
   

   Line 12: curly braces { should end a line, not start one
   {
   
   Line 15: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
         return '<p>'.t('Yet another spam protection').'</p>';
   
   Line 23: curly braces { should end a line, not start one
   {
   
   Line 42: curly braces { should end a line, not start one
   {
   
   Line 55: curly braces { should end a line, not start one
   {
   
   Line 88: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
             'encryption' => variable_get('aspam_rsa_encryption', false),
   
   Line 93: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
           static $_aspam_jsadded = false;
   
   Line 94: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
             if ($_aspam_jsadded == false) {
   
   Line 95: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
                 $_aspam_jsadded = true;
   
   Line 103: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
   
   Line 112: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
               '#required' => true,
   
   Line 133: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
                 jQuery.spam.aspam( '.drupal_json_encode($aspam_options).', "#aspam" );
   
   Line 139: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
               '#markup' => '<div id="aspam_container" style="overflow: hidden; position: relative;">'.$aspam_custom_html.'</div>',
   
   Line 148: curly braces { should end a line, not start one
   {
   
   Line 152: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
     if (variable_get('aspam_rsa_encryption', false)) {
   
   Line 153: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
         $page['content']['#attached']['js'][drupal_get_path('module', 'aspam').'/lib/jsencrypt/jsencrypt-all.js'] = array(
   
   Line 158: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
     $page['content']['#attached']['js'][drupal_get_path('module', 'aspam').'/lib//raphael/raphael-min.js'] = array(
   
   Line 162: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
     $page['content']['#attached']['js'][drupal_get_path('module', 'aspam').'/lib/crypto-js/md5.js'] = array(
   
   Line 166: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
     $page['content']['#attached']['js'][drupal_get_path('module', 'aspam').'/fonts/helvetica.js'] = array(
   
   Line 170: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
     $page['content']['#attached']['js'][drupal_get_path('module', 'aspam').'/js/aspam.js'] = array(
   
   Line 176: curly braces { should end a line, not start one
   {
   s
   Line 182: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
   function aspam_captcha_validation($solution = null, $response = null)
   
   Line 183: curly braces { should end a line, not start one
   {
   
   Line 193: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
           ? md5($vhash.$cres[$i].ord($cres[$i]))
   
   Line 194: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
           : md5($vhash.ord($cres[$i]));
   
   Line 197: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
           if (variable_get('aspam_rsa_encryption', false)) {
   
   Line 201: else statements should begin on a new line
           } else {
   
   Line 206: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
               $resp = true;
   
   Line 207: else statements should begin on a new line
           } else {
   s
   Line 208: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
               $resp = false;
   
   Line 214: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       return false;

2. In aspam.admin.inc file:
   

   Line 12: curly braces { should end a line, not start one
   {
   
   Line 20: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 29: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 38: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 47: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 56: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 64: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 70: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#collapsible' => true,
   
   Line 71: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#collapsed' => true,
   
   Line 77: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#default_value' => variable_get('aspam_rsa_encryption', false),
   
   Line 80: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => false,
   
   Line 86: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#default_value' => variable_get('aspam_pem_private_key', false),
   
   Line 88: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => false,
   
   Line 94: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#default_value' => variable_get('aspam_pem_public_key', false),
   
   Line 96: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => false,
   
   Line 113: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#collapsible' => true,
   
   Line 114: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#collapsed' => true,
   
   Line 123: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 132: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 141: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 150: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 159: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 168: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 177: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 186: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 195: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 204: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 213: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 222: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 231: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
       '#required' => true,
   
   Line 243: curly braces { should end a line, not start one
   {

3. In aspam.install file:
   

   Line 12: curly braces { should end a line, not start one
   {

4. Aspam widget width and Aspam widget height fields are not working. They just providing padding to the ASPAM text.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

The installation instructions in README.txt are totally different from those found on the project page.

For instance, in README.txt we can read the following ...

INSTALLATION
------------
 
 * Install as you would normally install a contributed Drupal module. See:
   https://drupal.org/documentation/install/modules-themes/modules-7
   for further information.

 * You may want to disable Toolbar module, since its output clashes with
   Administration Menu.

Moreover, in README.txt, the word encryption does not appear even once while the project page explains how to increase security by enabling encryption.

I believe that the README.txt is the one that should/could potentially be more verbose than the project page. For example, you can move the encryption related explanations from the project page into the README.txt file and simply add a reference to it. More advanced usages are usually described in detail in README.txt if they are too verbose.

You can also keep it in both. It's a good selling point after all.

Also, in README.txt you are warning users about a conflict with the Toolbar module. This is very important, but it is not mentioned on the project page. And it should be mentioned in a more prominent fashion under a heading called "Known problems" according to the project page template. Some users may have a project that depends unconditionally on the Toolbar module and disabling it may not be a viable option. So they would want to easily spot that problem before downloading this module.

Readme improvements are a good point but surely not application blockers. Anything else that you found or should this be RTBC instead?

It is RTBC
Correction, I have just noticed few things that may not be acceptable. I am performing a manual review that I will post soon.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. But this is not a blocking issue. There are plenty of Turing test related modules for Drupal. However, this one add a very interesting way of securing forms.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

No: Does not follow the licensing requirements. The libs/ folder contains several libraries that have custom licenses, BSD licenses or MIT licenses. This is a blocker and cannot be allowed. Additionally, there is a file called LICENSE in the project folder. This is now allowed.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code. The module is embedding several third party libraries that should be placed in drupal's official libraries folder instead of being committed in Git on drupal.org.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. There are two README files, one called README.md that does not follow any Drupal README template and another one called README.txt that is following Drupal's README template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity. There seems to be enough code for what the module does.

Secure code

Yes: Meets the security requirements as far as I can tell.

Coding style & Drupal API usage

1. (*) Not using Drupal's libraries when it should. Instead, libraries are being embedded in the module's libs folder. No third party library should be committed on drupal.org

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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