Ubercart Attribute Per Role

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.

You should have a read of the release naming conventions and possibly rename the branch to 6.x-1.x if it is targetted at Drupal 6.

git checkout -b 6.x-1.x
git push -u origin 6.x-1.x

Review of the master 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/uc_attribute_per_role.module:
   +4: [minor] Comment should be read "Implements hook_foo()."
   +48: [minor] Comment should be read "Implements hook_foo()."
   +58: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 1 files, 3 minor warnings, 0 warnings were flagged to be ignored
  

• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• ./uc_attribute_per_role.admin.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

        // Append 'X more' if more than $MAX_DISPLAYED_ATTR exist in the definition
        // If we could not load a definition, it either does not exist or the role does not exist.
  

• Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./uc_attribute_per_role.admin.inc:71:function uc_attribute_per_role_edit($form_state, $target_role_id=NULL) {
  ./uc_attribute_per_role.module:114:  $definition = db_fetch_array(db_query("SELECT * FROM {uc_attribute_per_role} WHERE role_id=%d", $role_id));
  ./uc_attribute_per_role.module:146:  $num_deleted = db_query("DELETE FROM {uc_attribute_per_role} WHERE role_id=%d", $role_id);
  

• Do not use t() in hook_schema(), this will only generate overhead for translators. (Also, t() isn't available this early in the execution.)
  

      'description' => t('Tracks which attributes should be hidden for a given role.'),
          'description' => t('The role ID for which to hide attributes for.'),
          'description' => t('A serialized array of IDs of attributes to be hidden.'),
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

Manual review comments:
- I didn't have time to test directly (and is probably a non-issue), but I did note the lack of sanitization around retrieved roles. While it's true that it's a slim chance that someone malicious would have the ability to create roles, it may be worthwhile to add a few more check_plain() calls just in case (or alternatively, create a role of <script>alert('xss');</script> and make sure it doesn't cause pop-ups anywhere on the site).

- Otherwise, things look pretty clean ... nice work!

I'll submit a bug about that soon.
Please do. :)

BTW - does PAReview run a modified version of coder
The review script runs against a D7 version of coder, and those lines were actually a coding style change from D6 to D7 ... that's likely why they didn't show up. Minor, in any case. :)

All items in #1 and #2 have been addressed ... I'm going to leave this open for a bit longer, to give a chance for someone more familiar with ubercart to take a peek at it (or maybe an actual install/test) ... but from simply looking at the code, I don't see any more issues at the moment.

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/uc_attribute_per_role.module:
   +6: [minor] There should be no trailing spaces
  
  sites/all/modules/pareview_temp/test_candidate/uc_attribute_per_role.install:
   +8: [minor] Comment should be read "Implements hook_foo()."
   +17: [minor] Comment should be read "Implements hook_foo()."
   +25: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 3 files, 4 minor warnings, 0 warnings were flagged to be ignored
  

• ./uc_attribute_per_role.admin.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

        // If we could not load a definition, it either does not exist or the role does not exist.
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so I can get back to yours sooner.

manual review:

• "$MAX_DISPLAYED_ATTR": function variables should be lower cased, see http://drupal.org/node/318#naming . Maybe you want to use this variable as a constant, then you should use define().
• "t('Role display settings for @role were not found in the database.', array('@role' => $roles[$target_role_id])": use the % placeholder, which will add the emphasize tags automatically for you. Also elsewhere.

But that are relatively minor issues, so RTBC for me.

Please take a moment to make your project page follow tips for a great project page.

Adding tag per comment #2 identifying some XSS.

On line 122 of uc_attribute_per_role.admin.inc you create a selectbox with $available_roles which looks like it could be a path for XSS. Can you test that for XSS and see if it's possible to execute Javascript via that select box? Per the Handle text in a secure fashion page it would be possible in a select box, though maybe that page is out of date.

RTBC seems right.

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

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

  
  FILE: ...ll/modules/pareview_temp/test_candidate/uc_attribute_per_role.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   78 | WARNING | Line exceeds 80 characters; contains 96 characters
  --------------------------------------------------------------------------------
  
  
  FILE: .../all/modules/pareview_temp/test_candidate/uc_attribute_per_role.install
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   25 | WARNING | Format should be * Implements hook_foo().
  --------------------------------------------------------------------------------
  

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

  ./CHANGELOG.txt ./uc_attribute_per_role.info ./uc_attribute_per_role.module ./README.txt ./uc_attribute_per_role.install ./uc_attribute_per_role.admin.inc
  

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.

But that are just minor issues, so ...

Thanks for your contribution, firewing1! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.