phpMailer mass mailing script

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();

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)).

StackExchange Code Review Q#61428, answer score: 2