[D7] Reactor Integration

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

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.

@smido, these two links should help you to fix the master and default branches #1659588: Setting a default branch and #1127732: Moving from a master to a version branch.

The README should be wrapped to 80 cols.

Assigning to myself for a proper review, which will be in the next day or two.

Automated Review

Git errors:

• There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /Users/mattd/Dropbox/Drupal/par/pareview_temp/README.md
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
  ----------------------------------------------------------------------
    3 | WARNING | Line exceeds 80 characters; contains 117 characters
    9 | WARNING | Line exceeds 80 characters; contains 96 characters
   12 | WARNING | Line exceeds 80 characters; contains 116 characters
  ----------------------------------------------------------------------
  
  Time: 157ms; Memory: 5.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.

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

No: Does not follow the guidelines for master branch. You have a 7.x-1.x branch, but still have master. You need to remove it.

Licensing

[Yes: Follows / 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. This is a really lean README. Is there anything else you can add?

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

This module has a few security problems, one of which is directly exploitable. As part of Review Admin mentoring, I am assigning to naveenvalecha to find them, in particular the direct exploit.

Coding style & Drupal API usage

What is release.sh for? I don't think you need this in the repo?

You don't need to set a variable in the install. Just pass the default to variable_get().

In reactor_admin_settings_form(), wrapping single fields with collapsible kinda seems like overkill.

Your non-hooks need proper doclocks, eg reactor_get_picture_url().

In reactor_page_alter(), use user_is_anonymous() instead of checking the uid.

User tracking isn't an emergency; log config problems as errors.

Using #attached is preferred over drupal_add_js().

I get a consistent warning

    Notice: Undefined variable: profile_url in reactor_get_picture_url() (line 47 of /home/s04358c6ec93c294/www/sites/default/modules/2434189/reactor.module).
    

You should have a hook_help()

You should have a hook_requirements() to check the config.

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.

First,Thanks for your contribution.

Automated Review

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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.

FILE: ...s/naveeen/Documents/pareviewsh/pareview_temp/inc/reactor.admin.inc
---------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
---------------------------------------------------------------------------
 14 | WARNING | A comma should follow the last multiline array item.
    |         | Found: )
 30 | WARNING | A comma should follow the last multiline array item.
    |         | Found: )
---------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/naveeen/Documents/pareviewsh/pareview_temp/reactor.module
---------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
---------------------------------------------------------------------------
 136 | ERROR | Whitespace found at end of line
 148 | ERROR | Whitespace found at end of line
 150 | ERROR | Files must end in a single new line character
---------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/naveeen/Documents/pareviewsh/pareview_temp/README.md
---------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
---------------------------------------------------------------------------
 19 | WARNING | Line exceeds 80 characters; contains 111 characters
---------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
---------------------------------------------------------------------------

Manual Review : Read 10c19a0d7be3...

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

Yes: Follows the licensing requirements.

3rd party assets/code

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

README.txt/README.md

Yes: Follows 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

No: Meets the security requirements. / No: List of security issues identified.]

• reactor_page_alter :
  

        $script .= "
          _rcr('collect', {
            user_id: '" . $user->uid . "', 
            username_s: '" . $user->name . "',
            email_s: '" . $user->mail . "',
            profile_picture_s: '" . reactor_get_picture_url($user->picture) . "',
            registration_date_d: " . $user->created . ",
            active_b: " . $user->status . ",
          });
        ";

  You should sanitize the data using proper sanitization functions before passing to drupal_add_js .http://drupal.stackexchange.com/questions/10280/is-check-plain-enough

Coding style & Drupal API usage

1. (*) Notice: Undefined variable: output in reactor_help() (line 149 of /Applications/MAMP/htdocs/d7.dev/sites/all/modules/sandbox/reactor_am/reactor.module).
2. Project page needs some improvements.
3. reactor_requirements : The hook_requirements is not used in the module.So remove it.
4. reactor_page_alter : You are adding js file using drupal_add_js rather use #attached to add the JS file
   with page rendered array.
5. reactor_page_alter : you are adding js on every page.Please add a comment.I would suggest for long term create a seperate file.
6. reactor_page_alter :
   

     // Do not collect anonymous users.
     if (user_is_anonymous()) {
       return;
     }

   Please specify on the project page regarding this that the module is not collecting data for anonymous users.

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.

This is what I had:

  ', alert('XSS'), '
  

You are also using values from the $user object directly. I could not directly exploit these, because of validation, but these should also be passed via settings, which are escaped properly by core when added to a page.

Removing review bonus tag, you have not done all manual reviews. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Manual Review : Read 47bd61...
The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 andhttps://www.drupal.org/node/1051722

1. reactor.admin.inc : reactor_admin_settings_form : Wrong doc comment.This is not a hook.
2. reactor_page_alter : Please specify on the project page that the module not track anonymous users.For long term make it configurable at admin part and choose the admin to track which roles.
3. reactor_page_alter : No need to sanitize uid,email,picture,created and status Remove the check_plain from them.check_plain($user->name)Use format_username that will also work later when module will enable the tracking for anonymous users.
4. If reactorid will be numeric always then you can restrict it to numeric at the admin configuration as well.Then there will be no need to sanitize this variable as well.

Assigning to heddn for final review if he has time.

Manual Review : Read 82a55b...
Your git commits are still not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722 if you have changed the email address then file an issue in the webmasters issue queue to update your commits specified with that email address.
No Objections from more than 1 week.
No blockers.
Thanks for your contribution, @smido!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.