[D7] Commerce Product Comparison

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.

removed automated reviews

Hi,

Very good module!

Manual review:
- Add commerce dependency (.info)
- Missing return statement: commerce_product_comparison_forms() and commerce_product_comparison_block_view().

Bye,

Ran coder-review against the module to check for adherence to Drupal coding standards. It came up very clean. These two minor issues came up:

sites/all/modules/commerce_product_comparison/commerce_product_comparison.module:
+38: [normal] missing space after comma
+47: [normal] missing space after comma

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

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

  
  FILE: /home/klausi/pareview_temp/commerce_product_comparison.info
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   8 | ERROR | Files must end in a single new line character
  --------------------------------------------------------------------------------
  

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:

1. commerce_product_comparison_add_form(): do not document $form and $form_state, see https://drupal.org/node/1354#forms . Also elsewhere.
2. "if (arg(0) != 'admin') {": better use path_is_admin().
3. commerce_product_comparison_list_form_clear_submit(): why don't you use commerce_product_comparison_list_clear() directly as submit callback?
4. commerce_product_comparison.module: the module file is quite long. Have you considered splitting up page and form callbacks in *.pages.inc and *.admin.inc files?
5. "'#value' => 'Submit',": all user facing text must run through t() for translation.
6. I was not able to install your module: Notice: Undefined index: schema_fields_sql in includes/entity.inc on line 265. Catchable fatal error: Argument 2 passed to SelectQuery::fields() must be of the type array, null given, called in includes/entity.inc on line 284 and defined in includes/database/select.inc on line 1300
7. commerce_product_comparison_compare_page(): this looks vulnerable to XSS exploits, the product title is passed unsanitized to the theme function and I could not find a preprocessor that would sanitize the title. Unfortunately I was not able to exploit this because the module wouldn't install. Make sure to read https://drupal.org/node/28984 again.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

1. installation works now on simplytest.me, whatever you changed - it works now.
2. commerce_product_comparison_compare_page(): why do you have to call render() here? Just return the render array and Drupal will render it automatically for you later? Same in commerce_product_comparison_config(), please check all your code.
3. commerce_product_comparison_compare_delete(): one could argue that this is vulnerable to CSRF exploits, but since no persistent data is changed (only stuff in the user's session) this might not qualify as security hole. See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained

Otherwise looks got to me! Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to patrickd as he might have time to take a final look at this.

  13   // Set default permission for authentificated and anonymous user.
  14   user_role_change_permissions(DRUPAL_ANONYMOUS_RID,
  15     array('compare properties' => 1)
  16   );

Rather use TRUE/FALSE than integers when possible

16 drupal_set_title(t('Comparison'));
why setting the title again?

But just nitpicking..

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:

• 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.