[D7] PrivacyBuilder

Hello,

Please review at least 3 projects and add the urls of reviews on your issue page so that community members will consider reviewing your code.

Here is my observation on your module:

1. Where ever possible try making use #attach to attach js rather than drupal_add_js
2. Make use of t() function'#markup' => '<div id="consents"></div>'
3. The code has indentation issues. make use of pareview.sh or coder module
4. There is a un necessary temp file of the module

Hi Robert!

Automated reviews
There are many errors reported by automated review tools, did you already check them? See
http://pareview.sh/pareview/httpgitdrupalorgsandboxronslow2376897git

Manual review
1. Coding standard is very poor, I think you didn't follow the Drupal coding Standards.
2. Don't commit junk files, use git diff before committing code.
3. Info file has only one line of incorrect code

"name = PrivacyBuilder" name should be like this,
name = "PrivacyBuilder"
missing version like 7.x

4.
Not able to enable module

5.

   drupal_add_js('sites/all/modules/privacybuilder/js/jquery.validate.min.js', 'file');
   drupal_add_js('sites/all/modules/privacybuilder/js/mp_client.js', 'file');

Don't use file path directly, Make use of drupal_get_path()

e.g.

  drupal_add_css(drupal_get_path('module', 'privacybuilder') . '/js/mp_client.js');

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.

In my testing, I have found one syntax error when I enabled this module.

Parse error: syntax error, unexpected '[' in ..\sites\all\modules\privacybuilder\privacybuilder.module on line 178

Thanks

For review by me

Hi @ronslow, I had time to review your module and found some issues:

Manual Reviews

- Please keep the standard of drupal issue to promote your project. Please read carefully this article.
https://www.drupal.org/node/1011698
- Please remove .gitignore file in git source.
- All variables used in your module should be removed when uninstall. For example, 'privacybuilder_infofield_map'.
You can use varaible_del() function in hook_uninstall
- Please add your permission role (administer privacybuilder settings) in hook_permission().
- Drupal prefer drupal_base64_encode() function over the use of base64_encode(). Please use drupal_base64_encode() function instead of base64_encode() function in your module.

Automated Review

The automated review still found a lot of best-practices and coding-standards violations. See http://pareview.sh/pareview/httpgitdrupalorgsandboxronslow2376897git for more information.

In two places, you added items to $form_state['values'] from $form_state['input'] manually, but Drupal does this automatically in drupal_form_submit().

The docblock for privacybuilder_admin() says it "Implements hook_admin()." but there is no hook_admin() in Drupal 7. Rather, it's a a hook_menu() page router callback function.

Manual Review

Individual user account

Follows the guidelines for individual user accounts.

No duplication

Does not cause module duplication and/or fragmentation.

Master Branch

Follows the guidelines for master branch.

Licensing

Does not follow the licensing requirements.

You include the jQuery Validation Plugin which is MIT-licensed. Only GPL-licensed code is allowed in Drupal repositories.

3rd party assets/code

Does not follow the guidelines for 3rd party assets/code.

You include the jQuery Validation Plugin.

README.txt/README.md

Follows the README Template.

Code long/complex enough for review

Follows the guidelines for project length and complexity.

Secure code

No:

1. The permission administer privacybuilder settings allows a user to insert raw JSON (or their own malicious JavaScript; i.e.: XSS attack) into the page. As far as I can see this is intentional and unavoidable, but the administer privacybuilder settings role should be defined with 'restrict access' => TRUE. See hook_permission() for more information.

Possibly insecure:

1. The site owner's PrivacyBuilder.eu password is stored, unencrypted, in the database. If there is a way to use an access token instead, use it. If this "password" is supposed to be an access token, make the UI very clear that it should be their access token instead of their actual password.

Coding style & Drupal API usage

1. (*) In privacybuilder_admin, the there is a form control named privacbuilder_infofield_map (note the missing "y"); but the rest of the code, including it's #default_value refer to privacybuilder_infofield_map. The difference in the spelling of the name of the form control will cause the data entered in this field to be saved to a variable name that is never used elsewhere in the module; and other parts that read from the correctly-spelled variable name will get a NULL value.
2. (*) Some parts of the module refer to a variable named privacybuilder_username and other parts refer to a variable named privacybuilder_user. If this is intentional, you will need to variable_delete('privacybuilder_user') in privacybuilder.install::privacybuilder_uninstall().
3. (*) Inline control structures are not allowed
4. (+) There are a LOT of incorrect indentations, extra whitespace at EOL, etc.
5. (+) There's a lot of code which has been commented out, especially in mp_client.js. This should be removed before release.
6. There are a number of comment style issues.

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.

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.

Accidentally set the wrong category.

Updating the issue summary.

I also noticed that there are some translatable text problems:

• In privacybuilder_form_user_profile_form_alter(), the label "Consents" is not translatable:
  

    $form['consent_div'] = array(
      '#markup' => '<label for="consents">Consents</label><div id="consents"></div>',
    );
  

• title and description strings in privacybuilder_permission() need to be wrapped in t() functions.
• There are strings in mp_client.js (e.g.: "Forget me") which need to be wrapped with Drupal.t(): see https://www.drupal.org/node/304258 and https://www.drupal.org/node/114774#javascript-translation
• The list of consents is not translatable. See Translating user-entered strings in D7 for a tutorial on how to make them translatable.

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.