Bean Tax

Review of the 7.x-1.x-dev 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/bean_tax.module:
   +2: [minor] There should be no trailing spaces
   +3: [minor] There should be no trailing spaces
   +7: [minor] There should be no trailing spaces
   +8: [minor] Format should be * Implements hook_foo().
   +8: [minor] There should be no trailing spaces
   +9: [minor] There should be no trailing spaces
   +10: [minor] There should be no trailing spaces
   +11: [minor] There should be no trailing spaces
   +13: [minor] There should be no trailing spaces
   +87: [minor] Use an indent of 2 spaces, with no tabs
  
  Status Messages:
   Coder found 1 projects, 1 files, 10 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.
• "plugins/bean/bean_tax.relevant.inc" in bean_tax.info: It's only necessary to declare files[] if they declare a class or interface.
• "bean_tax.theme.inc" in bean_tax.info: It's only necessary to declare files[] if they declare a class or interface.
• ./bean_tax.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function get_node_terms($node) {
  function cmp($a, $b) {
  

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

  ./bean_tax.module
  

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.

Your not working correctly with branch names, so you'll not be able to create a release. For additional resources please see the documentation about release naming conventions.

Your README.txt is not very informative, what about installation / configuration instructions? you got a plugin system but no documentation about it?

Your .install is empty, remove stuff you don't need.

The bean_tax.relevant.inc plugin got lots of lines but only a few comments. How should we understand what you're doing here? Please tell us your thoughts by commenting. Don't be afraid, there is no "too much" commenting - if there is something to say, say it ;-)

Further reviews will follow after you fixed these issues and enhanced your code documentation.

regards

Hi brantwynn,
Actually, your branch name is wrong.
As you can see in the documentation about release naming conventions, the branch name should be "7.x-1.x" (and you can add "7.x-1.x-dev release" to the branch description.
I know it's a little confusing, i made the same mistake sooner.

You can check your naming convention by testing your code here: http://ventral.org/pareview/httpgitdrupalorgsandboxbrantwynn1376658git

There's also a warning with your README.txt:

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 2 | WARNING | Line exceeds 80 characters; contains 88 characters
--------------------------------------------------------------------------------

Regards

Sorry for the long delay!

1. Please take a moment to make your project page follow tips for a great project page.
2. Always use TRUE and FALSE for booleans (eg. '#tree' => 1,)
3. Isn't it possible to work with drupal_add_js setting option instead of doing this raw stuff for the cycle settings?
4. Prefix all your constants with the full module name
5. drupal_get_path('module', 'bean_tax') you already know where you are, dirname(__FILE__) would do the same job with less work ;)

That's all what catched my eyes, as I got no idea about the bean module and it's proper usage, so have to stop here.

We do really need more hands in the application queue and highly recommend to get a review bonus so we will(/can) come back to your application sooner.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws). Also please add your reviews to the issue summary.

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

• Drupal Code Sniffer has found some issues with your code (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. Get a review bonus and we will come back to your application sooner.

manual review:

1. bean_tax_setting_is_numeric(): if you are not using any HTML in variables for t() you should use the "@" placeholder per default instead of "!".
2. "if (!$records_shown = $bean->settings['records_shown']) {": you should use !isset() instead, no? Otherwise you will get PHP notices if bean->settings['records_shown'] is not set.
3. TaxRelatedBean::view(): $node is not defined, so isset($node) will always be FALSE.
4. "$content['#markup'] = 'No Results';": all user facing text must run through t() for translation.

But that are just minor issues, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Updated for current implementation that does not include cycle.js

I don't know if this is an issue of your module or of beans: I can put whatever kind of html into the block title.
As only administrative users can edit these block titles it's not a real security issue, however it's an unnecessary risk (IMHO).

Thanks for your contribution and 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.

Thanks to the dedicated reviewer(s) as well.