[D7] S3 File System

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxcoredumperror2097947git

I'm a robot and this is an automated message from Project Applications Scraper.

Hi,

this looks mighty impressive. :)
It's pretty hard to test, given I have no access to S3. pareview.sh has a lot to complain about. Mostly "Whitespace found at end of line" which seems picky, but has a reason when people later write patches for your module, as those whitespaces will cause noise in patches.

Also I see you wrote your own tests. I guess it's fine, as you also state a good reason, but as pareview points out, it is a really good idea, to prefix functions with your module name, else a collision in function names is trouble waiting to happen.

Or maybe remove the tests directory entirely. At least on drupal.org.

Maybe remove the entire tests directory? It comes down to the question, if the test file will be helpful to anybody else. If you would assume so, then leave it in.

About that whitespace/patch thing. There is an explaination somewhere, but I can't find it. What can happen is, that my editor removes these whitespaces automatically, while they are still in. If I then create a small one line patch, it would become huge, because alle the removed whitespaces would show up as changes in the patch file.

Don't get me wrong: even some core modules do not respect this completely, but it's a nice guesture...

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

We've been following this module for a while. It is a fork that was created in response to performance issues with the original Amazon S3 module and their decision not to make the changes that this module makes.

This module radically improves performance. We found the original essentially unusable in our setup.

We are currently using this in production.

Added "Other considerations"

Yeah!

Have to +1 on this. I've swapped over to this module on couple of production sites, and it's working like a charm. Original was pretty much useless in my use case, mainly because of the speed, or lack thereof.

To full project!

Added a second review.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/S3StreamWrapper.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   11 | WARNING | Class name must be prefixed with the project name "s3fs"
      |         | (omitting underscores)
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/s3fs.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   510 | WARNING | Class name must be prefixed with the project name "s3fs"
       |         | (omitting underscores)
  --------------------------------------------------------------------------------
  

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. I'm a bit disappointed that collaboration with the existing amzons3 project failed, could you add more details to the project page in what regard the modules differ?
2. Having white spaces at the end of lines is bad because people have their editors setup to automatically remove them, so if they want to write patches against your code they will always contain unrelated white spaces changes. Please reconsider removing them. There are also some other things in the automated report that should be fixed.
3. Yes, you can ignore warnings about method names that you cannot change.
4. s3fs_requirements(): the TODO comment does not make sense, since you already depend on the awssdk module, right?
5. "variable_set('s3fs_hostname', filter_xss($form_state['values']['s3fs_hostname']));": do not use filter_xss() here: "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." - see https://drupal.org/node/28984
6. s3fs_refresh_cache_submit(): same here, do not filter on input, only when you are printing stuff to HTML.
7. "class AWSException": that class needs to be prefixed with you module's name.
8. class S3StreamWrapper: wrong doc format of the class properties, see https://drupal.org/node/1354#var
9. "db_query("SELECT * FROM {s3fs_file} WHERE uri LIKE :folder", array(':folder' => "$uri%"));": this should use the db_like() helper function.

Otherwise looks pretty good. The wrong usage of filter_xss() is a blocker right now, you need to know when to sanitize text and when not. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Added third review for review bonus

• You have some non-whitespace errors reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxcoredumperror2097947git.
• Regarding the whitespace issue, that is part of our coding standard, see https://drupal.org/coding-standards#indenting. So the problem is with you and your collaborators' editor settings, and you should all switch over :) It is a big pain though, so it's not a blocking issue for this project. If you'd like to comment on Drupal's code standards, the review group is here: https://groups.drupal.org/coding-standards.
• Where are you using S3FS_VERSION? It would be easier to maintain to read that value from the system table in the info column.
• I'm not sure about the access TRUE for s3/files/styles/%image_style. Drupal after 7.20 now uses tokens to ensure that image style requests are legitimate, otherwise this can be considered a vector for denial of service attack.

Setting to needs work for the security issue, otherwise looks great!

----
Top Shelf Modules - Crafted, Curated, Contributed.

@coredumperror - sorry you feel this process has involved stonewalling you, that's certainly not our intent. There's a lively discussion about this going on here https://groups.drupal.org/node/374478 in the code-review group. It is true that we're a lot stricter in the project application queue! I was trying to say you don't have to make the whitespace change, but thanks for doing so anyway. Our list of issues to check on each application is available in that group and here http://pareview.sh/pachecklist.

Your function s3fs_image_style_deliver() is largely the same as Image module's, so you're right, this is not a security issue.

----
Top Shelf Modules - Crafted, Curated, Contributed.

manual review:

• Class names should start upper case, so the module prefix should also start upper case. I committed a fix to DrupalPractice sniffer to suggest a correctly camel case prefix in the first place.

But otherwise looks good to me, so ...

Thanks for your contribution, coredumperror!

I updated your account so you can 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 stay 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.