Filefield Role Limit

Hi!

I just tested your module and noticed a bug: although I set a maximum file size for "authenticated" role, I could upload a file with a greater size.

Here's how you can recreate the bug:
1- I created a user with role "authenticated"
2- I created a content type and added a new field as you suggested in README.txt
3- The field settings are as follow:
- Permitted upload file extensions: pdf
- I didn't configure 'Maximum upload size per file', but configured 'Maximum upload size per file per role' and set it to '10K' for authenticated users.

The file I uploaded has 1.5M.

Even when I set 'Maximum upload size per file' (2M) and 'Maximum upload size per file per role' (10K), I'm still able to upload my file (1.5M).

You can see attached files.

Good work on this module. I've test it it and it works pretty darn well. A few notes from my review:

1) I was able to run the PAReview.sh script on your work, and it passes without issue.

2) There are a few places in your code where you have commented out code sections. I think the convention is to avoid this type of stuff...comments should be for just that...comments. :)

3) It's great that you provide some information to the user when they install the module via drush. The text is escaped funkily though. it's outputted with some HTML entity encoding:
FileField Role Limit settings are available for FileField fields under Administer > Content management > Content types or Administer > Content management > Content types > Fields.

4) Lastly, I think the descriptions for the text fields that take the maxmimum upload sizes could be a little overwhelming, and they all say pretty much the same thing. Imagine a site with 10 or 20 roles, that page could become a little crazy. Maybe there's some way to explain what should be put into each textfield more generally? Core does seem to have its explanation twice, but having it 40 times might be a bit much.

Other than that it looks pretty great to me! I could definitely see a use case for this in my work.

Just being kinda nitpicky design-wise, but let's say I want to use this on my live site. My live site has 18 roles currently, fieldset is going to be HUGE with that many roles. Why not just switch to an addition style where you choose the role in a drop down and have a textbox next to it, and clicking Add actually adds that role/limit check instead of having it save blanks initially?

Just a thought...now on to manual review.

- You still have some commented out code (lines 106, 108, 122) in your .module file.
- You have a few code lines that are LOONNNGGG. Try spanning them across multiple lines (lines 53, 60, 255, 276)
- You have quite a few occurrences of "[some boolean] === TRUE", this is unnecessary. For example line 35 has

if (empty($roles) === FALSE) { // Will equate to TRUE when $roles is not empty
}
// Simplified form
if (!empty($roles)) { // NOT empty($roles)
}

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: ...es/all/modules/pareview_temp/test_candidate/filefield_role_limit.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   73 | ERROR | BREAK statements must be followed by a single blank line
  --------------------------------------------------------------------------------

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.

manaul review:

• "$message = st("The %name module installed successfully.",": %name is not a dynamic value, so no placeholder needed. You can just add the emphasize tags in the string if you want them.
• filefield_role_limit_install(): why do you need to increase the weight of your module? please add a comment about that.
• filefield_role_limit_widget_settings_alter(): do not pass variables to t(), use literal strings where possible. Otherwise translation tools cannot extract the text from the source code. Also elsewhere.

Otherwise I think this is nearly ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

• filefield_role_limit_install(): filter_xss() is not needed, no user provided input involved.
• filefield_role_limit_widget_settings_alter(): $perfile_description does not have to be an array, use ".=" to append to an existing string. Same for $pernode_description, then you also don't need the implode().

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

Forgot to remove the tag.

Added a new review

Please don't assign the issue to your self, only the current reviewer should do this.

Assigning to tim.plunkett, as he said he might have time to review this.

The module looks great, 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.