classified

Hi bhosmer,

I installed the module and it seems to work as expected. Your module code seems good to me when I go through it, but a manual review by a professional reviewer is needed as well. Just wait a little...
I ran codesniffer and found no errors, but you already did that too.

Please rename README.md to README.txt and perhaps format the README.txt file a little. Here are a few examples of README files: http://drupal.org/node/447604

To make your project more sexy, format the project page in sections, as e.g. mentioned here: http://drupal.org/node/997024. Perhaps a demo page could be useful too.

I might have a tip. Right now the banners are stuck to the top and bottom of the page. So if you scroll the page, the banners scroll as well and you might end up with no banners on a certain part of the page. Perhaps an option to make the banners stick to be always visible could be a nice addition, similar to sticky table headers. Unfortunately I can't tell you how to do that right now. See for yourself if you find this useful or not. But I would like to have that option if I would use your module.

cheers,

Joost

Stickiness will definately involve javascript, so indeed look in that direction.

Wrong issue referenced.

You could do that, but keep in mind that hook_footer() doesn't exist in D7, but you know that I think.

Very good! Using CSS it should work fine (just googled some stuff, hehe). I'm also interested in sticky stuff.
If other reviewers find stickiness not important, you should get approval soon with your bonus. Good luck.

Aaah, yes man, this is very sharp.
I also noticed you have made a custom level, which is what I was going to ask for. :)
If you can commit this dev code to the 6.x-1.x branch and fix the CodeSniffer errors below, I will set it to RTBC.

FILE: .../drupal6/sites/all/modules/contrib/classified/classified.module
--------------------------------------------------------------------------------
FOUND 18 ERROR(S) AFFECTING 16 LINE(S)
--------------------------------------------------------------------------------
  62 | ERROR | Missing function doc comment
  74 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
 123 | ERROR | Array indentation error, expected 4 spaces but found 6
 124 | ERROR | Array indentation error, expected 4 spaces but found 6
 125 | ERROR | Array closing indentation error, expected 2 spaces but found 6
 133 | ERROR | Line indented incorrectly; expected 6 spaces, found 4
 134 | ERROR | Line indented incorrectly; expected 8 spaces, found 4
 135 | ERROR | BREAK statements must be followed by a single blank line
 135 | ERROR | break statement indented incorrectly; expected 6 spaces, found 4
 136 | ERROR | Line indented incorrectly; expected 6 spaces, found 4
 137 | ERROR | Line indented incorrectly; expected 8 spaces, found 4
 138 | ERROR | BREAK statements must be followed by a single blank line
 138 | ERROR | break statement indented incorrectly; expected 6 spaces, found 4
 139 | ERROR | Line indented incorrectly; expected 6 spaces, found 4
 140 | ERROR | Line indented incorrectly; expected 8 spaces, found 4
 141 | ERROR | break statement indented incorrectly; expected 6 spaces, found 4
 147 | ERROR | No space before comment text; expected "// $output .= '<div
     |       | id="classified-bottom">' . $_output_text . '</div>';" but found
     |       | "//$output .= '<div id="classified-bottom">' . $_output_text .
     |       | '</div>';"
 149 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

FILE: .../drupal6/sites/all/modules/contrib/classified/classified.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  6 | ERROR | File doc comments must be followed by a blank line.
 11 | ERROR | Do not use t() or st() in installation phase hooks, use $t =
    |       | get_t() to retrieve the appropriate localization function name
--------------------------------------------------------------------------------

Sorry, wrong tag. ;)

Almost there my friend.

In classified.install you have:

function classified_install() {
  $t = get_t();
  $translated = $t('The classified module was successfully uninstalled.');
  drupal_set_message($message = $t, $type = 'status');
  variable_set('classified_level', '0');
  variable_set('custom_classification_color', '0');
  variable_set('custom_classification_text', 'Classification Level Not Specified');
}<

Change this to:

function classified_install() {
  $t = get_t();
  $translated = $t('The classified module was successfully installed.');
  drupal_set_message($message = $translated, $type = 'status');
  variable_set('classified_level', '0');
  variable_set('custom_classification_color', '0');
  variable_set('custom_classification_text', 'Classification Level Not Specified');
}<

So change the typo 'uninstalled' to 'installed' and change the $t to $translated in drupal_set_message().

You should fix these issues, but since these are minor I'm setting this to RTBC.

cheers,

Joost

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 6.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. You have to get a review bonus to get a review from me.

manual review:

1. Please remove the 6.x and master branch, they are confusing.
2. classified_install(): no need to set variables upon installation, as you can use default values with variable_get() anyway.
3. classified_enable(): does not do anything useful? Please remove it. And I wonder if all the messages in the install file are really needed?
4. classified_perm(): the permission is never used.
5. It was not clear to me that the banner is used on every page on the site, I thought there would be some mapping per path or similar. Maybe make that more clear on the project page that this effect is global?
6. "variable_set('classified_level', check_plain($token));": "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database". Please read http://drupal.org/node/28984 again.
7. "$_output_text = 'Unclassified';": all user facing text must run through t() for translation.
8. classified_footer(): this is vulnerable to XSS exploits, you need to sanitize user provided text before printing. This is not a security issue as the "administer site configuration" permission is considered as site owning permission anyway. See also http://drupal.org/security-advisory-policy
9. classified.js: so this file is not needed anymore? Please remove it then.

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

Hey bhosmer, klausi will most likely not view this thread unless you get a bonus and set this thread to needs review afterwards.
I wish I could answer your question, but it also confuses me.

classified_footer(): this is vulnerable to XSS exploits, you need to sanitize user provided text before printing. This is not a security issue as the "administer site configuration" permission is considered as site owning permission anyway. See also http://drupal.org/security-advisory-policy

This means that in classified_footer() you are directly outputting variables that were entered into a settings form. You could craft input that would add arbitrary JS to the page.

Since your settings path has "administer site configuration" for the access permission, this isn't a true security problem since "administer site configuration" can do pretty much anything to the site anyway. If you ever add your own explicit permission for this path, then you could potentially have a problem.

As a precaution and best practice, you should sanitize everything a user inputs (as needed) via the UI when you render page output.

Looking at the 6.x-1.x that I just pulled.

In classified_footer(), custom_classification_color gets used directly. Even though you set this with a radio, it may be possible to bypass this with a custom POST, and then you use it directly. It is also a good idea to future proof a change, so a change in one place doesn't require a change someplace else. This is being paranoid, though. I think klausi was talking about custom_classification_text, which you did handle.

It is also a good idea to use the same prefix for all your variables, which is the same as your module name. This prevents any namespace collision. Don't forget to also update the hook_uninstall if you change them.

BTW, the conditional on lime 131 in classified.module will always evaluate as TRUE with the way it is written.

I think on L131 of classified.module he meant to have:

  elseif ($_level == '0' or $_level == '1' or $_level == '2') {

And shouldn't the levels be stored as numeric values not strings.

To be consistent the the case statements should be

case '0'
case '1'
etc.

Review of classified.

Description.

The description is a bit vague. OK - So it puts colored banners at the top and bottom - but how does it know which colors to apply?

Do you select the color by using tags - or is it a field you add to your content types. Or is it something which is tied to your user level/role and is there permanently after you have logged in? Explaining this at the description level would make users approaching this module a bit clearer on *how* the module operates.

And what is the overall purpose of the module - is it so that the user is easily aware of the classification level of the content they are viewing - or is it so that supervisors can easily see the level of documents which are being viewed by their staff - i.e. they can look around the room to see what level of stuff people are looking at.

Coding standards.

It's important to make sure that the module passes all coding standard tests on ventral.org. If there is no output then we're OK - if there is any output then it needs to be looked into - which is time consuming. There is no reason you shouldn't be able to sort out the coding standards issues - they are fairly easy to resolve.

Here is the report - http://ventral.org/node/94256/revisions/109792/view

The errors are really basic - there should be the correct number of spaces used for indentation - and spaces should be used not tabs. Listing of CSS properties in alphabetical order is important when you have CSS with large numbers of attributes set. If it helps - there is a really good Drupal mode for Emacs - I've blogged about it here - http://www.freewayprojects.com/2011/12/for-all-drupal-devlopers-who-use-...

Once you see how the indentation should be it becomes second nature - for details about the coding standards look at:

http://drupal.org/coding-standards
http://drupal.org/node/302199

Branches

The branches are not correct - currently they are:

kbailey@dev3:~/projects/icmsv4/branches/classified/sites/all/modules/classified> git branch -a
* 6.x-1.x
  origin/6.x-1.x
  origin/7.x
  origin/HEAD
kbailey@dev3:~/projects/icmsv4/branches/classified/sites/all/modules/classified>

You should rename the 7.x branch to 7.x-1.x

classified.install

function classified_install() {
  $t = get_t();
  $translated = $t('The classified module was successfully installed.');
  drupal_set_message($message = $translated, $type = 'status');
}

A comment on '$t = get_t();' would be nice to explain why it is there.

You call a function without the '$message = ' and '$type = ' parts.

drupal_set_message($message = $translated, $type = 'status');

should be

drupal_set_message($translated, 'status');

Similarly

drupal_set_message($message = t('The classified module was successfully disabled.'), $type = 'status');

should be

drupal_set_message(t('The classified module was successfully disabled.'), 'status');

classified.module

function classified_footer() has no comments. It's fairly easy to see what is there but I'm a bit concerned that you seem to be forgetting comments altogether.

README.txt

The installation and administration instructions are confusing.

'Enable it' should be more detailed - 'Enable the module at Administer - Site building - Modules (admin/build/modules)'

You've then mentioned enabling it twice.

When you get to admin/settings/classified the currently selected options should be filled in already - currently the radio buttons are all empty. Once you've saved something they are soved correctly and re-presented.

Reset to defaults empties out the radio buttons - again, they should be set to the default values.

Other than these points the module seems to work without errors.

My only concern is that the module is a little small to be really allowed to set you up as a Drupal contributor - but one of the other guys will decide that issue. Also, some of the coding shows that you are new to writing code - I would suggest some decent PHP books or a course to get up to speed.

A couple of points to note:

* Comments are a crucial part of proper code - not some sort of luxury add-on.
* Calling functions is different from declaring them - check out http://www.php.net/manual/en/functions.arguments.php#functions.arguments...
* It's not normally necessary to put underscores in from of variable names - variables are only available in the scope of the function they are sitting in. Underscores are normally used for private methods in object declarations. See: http://php.net/manual/en/language.variables.scope.php

Don't want to sound too negative - module is small and does do its job. The job is a little limited - but I think it is filling a requirement so that is always good!

manual review:

1. the CSS file is still violating your coding standards, see the automated review in comment #20 and http://drupal.org/node/302199
2. "variable_del('custom_classification_color');": all variables that your module defines must be prefixed with your module's name to avoid name collisions.
3. "'#default_value' => variable_get(check_plain('custom_classification_text'), NULL),": #default_value gets automatically sanitized by the Form API, using check_plain() here means double escaping which is bad. Please read http://drupal.org/node/28984 again.

Settings this back to "needs work", you need to know where to use check_plain() and where not. I think you are pretty close otherwise, fix those issues and you should be ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Please add all your reviews to the issue summary.

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

Hi bhosmer, when there are no issues from other reviewers for more than a week, klausi will set it to fixed.

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

Thanks for your contribution, bhosmer!

I updated your account to let you 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 get 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.

Congrats bhosmer! Good feeling right? :-)