FileField Mail D7

filefield_mail.info
-dependencies[] = file- added twice

filefield_mail.admin.inc
for type textarea attribs;
remove '#size' => 80,
enter #cols + #rows

Par review shows no errors, good stuffs!

http://ventral.org/pareview/httpgitdrupalorgsandboxmartinmayer1877466git

Manual Review:

Hmmm, I wonder about this code in fielfield_mail.com

  if (empty($record)) {
    $record = array(
      'fid' => $fid,
      'email' => $email,
      'changed' => time(),
    );
    drupal_write_record('filefield_mail', $record);
  }

Is it me... or are you taking direct user input ($email being $email = $form_state['values']['email'];) and inserting it directly into the database via drupal_write_record() ...

I tried to google if Drupal write record automatically sanitizes it's input, but couldn't find anything on it. Maybe just double check or consider it? Maybe you can sanitize it with t()? Anyways, just a thought.

No further thoughts or issues spotted by me (but I'm a noob!)

How it actually worked when installed:

Directions were fine. Went ahead and installed, configured, setup content type, uploaded image. Downloaded logged in fine (skipped straight to download). Logged out and went to download as an anonymous user. Two things:

1: I got an email, but the link provided was bunk: http://dev.jessenicola.com/module-testing//Jellyfish.jpg , Should be sites/default/files/Jellyfish.jpg

2: Should you maybe check anonymous download email addresses against user emails and perhaps just tell them to login to download the file?

Anyways, it didn't work as promised despite nothing blatantly obvious being wrong in the code. Marking as "needs work"

Hi Martin,

very nice job, thank you very much for your job and contribution to the community.

Just a couple of comments.

The code itself is quite nice, well written and it is easily readable. I just would improve it adding a couple of comments following the Doxygen and comment formatting conventions http://drupal.org/node/1354

Executing paraview tool i also get this issues:

filefield_mail.inc

Line 66: missing space after comma [style_comma_spacing]
  $email = filter_xss($form_state['values']['email'],$allowed_tags = array());
Line 66: No space found after comma in function call [sniffer_generic_functions_functioncallargumentspacing_nospaceaftercomma]
  $email = filter_xss($form_state['values']['email'],$allowed_tags = array());
Line 83: The $string argument to t() should not begin or end with a space. (Drupal Docs) [i18n_11]
  drupal_set_message(t('Thank you! You will shortly receive an e-mail with your personal download link. '), 'status', TRUE);
Line 93: Use ANSI standard <> instead of != [sql_noteq]
  $result = db_query('SELECT * FROM {filefield_mail} WHERE ((uid = 0 AND email = :email) OR (uid != 0 AND uid = :uid)) AND fid = :fid', array(

Thank you very much again.

No problem Martin, thank you very much for your effort.

In what it concerns myself, I am happy about the review and tested status :-). Let me know if it is everything ok, it is my first time :-)

I cloned the repository, the directory name is filefield_mai, a small typo "l" is missing. It should be filefield_mail

Just pulled the module again and test. It works for me :)

manual review:

1. filefield_mail_stats(): why do you need the foreach loop? Can't you use ->fetchAll() or similar methods on the result? Please add a comment.
2. theme_filefield_mail_link(): no need to use check_plain() here, the l() function will sanitize attributes for you. And double escaping is bad.
3. filefield_mail_download_form_submit(): why do you call filter_xss() here? The email can never be anything malicious, since you checked that already in the validation function? Also the email is never printed to the user in this function? "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" http://drupal.org/node/28984
4. "filter_xss($mail_subject, $allowed_tags = array());": just pass array() here? Why the $allowed_tags variable?
5. filefield_mail_send_mail(): You should not pass the finished message to drupal_mail(), only the necessary variables. Markup and layout should all be done in filefield_mail_mail(). Make sure to read the documentation of drupal_mail() again.
6. "'Pragma' => 'hack',": what is that needed for? Please add a comment.
7. filefield_mail_send_mail(): IMO there is no need to run sanitization functions like filter_xss() on emails. I started a discussion to finally clarify this: http://groups.drupal.org/node/280368

Although you should definitively fix those issues, they are not critical application blockers, so ...

Thanks for your contribution, Martin Mayer!

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.

good job Martin :-)