[D7] Commerce Product Specific Attributes

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

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.

Reversing an unwarranted priority upgrade.

Your issue summary is missing a git clone command.

PAreview is not happy: http://pareview.sh/pareview/httpgitdrupalorgsandboxnostop82232329git

2. Basic repository checks

2.1 Ensure the repository actually contains code.

Repository contains actual code.

2.2 Ensure you are working in a version specific branch.

Uses version-specific branch.

3. Security Review

3.1 Ensure the project does not contain any security issues.

I didn't encounter any obvious security problems.

4. Licensing checks

4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file.

No LICENSE.txt

4.2 Ensure the repository does not contain any 3rd party (non-GPL) code.

No use of 3rd-party code.

5. Documentation checks

5.1 Ensure the project page contains detailed information.

Project page has detailed information.

5.2 Ensure the repository contains a detailed README.txt.

README.txt exists, but provides no useful documentation.

5.3 Ensure the code contains a well-balanced amount of inline-comments.

The code could afford to have more and better inline comments.

6. Coding standards and style

6.1 Run an automated review and ensure there are no major issues.

FILE: ...es/views/handlers/commerce_psa_line_item_handler_field_edit_options.inc
--------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
8 | ERROR | Class name must begin with a capital letter
8 | ERROR | Class name must use UpperCamel naming without underscores
13 | ERROR | Visibility must be declared on method "construct"
24 | ERROR | Visibility must be declared on method "render"
31 | ERROR | Method name
| | "commerce_psa_line_item_handler_field_edit_options::views_form"
| | is not in lowerCamel format, it must not contain underscores
31 | ERROR | Visibility must be declared on method "views_form"
--------------------------------------------------------------------------------

FILE: ...iew_temp/includes/views/handlers/commerce_psa_handler_field_options.inc
--------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
8 | ERROR | Class name must begin with a capital letter
8 | ERROR | Class name must use UpperCamel naming without underscores
13 | ERROR | Visibility must be declared on method "construct"
25 | ERROR | Visibility must be declared on method "query"
33 | ERROR | Visibility must be declared on method "render"
--------------------------------------------------------------------------------

FILE: ...var/www/drupal-7-pareview/pareview_temp/includes/commerce_psa.pages.inc
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
65 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

FILE: ...var/www/drupal-7-pareview/pareview_temp/includes/commerce_psa.admin.inc
--------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
11 | ERROR | [ ] Class property $db_table should use lowerCamel naming
| | without underscores
12 | ERROR | [ ] Class property $db_pk should use lowerCamel naming without
| | underscores
13 | ERROR | [ ] Class property $db_parent_pk should use lowerCamel naming
| | without underscores
298 | ERROR | [x] There must be no space before the colon in a DEFAULT
| | statement
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...ar/www/drupal-7-pareview/pareview_temp/includes/commerce_psa.common.inc
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
194 | WARNING | Line exceeds 80 characters; contains 107 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/commerce_psa.js
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
42 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/commerce_psa.info
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
9 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
1 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/commerce_psa.module
--------------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 3 LINES
--------------------------------------------------------------------------------
186 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
216 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
247 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/templates/edit-options.tpl.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
1 | ERROR | [x] Whitespace found at end of line
11 | ERROR | [ ] Files must end in a single new line character
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
7. API and best practices Review

7.1 Ensure you are using Drupals API correctly.
Adherence to the Drupal API seems to be good.

Specific Issues that I noticed.
using include_once 'includes... in module head may be an issue in certain circumstances. module_load_include is the preferred method.
in commerce_psa_module_implements_alter(), you shouldn't count on being able to put your hooks where you want in the invoke queue.
in commerce_psa_permission(), a short title and a longer description might be advisable, from a user's perspective.
in commerce_psa_line_item_handler_field_edit_options,inc on line 37, you add drupal's ajax.js directly, when you probably should do something like drupal_add_library('system', 'drupal.ajax');

Regarding inheriting Views Handler and automated code review: Yes, you're right, and I think that can safely be ignored as a false positive.

Regarding $form_state['input']: It looks like you're trying to pass a non-form element as a form element, and that is why it's only available in $form_input. Consider refactoring to avoid doing this (if this is, in fact, what you are doing).

Regarding wrapping a variable with t(), I would do something like this:

$options[$value['option_id']] = t('@name', array('@name'  => $value['name']));

Though I recognize that it's largely pointless to do so.

1. I've looked at the code in question for your table form, and the most obvious issue is that you aren't using '#tree' => TRUE at each level of the hierarchy:
   When a button is pressed, the inputs are sent to the form processing function (which is why they are directly available per row in $form_state['input']), but they collide before getting to $form_state['values'].

   If you insert

      $table[$i] = array(
          '#tree' => TRUE,
       );
   

   At or around line 114 and this

   $form['table']['#tree'] = TRUE;
   

   at line 45 in commerce_psa.admin.inc, then you should be able to address the data directly in $form_state['values'].
   This isn't exactly trivial, since using $form_state['input'] directly is a security issue.

2. I understand what you are trying to do with t(), the problem is that passing dynamic data to t() can be a security risk, because it isn't escaped. A better option might be to call locale directly, then escape the output.

So I just noticed something; why are your class methods magical?

yes, methods like __ are treated as magic by PHP. And the rule is, that they are not to be called in code. They're for PHP. (PHP's rule, I don't know if Drupal cares)

There are some more errors and warnings in PAreview - they are certainly worth paying attention to, but I don't think they should gate the release. Aside from the use case stated (events with two dimensions of attributes and the need to automatically create variants), there are plenty of other product categories that would benefit from this and make it easier to get Drupal Commerce acceptance. Any product that has a natural dimension of configurability would benefit from this (I'm running a web store for printed photography and we hard-coded a similar solution that is far less elegant).

For these reason: I am strongly in favor of promoting this module for the general public, ideally after fixing the outstanding PAreview items.

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxnostop82232329git (commit 1e2c82e) found 6 issues from Code Sniffer, none of which seem severe enough to hold the release. DrupalPractice found 5 issues that all come down to the use of "$form_state['input']" instead of "$form_state['values']". The latter should be easy to address and should indeed be addressed before release.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

Nothing found that needs to be addressed before release, except for the issues identified by PAreview.sh (see above). Switching back to "needs work" for a fix.

I checked the remedies mentioned above and agree that the remaining issues should not hold a release. Setting status to RTBC.

manual review:

1. commerce_psa.js: you should use 2 spaces per indentation level. See https://www.drupal.org/node/172169#indenting
2. commerce_psa_module_implements_alter(): the comment should say why you need to alter the form earlier, e.g. before which other hook yours needs to run.
3. commerce_psa_theme_registry_alter(): why do you need to swap out inline_entity_form? Please add a comment.
4. When I enter attributes at node/1/psa and try to save I get an Exception: "EntityMalformedException: Missing bundle property on entity of type commerce_product. in entity_extract_ids()".
5. The "Manage Options" screen is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as attribute name and then click on manage options I get a nasty javascript popup. You need to sanitize user provided text before printing to HTML in commerce_psa_option_form(). Make sure to read https://www.drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

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.