Problem
I’m working on a script that will be able to send mass e-mails. I’m using cron to run this script. I’m fetching e-mails from queue.
I need to ensure that:
-
To one e-mail will NEVER be sent more than one message (it could be executed more than once)
-
One e-mail must have only one recipient (not multiple to)
-
It must to jump over non existing e-mail addresses
Please, can you help me to improve this script? I need to find hidden bugs in this code.
try {
$logger = new Logger(BASE_DIR . '/log/');
$dbh = new PDO($dsn, $username, $password, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$dbh->beginTransaction();
// Fetch queue + create lock for update
$sql = 'SELECT mq.`mail_queue_id`,
mq.`to`,
mq.`priority`,
mb.`from`,
mb.`reply_to`,
mb.`alias`,
mb.`subject`,
mb.`message`
FROM mail_queue mq
JOIN mailer_batch mb ON mq.`mailer_batch_id` = mb.`mailer_batch_id`
WHERE mq.`mail_status_id` = 2
ORDER BY mq.`priority` DESC, mq.`created` ASC
LIMIT ' . BATCH_SIZE . ' FOR UPDATE;';
$sth2 = $dbh->prepare($sql);
$sth2->execute();
$queue = $sth2->fetchAll();
if (count($queue) > 0) {
// Fetch active SMTP data
$sth1 = $dbh->prepare("SELECT * FROM `smtp_server` WHERE `active` = 1 LIMIT 1;");
$sth1->execute();
$smtpData = $sth1->fetch();
if ($smtpData === false) {
throw new Exception("There is no active SMTP server.");
}
$smtp = new SmtpServer($smtpData['host'], $smtpData['port'], $smtpData['auth'], $smtpData['username'], $smtpData['password'], $smtpData['secure']);
}
// Send it
foreach ($queue as $row) {
$csvArray = array();
// Fetch all params
$sql = 'SELECT * FROM mail_param WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth3 = $dbh->prepare($sql);
$sth3->execute();
$params = $sth3->fetchAll();
$paramArray = array();
foreach ($params as $param) {
$paramArray[$param['param_key']] = $param['param_value'];
}
if (count($paramArray) > 0) {
$csvArray[$row['to']] = $paramArray;
}
// Create mail template
$mailTemplate = new MailTemplate($row['from'], $row['alias'], $row['subject'], $row['message'], $row['reply_to']);
$mailTemplate->applyParameters($csvArray);
try {
$res = $smtp->send($row['to'], $mailTemplate);
// Update state (3 = sent, 4 = sending error)
if ($res) {
$sql = 'UPDATE mail_queue
SET mail_status_id = 3
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
} else {
$sql = 'UPDATE mail_queue
SET mail_status_id = 4
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$logger->log('E-mail pro "' . $row['to'] . '" se nepodařilo odeslat.');
}
$sth4 = $dbh->prepare($sql);
$sth4->execute();
} catch (phpmailerException $e) {
$logger->log($e->getMessage(), $logger::WARNING);
$sql = 'UPDATE mail_queue
SET mail_status_id = 4
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
} catch (Exception $e) {
throw $e;
}
}
$dbh->commit();
} catch (PDOException $e) {
$logger->log($e->getMessage(), $logger::ERROR);
$dbh->commit();
} catch (Exception $e) {
$logger->log($e->getMessage(), $logger::ERROR);
$dbh->commit();
}
And SmtpServer->send() method:
public function send($emailRecipient, MailTemplate $mailTemplate)
{
$mailer = new PHPMailer(true);
$mailer->isSMTP();
$mailer->SMTPKeepAlive = false;
$mailer->SMTPDebug = 0;
$mailer->Debugoutput = 'html';
$mailer->Host = $this->getHost();
$mailer->Port = $this->getPort();
// SMTP auth
$mailer->SMTPAuth = $this->getSmtpAuth();
if ($this->getSmtpAuth()) {
$mailer->SMTPSecure = $this->getSecure();
$mailer->Username = $this->getUsername();
$mailer->Password = $this->getPassword();
}
// Set ReplyTo
$replyTo = $mailTemplate->getReplyTo();
if ($replyTo != "") {
$mailer->AddReplyTo($replyTo, $mailTemplate->getAlias());
}
// From and alias
$mailer->setFrom($mailTemplate->getFrom(), $mailTemplate->getAlias());
$isMessageHtml = $mailTemplate->isMessageHtml();
$mailer->Subject = $mailTemplate->getSubject($emailRecipient);
$mailer->CharSet = 'UTF-8';
// HTML or plain text body
if ($isMessageHtml) {
$mailer->msgHTML($mailTemplate->getMessage($emailRecipient), dirname(__FILE__));
} else {
$mailer->isHTML(false);
$mailer->Body = $mailTemplate->getMessage($emailRecipient);
}
// To
$mailer->addAddress($emailRecipient);
if ($mailer->send()) {
return true;
} else {
return false;
}
}
In the past I had some serious issues with multiple “To” in one e-mail; this is why I reconnect after sending each e-mail. I think ClearAllRecipients() and other Clears function are not safe enough and reconnecting after sending one e-mail is much more safer.
Solution
This is not a complete review, just a couple of points I noticed. I didn’t see any bugs, but I didn’t test the code in-depth.
Security
You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id'] and $row['to']. Otherwise, you might be vulnerable to second order SQL injection. See also here.
if (cond) return true; else return false;
if ($mailer->send()) {
return true;
} else {
return false;
}
can be rewritten as:
return $mailer->send();
Duplicate Code
$sql = 'UPDATE mail_queue
SET mail_status_id = [id]
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
You have this code 3 times, I would either extract it to it’s own function (updateMailQeue(errorCode)), or just save the error code in a local variable and execute this code at the end.
Confusing Check
You have this check:
if (count($queue) > 0) {
// prepare smtp server
}
// do stuff with the smtp server
For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach is an implicit check itself). I would rewrite it like this:
if (count($queue) > 0) {
return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server
If you at some point extend the code, your code could easier lead to bugs.
Length of first Code Block
Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)).