[D7] Mail Safety

Your branch name should be : 7.x-1.x.

Branch 7.x-1.0 (should be 7.x-1.x)

It appears you are working in the "7.x-1.0" 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.

You should set a default branch http://drupal.org/node/1659588

  // Let other modules change the table structure to add or remove
  // information to be shown. e.g. attachments that need to be downloaded.
  $modules = module_implements('mail_safety_pre_view');

  foreach ($modules as $module) {
    $table_structure = module_invoke($module, 'mail_safety_pre_view', $table_structure);
  }

So you should use drupal_alter().

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.

I have used the sandbox version in a project that heavily relied on sending emails. It's a very useful module that does everything that you need when debugging or just generally watching which emails are sent.

I have done a code review and other than the already fixed problems I can't find anything, as far as I'm concerned, it's reviewed and tested by the community and can be published as a full project.

Agree with michielnugter.
look RTBC for me.

Made changes to the branch.

manual review:

1. mail_safety.api.php: the function bodies should have better examples.
2. mail_safety_permission(): Both permissions need to have the 'restrict access' => TRUE flag enabled, because you can easily take over a site if you have access to all mails (password reset mails for admins and such). This is a security blocker, sorry. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. theme_mail_safety_mail(): There are different opinions whether mail content should be escaped against XSS or not, but I think you should still add the appropriate sanitization functions here just to be safe (since you are displaying the mail in a HTML context). See also http://drupal.org/node/28984 Same for theme_mail_safety_details().
4. mail_safety_enable(): don't juggle with module weights, that is unreliable and prone to errors. Use hook_module_implements_alter() instead if you want to influence the order of a specific hook.

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

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

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

I couple more similar modules - https://drupal.org/project/null_mailer and http://drupal.org/project/maillog.

• In mail_safety_load() this code could be more efficient: foreach ($mails as $key => $mail) { foreach ($modules as $module) {...}}. It's unlikely to be a real problem unless there's tons of mails and more than 1 module implementing the hook, just something to be aware of.
• There seems to be a fair amount of code duplication between mail_safety_admin_send_original_form_submit() and mail_safety_admin_send_default_form_submit(), you could probably abstract out the common portions.
• I think you could use a confirm_form in mail_safety_admin_delete_form().

Those are not blocking issues though, the module looks nice!

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

manual review:

1. hook_mail_safety_table_structure_alter(): do not call theme() here, just return a render array in the table. Drupal will render the whole thing later for you. Same in mail_safety_admin_dashboard_form().
2. "'empty' => 'No mails found',": all user facing text must run through t() for translation. Please check all your strings.
3. "t('Are you sure you want to send "!subject" to !to?', array('!subject' => $mail['mail']['subject'], '!to' => $mail['mail']['to']))": this looks a bit vulnerable to XSS exploits. The subject could be user provided text, so it should be sanitized before printing. Use the "@" or "%" placeholder with t() to perform sanitization. Please read https://drupal.org/node/28984 again. Also elsewhere.
4. mail_safety_insert_mail(): This looks like an alter hook, so you should use drupal_alter(), right?
5. mail_safety_admin_delete_form(): you could use confirm_form() here.

So not sanitizing the subject is a blocker right now, but otherwise this looks almost ready.

Looks good to me now and since this was RTBC already:

Thanks for your contribution, barthje!

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.