[D6] Featurelist

Could you push the module's code to git? Currently your git repository only contains the .info file, and that's rather hard to review by itself.

Once you have the code up, just set the status on this issue to "needs review" and someone will come along and take a look.

Okay, some comments on your module:

• You don't need to include LICENSE.txt; the drupal.org packaging scripts add it automatically when you're creating a release for download.
• Your README.txt should wrap lines at 80 characters as noted in the module documentation guidelines.
• You don't need $Id$ lines in your files, no matter what the Coder module tells you; it's not necessary since Drupal.org uses Git instead of CVS.
• Your .module file should have a @file block explaining how it works and what it does.
• In featurelist_admin_settings, every #title and #description field must be run through t(). Currently it seems about half are. Check all your other functions as well.
• In featurelist_admin_settings, the featurelist_icon_default field is built with an $options array that is created by simply substituting the paths into HTML image tags. If a path contains any HTML special characters, this will mess up the formatting -- and if someone malicious can manipulate the paths, it's a security issue. Use check_plain to properly escape any HTML entities. Also, check any other location that you output HTML (like theme_featurelist_view) and ensure you escape whatever should be escaped.

Apart from these issues, the module looks fairly good. Just give your HTML output a thorough look-over to be sure there's no way that unwanted HTML could creep in. Once these issues are fixed, set the status here back to "needs review" and someone will take another look through.

Looks good. I'm still a bit nervous about featurelist_field() and how it builds CSS classes and HTML, and I hope you've tested it thoroughly to be sure unwanted HTML can't get through. Otherwise, my only comment is that when you use the ternary operator ($condition ? $something : $something_else) there should be spaces around the ? and : operators.

I'm fairly certain that #default_value and #value are automatically passed through check_plain() for you, so you don't need to do it yourself. The documentation is somewhat unclear, but I checked the code and it should be done automatically.

Otherwise the module looks good. Thanks for following up quickly.

One of the Git administrators will have to review the request and grant you permission. There's a rather large backlog at the moment, so I'm not sure when that will happen.

There are large sections of code that is commented out, and other functions that are only called by the commented out code.
For example, when is featurelist_import_csv() used? Or featurelist_delete_table_values()?

Also, in the case of featurelist_field(), that runs on every field? Is it supposed to? I expected to see a check for the field type. Or documentation explaining why such a check isn't needed.

Also, there are some coding standards issues. Here's a patch to get you started, but I added a couple @TODOs as well.

Coder found 1 projects, 2 files, 1 normal warnings, 5 minor warnings. I noticed some very small code style issues. Please run the coder module on "minor" setting to help catch these. The coding standards have even more information in this area.

Your featurelist_install and featurelist_enable are the same - is that really appropriate? It seems odd to me.

This will end up escaping the data multiple times and is unecessary. To see the problem create a path with an ampersand in it and then save the page over and over and over. It will become &

79 '#type' => 'textfield',
80 '#title' => t('Icon path "Checked"'),
81 '#default_value' => check_plain($featurelist_icon_path_y),

check_plain is not appropriate to use directly for urls.

Instead of:
check_plain(base_path() . $path_to_module) . '/icons/ico_check_green.png',
You should do something more like:
url($path_to_module . '/icons/ico_check_green.png'),

Similarly, instead of:
<img src="' . check_plain($featurelist_icon_path_y) . '" alt="' . t('Feature available') . '" />',
you can use theme_image which provides protection against malicious URLS and allows for overriding via a theme function.

It's not common to include comments like "#JP111210" - I suggest removing those or replacing them with explanations of what is happening in the code.

Most of the criticism is based on things from the tablefield module.

Yes and no...the process has changed a bit, but also once someone can create a project they can do whatever they want inside of it. This process is meant to help new maintainers learn the process very well so they are more likely to create high quality modules in the future.

Thanks for sticking with this project! It seems like a useful feature.

Hi Anybody - no need to apologize. I'm glad this is a learning experience for you - I look forward to reviewing the module after your changes.

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.
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/featurelist.module:
   +23: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +405: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +481: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +493: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +597: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 2 files, 4 normal warnings, 1 minor warnings, 0 warnings were flagged to be ignored
  

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

          // Multivalue fields will have one row in the db, so make sure that it isn't empty.
    // @todo: Preparing draggable rows, unsure if that will work (without callback).
  

• There should be a space before and after operators like ==, ===, && and ||. See http://drupal.org/node/318#controlstruct
  

  featurelist.module:228:                    . ($rows_abs==$row_key+2) ? ' row-last' : '';
  featurelist.module:462:    $is_titlerow = $i==0;
  featurelist.module:468:      $is_titlecol = $ii==0;
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./featurelist.css:                                        ASCII C program text, with CRLF line terminators
  ./README.txt:                                             ASCII English text, with CRLF line terminators
  

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

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

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./featurelist.module:20:  $output = ''; // declare your output variable.
  

• ./featurelist.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  300- *  HTML The featurelists HTML formatted output.
  556- *  $featurelist Array representing the table.
  558- *  Cleaned up table array.
  591- *  The icons key (char-code).
  593- *  HTML icon code.
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./featurelist.css:                                        UTF-8 Unicode (with BOM) C program text, with CRLF line terminators
  ./README.txt:                                             UTF-8 Unicode (with BOM) English text, with CRLF line terminators
  README.txt
  

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

manual review:

• "'access arguments' => array('administer site configuration'),": this permission is too generic, please provide your own permission.
• "'#columns' => array('value'), '#delta' => 0,": the #delta item should be on its own line.

Otherwise this looks nearly ready to me.

Hi, some few issues remain:

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

• ./featurelist.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  600- * 	 The icons key (char-code).
  

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

  ./featurelist.css ./README.txt ./featurelist.install ./featurelist.module ./featurelist.info
  

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.

HTH, dave

Hi,

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

• ./featurelist.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  600- * 	 The icons key (char-code).
  

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

  ./featurelist.css ./README.txt ./featurelist.install ./featurelist.module ./featurelist.info
  

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.

Manual:
Just checked your recent changes... I think there is still some confusion with the doxygen comment style; please take another look at http://drupal.org/node/1354#functions

IMO you missed out some empty lines between @param and @return. It's a bit tricky. (The param type is not yet required, but of course it is appreciated that you provide it though.) :)

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

• Drupal Code Sniffer has found some code style issues (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.

FILE: ...pareview/sites/all/modules/pareview_temp/test_candidate/featurelist.css
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  5 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
  5 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 98 | ERROR | Line indented incorrectly; expected 2 spaces, found 0
--------------------------------------------------------------------------------


FILE: ...view/sites/all/modules/pareview_temp/test_candidate/featurelist.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 11 | ERROR | You must use "/**" style comments for a function comment
--------------------------------------------------------------------------------


FILE: ...eview/sites/all/modules/pareview_temp/test_candidate/featurelist.module
--------------------------------------------------------------------------------
FOUND 24 ERROR(S) AND 3 WARNING(S) AFFECTING 24 LINE(S)
--------------------------------------------------------------------------------
  11 | ERROR   | Missing parameter type at position 1
  13 | ERROR   | Missing parameter type at position 2
  16 | ERROR   | Data type of return value is missing
  23 | ERROR   | Line indented incorrectly; expected 4 spaces, found 2
  24 | ERROR   | Line indented incorrectly; expected at least 6 spaces, found 4
  50 | WARNING | A comma should follow the last multiline array item. Found:
     |         | MENU_NORMAL_ITEM
 232 | ERROR   | An operator statement must be followed by a single space
 232 | ERROR   | There must be a single space before an operator statement
 232 | WARNING | A comma should follow the last multiline array item. Found: )
 236 | ERROR   | An operator statement must be followed by a single space
 236 | ERROR   | There must be a single space before an operator statement
 240 | ERROR   | Closing brace must be on a line by itself
 251 | ERROR   | Line indented incorrectly; expected 12 spaces, found 14
 254 | WARNING | A comma should follow the last multiline array item. Found: ]
 362 | ERROR   | Missing parameter type at position 1
 364 | ERROR   | Missing parameter type at position 2
 367 | ERROR   | Missing parameter type at position 3
 369 | ERROR   | Missing parameter type at position 4
 371 | ERROR   | Missing parameter type at position 5
 374 | ERROR   | Data type of return value is missing
 549 | ERROR   | Comments may not appear after statements.
 551 | ERROR   | Space found before comma in function call
 564 | ERROR   | Parameter comment must be on the next line at position 1
 578 | ERROR   | Line indented incorrectly; expected 4 spaces, found 6
 581 | ERROR   | Line indented incorrectly; expected 6 spaces, found 8
 588 | ERROR   | Line indented incorrectly; expected 6 spaces, found 8
 601 | ERROR   | Parameter comment indentation must be 2 additional spaces at
     |         | position 1
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Manual Review
The meat an potatoes of this module seem to happen at line 385.

/**
 * Process the featurelist.
 */
function featurelist_process($element, $edit, $form_status, $form)

That has got to be more that can be said about this function than "process the featurelist" at the very least all the input paramaters should be described.

I really look forward to a d7 release of this module. I could see many use cases.

Hello,
I have just used pareview and there are still errors:
http://ventral.org/pareview/httpgitdrupalorgsandboxanybody1106696git

I think the new error report is based on the improvement of the tools but the errors are older.
See http://drupal.org/node/318#controlstruct for "BREAK statements must be followed by a single blank line"

Greetings,
Carsten

Edit: I just installed the module in a test system and it seems that it works as suggested.

It's minor but re-running the automated test gave an issue:

FILE: ...eview/sites/all/modules/pareview_temp/test_candidate/featurelist.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
261 | ERROR | Line indented incorrectly; expected 14 spaces, found 16
--------------------------------------------------------------------------------

also minor: Theme function comments are not formatted exactly according to http://drupal.org/node/1354#themeable and the form constructor does not follow exactly http://drupal.org/node/1354#forms

Line 392: The comment should start with "Processes" instead of "Process" according to http://drupal.org/node/1354#functions

I'd correct the indentation on line 261 so nobody complains :)

Otherwise it looks good!

Hi Anybody.

It looks, to me, like you're repo is a bit outta whack.

Each branch I switch to looks to be empty.

You probably want to fix that, eh?

;-)

-Aaron

Hey @AaronELBorg again, it seems like there's something wrong with your git setup. The branches look file to me, see:

http://drupalcode.org/sandbox/Anybody/1106696.git/tree/refs/heads/6.x-1.x

and

http://drupalcode.org/sandbox/Anybody/1106696.git/tree/refs/heads/master

(Actually master is empty and it's supposed to habe a README.txt pointing to the branches)

Weird. I re-git-cloned the module and it worked.

I had to switch to the 6.x branch manually (it was on 'master' when I got it) but, yeah, not too sure what the deal was? Thanks for the heads-up, lucascaro.

To me, the code looks really good. My local install of code-sniffer didn't find any issues and my install of the module on my local box seems fine.

The only things I can report worth fixing are simple things like spelling/grammar mistakes and maybe a better explanation as to why someone might want to use this module?

On line 24 of featurelist.module the contents of the $output variable reads:

Featurelist provides a configurable table of feature status, for example you products or services features. You may check or negate a feature and set an optional text for each

Probably should be:

Featurelist provides a configurable table of feature status, for example your products or services features. You may check or negate a feature and set an optional text for each

Actually, that same problem happens on line 45 as well, fyi.

Line 138 is somewhat confusing:
"Select the default icon, which is preselected"
...but perhaps it's one of those situations where this kind of ackward help-text is unavoidable?

On line 175 of featurelist.module, in hook_field_info() the description's value is "Provides a configurable table of feature states". That seems a bit more grammatically correct to me than the stuff in hook_help() and hook_menu() ("Featurelist provides a configurable table of feature status, for example you products or services features".) Nothing major. Just putting it out there.

To be honest, Anybody's module looks pretty solid to me. It looks like he/she has taken the advice from everyone prior and followed it to the t. I have to admit that reviewing someone's module this late in the game usually gives me a good opportunity to learn more than offer advice but that's not a bad thing at all.

NIce work, Anybody.

I did a complete manuel code review and test.

The only one small issue i would have is:
How many data columns: and
How many data rows:
Should throw an errors when something else as numbers where enterd.

For me its a realy usefull modul for every body who have products in different version.

A minor note - you have and dependency on content - which is a part of cck, the dependency should be like:

dependencies[] = cck
dependencies[] = content
dependencies[] = tablefield

Else I think you our going to have problems with drush, because it would not find content and download it, it needs cck. You should also fix what greenrover33 says in comment #37.

Else, thanks for your contribution, Anybody! 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.