4
\$\begingroup\$

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:

  1. To one e-mail will NEVER be sent more than one message (it could be executed more than once)

  2. One e-mail must have only one recipient (not multiple to)

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

\$\endgroup\$
2
\$\begingroup\$

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

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.