This module is intended to be an alternative to the core Contact module. It is recommended for those who have been unsuccessful in getting the Drupal 7 Contact module to send messages and other contributed modules have not worked to their satisfaction. Many who are on shared hosting have experienced this as well as those who are having trouble configuring up their own servers correctly.

This module uses the PHPMailer library, a rock solid object oriented program that makes sending smtp emails very simple. Users are allowed to store multiple smtp contacts each with their own settings and passwords. Passwords are stored in the database using 128bit encrytion. A block is included which can be optionally enabled.

Sandbox page

Repository

core = 7.x

Thanyou in advance for reviewing this project. I believe this module fills a need in the Drupal community. The main points are:

  • It works for me where nothing else worked quite right.
  • Users will be able to add multiple contacts (categories) each with it's own settings.
  • SMTP passwords are stored as encypted binaries as opposed to plain text as I have seen in other modules.
  • A contact block is included.
  • Support for personal contact forms is planned for the near future.
  • Todo: make proper use of the Libraries API. Perhaps someone can contribute the proper code.

I am a fairly new module developer and look forward to everyone's input. I have not done others' reviews yet. I am holding off on that until I get some feedback on how my own code is reviewed before I go on to review other projects. Once again, thankyou for your input and happy coding.

Comments

vineet.osscube’s picture

vineet.osscube commented

Hi,
Firstly there is lots of issue regarding line spaces, indentation & comments.

Look at this !
http://ventral.org/pareview/httpgitdrupalorgsandboxharoldvillacorte18580...

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

tomgeekery’s picture

tomgeekery commented

Hello,

A couple of things that I noticed.

There are for sure some spacing / coding standard issues, the link that osscube provided will be very helpful for that.

You are also using a git branch called 7.x-1.x-dev. I made the same mistake, if you create a new branch without the -dev ventral.org will stop complaining about that. This is a helpful page for adding and removing branches.

In your module file > hook_menu you don't need the type => MENU_NORMAL_ITEM in either of your items. That is the default so you can safely remove those lines.

You might be able to use the variables table in Drupal to store the key. If I am looking at this correctly the user won't need to enter multiple keys. This could save on creating a database field for it. You could use variable_get and variable_set for this. I could be completely wrong about the functionality of the keys so feel free to tell me if I am =] If you can use the variables table you would also need to write an uninstall hook in the install file to remove those variable(s) if the user removes the module.

tomgeekery’s picture

tomgeekery commented
Status: Needs review » Needs work
Harold Villacorte’s picture

Harold Villacorte commented

I will be working on this tonight.

  • Will complete basic refactoring according to Drupal coding standards.
  • Will use branch 7.1-1.x and delete branch 7.1-1.x-dev
  • Will try hook_menu without the 'type' index.

Tom you might be correct in that the variables table might work. I overlooked the fact that the table is structured similarly to my own table with the exception of "variables" using longblob whereas I am using blob. I used tinyblob in early development thinking that it would perform better but that was just an assumption, I never actually compared query execution times. I settled on blob because a book I read only lists blob and longblob as options in the Schema API. Putting the key and IV into the variables table might work but they are not a ($key => $value) pair so it would be using the table in a way it was not intended to be used. Not sure if this is a good thing but if it does not confuse Drupal then it might be good idea because it would obscure the location of the key and IV.

About using variable_set and variable_get: I am not sure the IV would stay intact because it is a binary before it goes to the database and it would have to get serailized and unserialized if I use those two functions. I will try it and if it works I will definitely use it.

Harold Villacorte’s picture

Harold Villacorte commented
  • Moved project to branch 7.x-1.x and deleted branch 7.x-1.x-dev
  • Refactored code and is now fully validating in Coder. One error remains in Code Sniffer and Tough Love but it does not appear to be fixable without breaking Drupal. The issue is mixing t() strings and html.
  • Commented out 'type' index on hook_menu calls as defualt is used.
  • Added various comments.
  • I am considering writing a seperate module as a library interface.

@tomgeekery : after looking more closely at persistent variables and the variable_set and variable_get functions I have come to learn that you are absolutely correct in your thinking. It would be the best way to set those fields. After attempting it though it seems that the $iv cannot be serialized and attempting it completely breaks Drupal. For now it seems that keeping a seperate table and setter function is the appropriate solution.

I feel that the project is ready for another review. One again, Thankyou.

Harold Villacorte’s picture

Harold Villacorte commented
Status: Needs work » Needs review

Additionally I am considering further organizing the functions into classes. I am not sure if that is the Drupal way, it is just a thought for now.

gnucifer’s picture

gnucifer commented

To me this seems like a redundant module. The same functionality can be achieved by using http://drupal.org/project/mailsystem coupled with http://drupal.org/project/smtp if I am not mistaken. No custom module is needed. I guess I should set this to "closed (won't fix)" but a more seasoned reviewer should probably have a look first and make the definite decision.

Harold Villacorte’s picture

Harold Villacorte commented

There is some overlap but it is not completely redundant. I just tested out out the combination you pointed out and it works on my deveopment machine just fine. I originally tried Mail System coupled with Swift Mailer and there were major issues. Aside from the fact that SMTP module is using third party code as opposed to an external library I am ok with it, as a mater of fact I will use it myself and perhaps contribute to it when warranted. I am still not completely certain that the smtp password is secure but if I find an issue I will point it out to the developer directly.

This module, PHPMailer Contact, still offers fuctionality that in my opinion jusities inclusion in Drupal. It does not attempt to replace the Drupal mail system, we have the SMTP module combined with Mail System to do that. What this module does is offer an alternative to the Contact module. It allows site builders to declare multiple categories each with it's own smtp settings. Each category administrator can safely be given access to the settings page to set his or her own server settings and password. I cannot find another contributed module that offers that fuctionality. If this feature in itself is not enough to warrant a contributed module I will not pursue the issue any further. Otherwise the project status should be set to "needs work" and the minimal work to be completed before the next review would be as follows:

  • Rewrite the project description to point out the similarities between this module and other contributed modules and make it clear to the user when and why they should use this module.
  • After futher review of my own code I can use the "variable" table but not variable_set and variable_get functions. I would only need one custom table and not two.
  • Work on integration with the Libraries API module. I have been experimenting with it and am close to getting it to work right. The way I am including the PHPMailer library now is Drupal "acceptable" but not Drupal "preferred." Drupal preferred is my own standard but only if it works better and not worse.
gnucifer’s picture

gnucifer commented

Ok. You are bypassing the Drupal API by implementing your own mail-solution though. You should be using hook_mail, and MailSystemInterface for implementing new providers. I'm sure you could use MailSystem and SMTP modules as dependencies for your own module to provide just the extra functionality without reimplementing stuff, or perhaps even write an module extending the "ordinary" contact module.

Harold Villacorte’s picture

Harold Villacorte commented
Status: Needs review » Needs work

gnucifier, thankyou for the review. You seem to be very knowledgable on this particular topic. This module will get a major rewrite. I have considered all of your suggestions and here is what I have come up with as a new development roadmap for this module:

  • Inject dependencies for the core Contact and SMTP Authentication Support modules.
  • Alter the core Contact forms using Drupal form_alter() functions.
  • Load the the SMTP module required files using module_load_include().
  • Instantiate the SmtpMailSystem object from the SMTP Authentication Support module.
  • Set the SmtpMailSystem object attributes from within the Contact Form submit handler when the system default category is not selected.
  • SmtpMailSystem methods can be used as is.
  • Rename the project to something more fitting.

If I am not mistaken, this should result in a high quality module. The only question I have is whether I should alter the Contact table or keep the custom table. Any other input is greatly appreciated. I am changing the project status to "needs work."

gnucifer’s picture

gnucifer commented

I have thought about it some more. Drupal is agnostic when it comes to the method of sending mails.

This is the method sending mails: http://api.drupal.org/api/drupal/includes%21mail.inc/function/MailSystem.... In your case you want to force the use of a certain MailSystemInterface that support different smtp-settings per "category". If you look in http://api.drupal.org/api/drupal/includes%21mail.inc/function/drupal_mai... you can easly force the usage of your own mail system by merging in array('your_module_name' => 'YourMailSystemClass') into the 'mail_system'-variable.

The "best" though in terms of least redundant code (and work for you) would be to use the smtp-module to send mails. Unfortunately it seems like it does provide an opportunity for other module to provide smtp-settings in it's mail-method. So unless you pursuade the module authros to provide a more pluggable interface (not too hard to do), you would either have to extend that modules SmtpMailSystem-class: this is risky and not very elegant since so much of the code is in the one method you need to override, or copy this class and use as a template for your module.

One way of "fixing" the smtp-module to provide pluggable settings could for example be replacing this (line 449 in smtp.mail.inc):

    // Set the authentication settings.
    $username = variable_get('smtp_username', '');
    $password = variable_get('smtp_password', '');

with this:

    // Set the authentication settings.
    // Allow other modules to provide smpt-settings based on id.
    $username = variable_get('smtp_username_' . $message['id'], variable_get('smtp_username', ''));
    $password = variable_get('smtp_password_' . $message['id'], variable_get('smtp_password', ''));

That way you should be albe save your different mail settings as variable_set('smtp_username_MODULE_NAME' . $category_name, $username); etc. and use category name as $key when sending mails with drupal_mail

Harold Villacorte’s picture

Harold Villacorte commented

The SMTP module does present a huge problem by instantiating the $mailer object within the the mail() method. Unless I am missing something, PHP does not have a way to access an object instantiated within a parent method. Extending the class would defeat the purpose of the module dependency because as you pointed out I would have to rewrite the entire $mailer object, thus repeating ourselves. On the other hand it would be a shame not to use the SmtpMailSystem class as it makes very robust use of the PHPMailer library.

What I am going to do is apply to join the SMTP Authentication Support project and see if they are willing to include some of the ideas stated here. If not I will implement my own MailSystemInterface.

gnucifer’s picture

gnucifer commented

Yes you are correct about it being hard to hook-into the SmtpMailSystem class at it's current state. There is probably other solutions than the one i provided above. Perhaps the mail method could take more optional parameters for phpmailer min the $message object in a 'phpmailer' namespace for example. Sounds good otherwise!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Closed (won't fix)

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

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)