0

We have a script that monitors the database. In the database JSON data and fromName and fromEmail are stored. If the handledAt timestamp is null the email still has to be send. So we have created a cronjob that monitors the handledAt column. If it is null the email should be sent.

Our scripts works fine, however it is a bit slow. Therefore I was wondering if we are doing it in a proper manner.

Grab All emails that need to be sent

foreach($ResultSelectEmails AS $key => $value)
{
        require_once '../library/Twig/Autoloader.php';

        Twig_Autoloader::register();
        $loader = new Twig_Loader_String();

        $twig = new Twig_Environment($loader, array(
                'cache' => '../tmp/cache/'
        ));

        $Contents = $twig->render(stripslashes($Templ['templateHTML']), $EData);
        $Subject = $twig->render(stripslashes($Templ['templateSubject']), $EData);

        $WriteFile = fopen('logs/'.$PDFLogFile.'.html','w');
        fwrite($WriteFile, $Contents);
        fclose($WriteFile);

        // Create the Transport
        $transport = Swift_SmtpTransport::newInstance('mail.example.com', 25)
        ->setUsername('info@example.com')
        ->setPassword('examplePassword')
        ;

        // Create the Mailer using your created Transport
        $mailer = Swift_Mailer::newInstance($transport);

        $message = Swift_Message::newInstance($Subject)
        ->setFrom(array($ResultSelectEmails[$key]['fromEmail'] => $ResultSelectEmails[$key]['fromName']))
        ->setTo(array($ResultSelectEmails[$key]['toEmail']))
        ->setBody($Contents, 'text/html', 'UTF-8')
        ;

        if(isset($EmailAttachements))
        {
            foreach($EmailAttachements AS $key => $value)
                $message->attach(Swift_Attachment::fromPath($value));
        }

        $result = $mailer->send($message);
}

In the script we create a separate transport for each email. Sometimes the mail server throws an error. Mainly that it can't login.

Can we improve the script performance?

Looking forward to your thoughts....

Peter Fox
  • 1,239
  • 2
  • 11
  • 31

1 Answers1

0

I see some calls that you shouldn't do in a foreach loop. I don't know how many mails you want to send but this is not optimal. As you're not processing the loop in parallel you can move a few lines outside which should prevent creating some instances over and over again. I don't know the exact purpose of your code and without any details it's hard to say if this is a noticable improvement, but you could give it a try:

require_once '../library/Twig/Autoloader.php';

Twig_Autoloader::register();
$loader = new Twig_Loader_String();

$twig = new Twig_Environment($loader, array(
    'cache' => '../tmp/cache/'
));

// Create the Transport
$transport = Swift_SmtpTransport::newInstance('mail.example.com', 25)
        ->setUsername('info@example.com')
        ->setPassword('examplePassword')
;

// Create the Mailer using your created Transport
$mailer = Swift_Mailer::newInstance($transport);

foreach ($ResultSelectEmails AS $key => $value) {
    $Contents = $twig->render(stripslashes($Templ['templateHTML']), $EData);
    $Subject = $twig->render(stripslashes($Templ['templateSubject']), $EData);

    $WriteFile = fopen('logs/' . $PDFLogFile . '.html', 'w');
    fwrite($WriteFile, $Contents);
    fclose($WriteFile);

    $message = Swift_Message::newInstance($Subject)
            ->setFrom(array($ResultSelectEmails[$key]['fromEmail'] => $ResultSelectEmails[$key]['fromName']))
            ->setTo(array($ResultSelectEmails[$key]['toEmail']))
            ->setBody($Contents, 'text/html', 'UTF-8')
    ;

    if (isset($EmailAttachements)) {
        foreach ($EmailAttachements AS $key => $value)
            $message->attach(Swift_Attachment::fromPath($value));
    }

    $result = $mailer->send($message);
}
Fred
  • 3,324
  • 1
  • 19
  • 29