ManyMail

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

• README.txt is missing, see the guidelines for in-project documentation.
• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  
  manymail.info
  manymail.install
  manymail.module
  views/manymail_views.info
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./INSTALL.txt ./misc/scrolltop.js ./token/manymail_token.module ./views/includes/manymail_views.views_default.inc ./views/manymail_views.module ./CHANGELOG.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

See http://ventral.org/pareview/httpgitdrupalorgsandboxkristiaanvandeneynde1...

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• manymail.module: Remove the require_once statements at the top, Drupal will autoload the classes for you. Therefore you need to register them in the info file.
• manymail_send_mail(): "// Here be dragons." is not a useful comment why you ignore the exceptions, please document why you do not log them or process errors otherwise.
• manymail_menu(): do not misuse the "access administration pages" permission, create your own configuration permission.
• Your module file is quite long. You should move form and page callbacks to *.inc files.
• Remove the .gitignore file from the repository.
• manymail_send_role_form(): If I create a role with the name <script>alert('XSS');</script> will that trigger a nasty javascript popup on that form? If so, make sure to sanitize the contents of the $options array before using it on #checkboxes form elements. See also http://drupal.org/node/28984
• class ManyMailRecipientList: there are 3 recipients member variables. Really? Why do you need them, why is one variable not enough to hold a list of recipients?

MANUAL REVIEW
--------------

1. Why would someone want to use your module, instead of the Mime Mail module? http://drupal.org/project/mimemail

2. After installing the module, when I went to /admin/config/, it displayed this error:
Fatal error: Call to undefined function libraries_detect() in /Users/chrisroane/Sites/acquia-drupal/sites/default/modules/manymail/manymail.install on line 21

However, I can access this page: /admin/config/manymail

3. The README.txt file is still not populated with information. It doesn't need to be 100% complete, but what you have there is not enough: http://drupal.org/node/447604

A few things that are not clear to me that should be cleared up on the project page and the readme file:

* How is this module intended to be used? I've installed the module, but there is not any information where I need to go from there.
* Information on the maximum amount of emails that we can expect this module to be able to send. If this information is not known, than what is the most emails this module has been tested to send out?

4. I tested the config area and it appears to save the settings. Are there options for sending plain text or html emails? I think this is a pretty important feature.

AUTO REVIEW
-----------
There are a few issues that PAReview brought that are not in the changelog file.

Thanks for clarifying the purpose of the module.

--------------
MANUAL REVIEW
--------------

1. I figured out why the error was appearing on the config page. It was because I had libraries 7.x-1.0 installed...which was the only version. I missed that you specified this on your project page. I would recommend you specify the version number of the libraries module that is required in your .info file. I think you can do this by making it look like this: dependencies[] = libraries (2.x) .

2. For the SMTP settings, is there a way in validating the settings that are saved? Maybe send one test email using the settings, once the form is submitted? I can't seem to get the send function to work, and I'm not sure if it is because of my SMTP settings or if it is a local server issue. The watchdog log doesn't give any specific details in why the send function didn't work.

3. Can you specify on the admin/config/manymail/config/defaults for the reply to name/email, that they can leave it blank for it to use the from name/email options? I noticed that it specified this on the manymail/all but not on the main config page.

4. Also on the SMTP settings page. After I enter the information and go back to the form...it is not clear to me that the password is being stored (the field is blank). Usually they only need to re-enter a password if they are changing the password. It would be great to see that functionality here.

5. The system doesn't seem to require the phpMailer library properly? I know it mentions this in the INSTALL.txt file...but when I didn't install the library and attempted to send an email I got this error: An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /batch?id=4&op=do StatusText: OK ResponseText: Fatal error: require_once() [function.require]: Failed opening required '/Users/chrisroane/Sites/acquia-drupal//class.phpmailer.php' . It would probably help people in getting this working if it displayed an error if it can't find this before they attempt to send an email.

6. The .module seems to do a lot. Could you break it up a bit to make things easier to maintain? Maybe separate the admin functions from the sending functions in separate files outside of the .module file.

------------
AUTO REVIEW
------------

I only see one warning, outside of files in the master branch and the changelog file with the online code review utility: http://ventral.org/pareview/httpgitdrupalorgsandboxkristiaanvandeneynde1... . You are close!

However, I ran the coder module code review. Attached are the drupal coding standards issues I found in a PDF file. I suggest running this module from your end and resolving these issues.

I do like what I see, and I think this module will be useful!

Updated status to needs work.

Okay, I see what was going on. You're right....I wasn't using the 7.x-1.x branch. I was just using the checkout code from your project page. It is a pretty big issue to have old files under the master branch...but I know you are going to fix that when you can.

1. I checked out the latest code for 7.x-1.x . But when I don't have the phpmailer module installed, it doesn't display an error until I try to send, and the error isn't very pretty.

2. In the recipeint list section, I know the format specified email@email.com|Name....which works great. But I don't think the name should be required (maybe they just have a list of email addresses). Can you make the name optional?

3. I got the "Mail All Users" option and the "Mail user roles" option to work. However, when I tried to test the recipient list send option, I received this error:

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /system/ajax
StatusText: Service unavailable (with message)
ResponseText: PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')) AND (u.status = '1')' at line 2: SELECT DISTINCT u.uid AS uid, u.mail AS mail, u.name AS name
FROM 
{users} u
INNER JOIN {users_roles} ur ON u.uid=ur.uid
WHERE  (ur.rid IN  ()) AND (u.status = :db_condition_placeholder_0) ; Array
(
[:db_condition_placeholder_0] => 1
)
in ManyMailRecipientList->addRole() (line 124 of /Users/chrisroane/Sites/acquia-drupal/sites/default/modules/manymail/classes/manymail.recipientlist.inc).

I'm new to reviewing drupal projects, but I think you are close!

Kristiaan:

1. I verified that the warning does show up in the Status area when phpmymailer library is not installed.

3. I verified that this bug was fixed on the 7.x-1.x branch.

I'm confused at why you are not able to clear out all of the files from the master branch? This is a pretty big deal, as it makes it confusing for those who want to contribute when there are files in that directory. Once that is fixed, I will mark it as "reviewed & tested by the community".

That's what I meant. I don't think you can delete the master branch in git (it is there by default). I think it is just drupal standards not to put anything in the master banch, since you don't know which version it is.

Looks good from my end. Changed it to "reviewed & tested by the community".

Review of the 7.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. Get a review bonus and we will come back to your application sooner.

manual review:

• "drupal_alter('manymail_recipients', $recipients);": this should also be documented in manymail.api.php
• "watchdog('ManyMail', 'Caught an exception when trying to send an e-mail: ' . $e->errorMessage(),...": do not concatenate a translatable string like this, use a placeholder for the exception message. Also elsewhere.
• manymail_send_mail_batch_finished(): same here, this could also lead the XSS vulnerabilities if the name contains malicious javascript. Make sure to sanitize the log message by using an "@" placeholder.
• manymail_form_manymail_send_role_form_alter(): do not check the admin role here, use permissions to determine whether a user can use a roles or just a limited set. And remove manymail_is_admin(), hard coding access to a particular role instead of a permission is bad. Same in manymail_send_form().
• you should implement hook_uninstall() to remove all the variables that your module creates with variable_set().
• manymail_lists_targets_lists_manage_create_form(): the javascript should be put into a separate .js file.
• "t('Are you sure you want to delete the following list:') . ' <strong>' . check_plain($data->display_name) . '</strong>'": Again, do not concatenate translatable strings, use placeholders instead.

Anyway, those issues should be fixed but are no blockers, so ...

Thanks for your contribution, kristiaanvandeneynde! 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.

Thanks to the dedicated reviewer(s) as well.