[D6] Social River

Not a full review, but a quick glance identified the following:

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.

Licensing issues

The jquery plugins (and lifestream.css) appear to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org. This also allows for multiple Drupal modules to leverage the same plugin code base.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 6.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/social_river.module:
   +19: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +55: [minor] There should be no trailing spaces
   +58: [minor] There should be no trailing spaces
   +63: [minor] There should be no trailing spaces
   +70: [minor] There should be no trailing spaces
   +81: [minor] There should be no trailing spaces
   +83: [minor] There should be no trailing spaces
   +87: [minor] There should be no trailing spaces
   +102: [minor] There should be no trailing spaces
   +103: [minor] There should be no trailing spaces
   +107: [minor] There should be no trailing spaces
   +122: [minor] There should be no trailing spaces
   +126: [minor] There should be no trailing spaces
   +129: [minor] There should be no trailing spaces
   +130: [minor] There should be no trailing spaces
   +140: [minor] There should be no trailing spaces
   +143: [minor] There should be no trailing spaces
   +144: [minor] There should be no trailing spaces
   +150: [minor] There should be no trailing spaces
   +161: [minor] There should be no trailing spaces
   +166: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +187: [minor] There should be no trailing spaces
   +195: [minor] There should be no trailing spaces
   +239: [minor] There should be no trailing spaces
   +242: [minor] There should be no trailing spaces
   +249: [minor] Missing parenthesis after function name
   +287: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +295: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +343: [minor] There should be no trailing spaces
   +379: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 1 files, 2 normal warnings, 28 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.
• ./social_river.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function social_river_menu() {
  

• ./social_river.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function service_list() {
  

• There should be a space before and after operators like ==, ===, && and ||. See http://drupal.org/node/318#controlstruct
  

  css/lifestream.css:25:#edit-social-river-user-settings-bitly { (BASE64 inline) }
  (and more)
  

• There should be no space after the opening "(" of a control structure, see http://drupal.org/node/318#controlstruct
  

  social_river.module:277:  if ( (!user_access('view own social river') && !user_access('view all social rivers') ) ||
  

• There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
  

  social_river.module:103:  $form['social_river_user_settings']['form_column_wrapper_start'] = array(    
  social_river.module:122:      $form['social_river_user_settings'][strtolower($service)] = array(      
  social_river.module:140:  $form['social_river_user_settings']['form_column_wrapper_end'] = array(    
  

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

  ./social_river.install ./social_river.module ./social_river.info ./README.txt ./css/lifestream.css
  

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.

• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.

Edit: I am really not sure whether base64 encoded is appreciated or even deprecated by security teams. Also there would be probably issues with the built-in CSS compression. Is there any reason not to put that content in separate files?

dineshcooper,

Regarding the css comment, the in-file image declarations may help optimise load time and performance as you say, but they come at the expense of readability ... and make it much less trivial for a themer to override an image or declaration.

I suspect the errors on lines 167 and 289 are false positives which are triggering on 'service_username'.

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• lifestream.css: formatting errors, see http://drupal.org/node/302199
• "'access arguments' => array('administer site configuration'),": this should use the "administer social river" permission, right?
• social_river_user_settings_form(): doc block is wrong.
• "$services_enabled = variable_get('social_river_services_enabled', $services);": $services is undefined here.
• social_river_user_settings_form(): indentation errors in the if/else statements, always use 2 spaces per level.

It appears that you have successfully completed the application process with a different application, and been granted the 'create full projects' permission:

Privatemsg Templates: http://drupal.org/node/1252548

Once their first application has been successfully approved, then an applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue.

With this in mind, I have marked your other applications as 'closed(duplicate)'. If this is incorrect, and you do not yet have the ability to create full projects, then please feel free to re-open this application.

Thanks in advance for your patience and understanding!