Commerce Popular Products

Auto code review shows lots of code standard errors. http://ventral.org/pareview/httpgitdrupalorgsandboxanthonylindsay1643068git

You can try use the this API http://drupalcontrib.org/api/drupal/contributions!commerce!modules!produ...

instead of

function commerce_popular_products_get_product_types() {
$sql = 'SELECT name from {commerce_product_type}';
$result = db_query($sql)->fetchCol();
return $result;
}

Hi,

I have reviewed your module also done setup with your specified modules as given on http://drupal.org/sandbox/anthonylindsay/1643068.
I used following modules as specified :
views 7.x-3.3,
commerce 7.x-1.3,
ctools 7.x-1.0,
Address field 7.x-1.0-beta3,
rules 7.x-2.1,
Entity api 7.x-1.0-rc3

I got following fatal error :
Fatal error: require_once(): Cannot redeclare class insertquery_mysql in /home/param/svn/drupalreviews/includes/database/database.inc on line 1742

I tried to replicate error after removing your module commerce module worked fine.
I came accross this issue when I tried to add product and while adding 2nd product I got this fatal error.
It seems some issue is there in module.

Also some minor issues mentioned by ventral about coding standards.

Regards,
prathK
extrimity.in

Major error on install

At first I couldn't make your module working:

First I installed commerce and views. Then I installed your module and the Store-link in admin-menu changed to your config-page: (see screenshot)

After I clicked on the link, the whole drupal-installation was broken.
When I first install also commerce_products and commerce_products UI, installing your module is working.

readme.txt

You should name your readme.txt "README.txt" which is the common way to name it. If you are working on OS X this post could be useful for you.

Please fix the installation-error, so we can continue reviewing your project.

I'm having issues getting the module functioning. When I first enable the module I get several warnings that all say the same thing. "Illegal offset type in isset or empty block.module:435" I've received this error on two different installs on my machine. After that I can go to the admin page for the module but it always tells me "You have not defined any product types. You need to define some first." and I can't figure out how to define the product types it will use?

Also, I noticed that the package is Commerce Extras. For consistency you should probably put it in Commerce (Contrib) so it matches other commerce modules.

   9 /**
  10  * Implements hook_install().
  11  */
  12 function commerce_popular_products_install() {
  13   $updates = drupal_get_schema_versions('commerce_popular_products');
  14   if (!empty($updates)) {
  15     foreach ($updates as $version) {
  16       $function = 'commerce_popular_products_update_' . $version;
  17       if (function_exists($function)) {
  18         $ret = $function();
  19         foreach ($ret as $id => $value) {
  20           if (!empty($value['query'])) {
  21             $error = ($value['success']) ? 'status' : 'error';
  22             drupal_set_message(check_plain($value['query']), $error);
  23           }
  24         }
  25       }
  26     }
  27   }
  28 }

You don't need this at all.

You run a complex query every time the block is displayed. This means that the code won't scale at all.
A version usable in production would need to do it in a completely different way, for example by maintaining a table with the counts of products sold (product_id | times_sold), and then the block can just fetch the top X counts.

A small suggestion, put the file commerce_popular_products.views.inc in a different folder called views and change the corresponding code in commerce_popular_products.module (Line #304 to #311)

function commerce_popular_products_views_api() {

  return array(
    'api' => 3,
    'path' => drupal_get_path('module', 'commerce_popular_products') . 'views',
  );
}

Hi anthonylindsay,
Your project is interesting :-)

• Your "repository link" in is not correct for anonymous users. Please change to: http://git.drupal.org/sandbox/anthonylindsay/1643068.git
• I think the better way is to set commerce_popular_product.module (lines 138-139) use the t () for a variable without verification check_plain Read more in Drupal API Documentation for function t()
• Errors from http://ventral.org/pareview/httpgitdrupalorgsandboxanthonylindsay1643068git still not been removed.
• To get the project approved before join in review bonus program.

Good luck,
frankye

I did a little manual review and the code looks great. Found only these little things:

1. In commerce_popular_products_uninstall(), there seems to be a unused variable '$status = '';'
2. I'm not sure about the use of $default = "..." in variable_get, never seen that before. For instance:
   

   variable_get('commerce_popular_products_time', $default = 'month')
   //could be
   variable_get('commerce_popular_products_time', 'month')
   

3. In commerce_popular_products_block_view the block title is defined like this:
   

   $title = variable_get('commerce_popular_products_title');
   $block['subject'] = t('@title', array('@title' => $title));
   

   I don't think it makes much sense to use "t" here. There is an i18n submodule i18n_variable which should already handle this. So I think you could do simply:
   $block['subject'] = variable_get('commerce_popular_products_title');

I manually reviewed this module both on a live website and a new installation. It works like a charm. Great job and great module. I am going to use it for sure. Thank for your contribution anthonylindsay!

I've glanced through the project, and came up with the following comments:

Info file:

This can be simplified a bit, by eliminating common dependencies. For example, if commerce_ui depends on commerce, you can list the commerce_ui dependency and leave out the commerce requirement (which is inherited). Definately not a showstopper. :)

Module file:

Line 191: Could rewrite as "<p>" . t(...) . "</p>", so that translations don't need to include the html tags

Lines 203, 204: The $default assignment isn't needed here ... just use variable_get('variable_name', 'default_value')

Line 224: Pull the html tags out of the t() call, and pass the link as an argument to t() instead of embedded. This way, any string translations don't need to include the full link tag. See http://drupal.org/node/322774 for further explanations.
Eg:

$markup = '<h4>' . t('Product types') . '</h4>';
$markup .= '<p>' . t('You have not defined any product types. You need to <a href = "@add-product-page">define some</a> first.', array('@add-product-page' => url('/admin/commerce/products/types/add'))) . '</p>';

None of the above are showstoppers ... however, the module as submitted is subject to an XSS vulnerability. The 'commerce_popular_products_*' variables contain user-submitted content which is not sanitized on output. By entering a malicious block title in the configuration form, I was able to execute arbitrary javascript via the module when the block was displayed. I suspect there could be similar vulnerabilities related to the product type form entry (and $products variable); though I was not able to prove it.

Please ensure that all user provided input is sanitized before being output (even if it was not submitted via this module) ... in this case, the $block['subject'] variable is where the vulnerability sneaks through.

Tagging (for metrics purposes, this tag is used to track how many applications we find with security issues).

Ah ... I see ... this was introduced based on the recommendation in #19 ... Previously, the sanitization occured via the t() function.

However, the comments in #19 are correct from the perspective that we do not run dynamic user-submitted values through t(), as there is no way for a translator to know what a user is going to enter as the block title; and thus can not translate it. The solution here would be as stated in #19, but wrapped in a check_plain() or filter_xss() call.

(retagging ... d.o likes losing tags arbitrarily ...)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

Changed git repo address to work for anonymous users.

Fixing tags.

Oh, and this needs review, right?

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

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

  FILE: /home/klausi/pareview_temp/README.txt
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   18 | WARNING | Line exceeds 80 characters; contains 333 characters
  --------------------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/commerce_popular_products.install
  --------------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 2 LINES
  --------------------------------------------------------------------------------
   6 | ERROR | Doc comment short description must be on a single line, further
     |       | text should be a separate paragraph
   7 | ERROR | There must be exactly one blank line after the file comment
  --------------------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/commerce_popular_products.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   5 | ERROR | Doc comment short description must end with a full stop
  --------------------------------------------------------------------------------
  

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

1. commerce_popular_products_settings(): do not call render() here, just return the form, Drupal will render it automatically for you later.
2. commerce_popular_products_settings(): you actually don't need this function, just use 'drupal_get_form' as page callback in hook_menu() and your form name.
3. commerce_popular_products_settings_form(): the check_plain() for $title is wrong here, because #default_value will get sanitized automatically and double escaping is bad. See https://www.drupal.org/node/28984
4. commerce_popular_products_settings_form_submit(): you can remove this function and just use system_settings_form() in commerce_popular_products_settings_form() instead which will save the values for you.
5. commerce_popular_products_get_product(): why do you have to foreach() over the $result of the query? The sorting can be done in the query, right? You could just fetchAllKeyed()?
6. I would not save commerce_popular_products_time as string "day" or "week", but rather save it as unix time interval in seconds. That way people can set the variable to an arbitrary period of their choice from a custom module and are not bound to the 3 options.

But that are not critical application blockers, otherwise I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

no objections for more than a week, so ...

Thanks for your contribution, anthonylindsay!

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.