This module provides SMTP Authentication Support for all users (each user can independently configure their authentication credentials and server information). Additionally, this module integrates with the new HTTP Parallel Request Module to enable mail message to be sent instantly (or in batch) but in a separate process (non-blocking). This should allow a much snappier experience for users sending mail messages as they don't have to wait for the server communications to complete before continuing.

This is a Drupal 6 module.

Link to the Sandbox Project

Link to the GIT Respository

Comments

ianwremmel’s picture

ianwremmel commented
Status: Needs review » Needs work

Hi ISPTraderChris,

Please check your leading spaces in smtplease.install. The code in hook_schema should be indented an additional level. Additionally, please run your project through the coder module. Mostly, it'll point out style problems, but there are several places where your database interaction needs to be changed slightly.

To the best of my knowledge, I don't have PEAR Mail on my testing environment (I'm using Acquia Dev Desktop). It would be helpful to add a test to somewhere in the install process and perhaps on the configuration pages to present a message to the user indicating that pear mail is missing.

After installing, I went to the module settings page at admin/settings/smtplease/default, then to my user edit page. I was presented with the message "user warning: Table 'd6coder.smtplease_user' doesn't exist query: select * from smtplease_user where uid = 1 in C:\Users\ian\projects\workspace\d6coder\sites\all\modules\smtplease\smtplease.module on line 76." It appears that you don't have in implementation of hook_install. drupal_install_schema needs to be invoked in hook_install to install the schema. Rather than dropping tables in hook_uninstall, invoke drupal_uninstall_schema.

ISPTraderChris’s picture

ISPTraderChris commented
Status: Needs work » Needs review

Hi Ian -

Thanks for your review and comments. I've run the module through Coder and have made all of the recommended changes. Additionally, I've added warnings that are displayed during install, and on the configuration page when the module is unable to detect the presence of the PEAR Mail module. I also updated the .install file per your recommendations (and tested on a fresh install).

All the above changed have been committed.

Please let me know if there is anything further I should address.

Thanks again!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
  • git release branch missing, see http://drupal.org/node/1015226
  • there is a "1299264" folder in the repository, please remove it
  • info file has wrong line endings ('\r' but should be unix style '\n')
  • "Implementation of hook_schema." should be "Implements hook_schema().", also for other hook implementing functions.
  • smtplease_schema(): don't run SQL schema related descriptions through t(), this will only cause overhead for translators.
  • use hook_requirements() to deal with your PEAR package dependency. Don't use hook_install() for that.
  • "variable_set('smtp_library', drupal_get_path('module', 'smtplease') . '/smtplease.inc');" you are doing that every time the module file is loaded. Please move it to hook_enable().
  • "// Build admin settings form" comments should end with a "."
ISPTraderChris’s picture

ISPTraderChris commented
Status: Needs work » Needs review

Thanks again for your review.

All of the changes noted above have been made, and committed. I have added a 6.x-1.x development branch, and will tag a release once the review process has been completed.

Please let me know if anything further is required.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
  • "php = 4.0.0" no need to specify that, Drupal 6 core already requires PHP 4
  • don't use t() directly in hook_requirements(), see http://api.drupal.org/api/drupal/developer--hooks--install.php/function/...
  • "db_query("delete from {smtplease_queue} where processed <> 0");": please see the SQL coding standards http://drupal.org/node/2497
  • smtplease_settings_form(): all messages should be translatable, also in drupal_set_message() and some #title values in that function.
ISPTraderChris’s picture

ISPTraderChris commented
Status: Needs work » Needs review

Updated and committed per #5. Ready for review.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

_smtplease_get_next_message(): apply SQL coding standards here, too. Check all your SQL strings.

Otherwise I think this is RTBC. Now while we are waiting for the access grants to create full projects would you be so kind to do a review of the other applications pending? Just pick one from this list: http://drupal.org/project/issues/projectapplications?status=8

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, ISPTraderChris! 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 minde: Commit messages - providing history and credit and Release naming conventions.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.