Optimising and consolidating multiple PHPMailer functions

[Josh Johnson]

So the two functions below both send emails via PHPMailer but both hold different messages (which use different data from the db). I was just wondering (as I plan to use PHPMailer more) a way to consolidate these functions into perhaps a sendMail() function which could handle different data variables and messages, whilst also allowing me to only set the $mail parameters once.

public function sendConfirmationEmail($firstName, $email, $emailCode) {

        global $mail;

        $to = $email;

        try {
            $mail->IsSMTP(); 
            $mail->Username           = "[email protected]";
            $mail->Password           = "••••••••"; 
            $mail->SMTPAuth           = true;            
            $mail->SMTPSecure         = "tls"; 
            $mail->Host               = "smtp.gmail.com";  
            $mail->Port               = 587; 
            $mail->addAddress($to);  

            $mail->From               = '[email protected]';
            $mail->FromName           = 'Connectd.io';
            $mail->AddReplyTo( '[email protected]', 'Contact Connectd.io' );

            $mail->isHTML(true); 

            $mail->Subject            = 'Activate your new Connectd account';

            $mail-&gt;Body               = "<p>Hey " . $firstName . "!</p>";
            $mail-&gt;Body              .= "<p>Thank you for registering with Connectd. Please visit the link below so we can activate your account:</p>";
            $mail-&gt;Body              .= "<p>" . BASE_URL . "login.php?email=" . $email . "&amp;email_code=" . $emailCode . "</p>";
            $mail-&gt;Body              .= "<p>-- Connectd team</p>";
            $mail-&gt;Body              .= "<p><a href='http://connectd.io'>www.connectd.io</a></p>";
            $mail-&gt;Body              .= "<img width='180' src='" . BASE_URL . "assets/img/logo-email.jpg' alt='Connectd.io logo' />&lt;br&gt;";

            $mail-&gt;Send();

        }catch(phpmailerException $e) {
            $general = new General($db);
            $general-&gt;errorView($general, $e);
        }catch(Exception $e) {
            $general = new General($db);
            $general-&gt;errorView($general, $e);
        }
    }


    public function sendVoteEmail($firstName, $email, $votes) {

        global $mail;

        $to = $email;

        try {
            $mail-&gt;IsSMTP(); 
            $mail-&gt;Username           = "[email protected]";
            $mail-&gt;Password           = "••••••••";
            $mail-&gt;SMTPAuth           = true; 
            $mail-&gt;SMTPSecure         = "tls"; 
            $mail-&gt;Host               = "smtp.gmail.com";
            $mail-&gt;Port               = 587; 
            $mail-&gt;addAddress($to);

            $mail-&gt;From               = '[email protected]';
            $mail-&gt;FromName           = 'Connectd.io';
            $mail-&gt;AddReplyTo('[email protected]', 'Contact Connectd.io');

            $mail-&gt;isHTML(true); 

            $mail-&gt;Subject            = 'You just got a vote on Connectd Trials!';

            $mail-&gt;Body               = "<p>Hey " . $firstName . "!</p>";
            $mail-&gt;Body              .= "<p>Congratulations - Someone has voted for you on Connectd Trials. </p>";
            $mail-&gt;Body              .= "<p>You now have <strong>" . $votes['CountOfvote_id'] . "/10</strong> votes</p>";
            $mail-&gt;Body              .= "<p>-- Connectd team</p>";
            $mail-&gt;Body              .= "<p><a href='http://connectd.io'>www.connectd.io</a></p>";
            $mail-&gt;Body              .= "<img width='180' src='" . BASE_URL . "assets/img/logo-email.jpg' alt='Connectd.io logo' />&lt;br&gt;";

            $mail-&gt;Send();

        }catch(phpmailerException $e) {
            $general = new General($db);
            $general-&gt;errorView($general, $e);
        }catch(Exception $e) {
            $general = new General($db);
            $general-&gt;errorView($general, $e);
        }
    }

[__]

Compose the message elsewhere, and make the function responsible only for preparing and sending the email.

Also, don’t do this!

function whatever( $arg1,$arg2 ){
    global $var1;
    //  ...
}

do this instead:

function whatever( $arg1,$arg2,$var1 ){
    //  ...
}

global state is evil.

[Josh Johnson]

@traq So say I have a file with a basic message in it with 2-3 variables, where do I call the function to prepare and send the mail? Would I still need separate functions for each different email message I need to send?

Thanks!

[Josh Johnson]

Would it be something like:

$email->sendConfirmationEmail($arg1, $arg2, $arg3);

public function sendConfirmationEmail($arg1, $arg2, $arg3) {
   $message = file_get_contents('email_templates/register.html');
   $this->sendEmail($subject, $message);
}

    public function sendEmail($subject, $message, $mail) {

        $to = $email;

        try {
            $mail->IsSMTP(); 
            $mail->Username           = "[email protected]"; 
            $mail->Password           = ""; 
            $mail->SMTPAuth           = true;            
            $mail->SMTPSecure         = "tls"; 
            $mail->Host               = "smtp.gmail.com";  
            $mail->Port               = 587; 
            $mail->addAddress($to);  

            $mail->From               = '[email protected]';
            $mail->FromName           = 'Connectd.io';
            $mail->AddReplyTo( '[email protected]', 'Contact Connectd.io' );

            $mail->isHTML(true); 

            $mail->MsgHTML($message);

            $mail->Subject = $subject;

            $mail->Send();

        }catch(phpmailerException $e) {
            $general = new General($db);
            $general->errorView($general, $e);
        }catch(Exception $e) {
            $general = new General($db);
            $general->errorView($general, $e);
        }
    }

[__]

Would it be something like…

Pretty much, yeah.

Question: where you have the variable $mail, are you actually doing anything with it beforehand? or are you just doing something like

$mail = new PHPmailer;

sendEmail( $subject,$to,$message,$mail );

? if so, you could just make the new PHPmailer object inside the sendEmail function.

public function sendEmail( $to, $subject, $message ){
        try {
            $mail = new PHPmailer;
            $mail->IsSMTP(); 
            $mail->Username           = "[email protected]"; 
            $mail->Password           = "********"; 
            $mail->SMTPAuth           = true;            
            $mail->SMTPSecure         = "tls"; 
            $mail->Host               = "smtp.gmail.com";  
            $mail->Port               = 587; 
            $mail->addAddress( $to );  
            $mail->From               = '[email protected]';
            $mail->FromName           = 'Connectd.io';
            $mail->AddReplyTo( '[email protected]', 'Contact Connectd.io' );
            $mail->isHTML(true); 
            $mail->MsgHTML( $message );
            $mail->Subject = $subject;
            $mail->Send();
        }catch(Exception $e) {
            $general = new General($db);
            $general->errorView($general, $e);
        }
    }

(also, there’s no reason to have two catch blocks if they don’t do anything differently.)

[Josh Johnson]

@traq Cool thanks!

Yeah the $mail variable just refers to a new object called in my initialisation file. I have heard quite a few people claiming global variables are bad – what is the problem with them – too open access?

Edit: After removing the global variable and creating a new object instead, I get a “Fatal error: Class not found”??

Also, as I plan to be sending about 10 different user emails, would you recommend having a class just for emails? At the moment it is just in a General class

[__]

I have heard quite a few people claiming global variables are bad – what is the problem with them – too open access?

The big thing is that it’s very easy for them to lead to hard-to-discover problems, especially as your program becomes larger or more complex. Your code should be doing everything it does deliberately.

In this case, you code simply assumes that a) there is a global variable named $mail, and b) it is an instance of the PHPmailer class. What if that var wasn’t available for some reason? You’d get an “undefined variable” notice, but many people have error reporting set to ignore notices. So, instead, you’d get a message along the lines of “trying to call method IsSMTP on a non-object”—which is far less clear and takes longer to track down.

Two solutions.

• dependency injection. That’s my first suggestion, where you actually have to pass the var you need into the function as a parameter. Any time you need a var from outside a function, it should be passed in deliberately.
• encapsulation. (This is usually considered in terms of objects, but the idea holds true here.) If your function needs something, and it is never needed outside the function, then the function can create and use it itself.

There are other benefits, having to do with making your code more flexible and adaptable in the future, being easier to debug, and just generally more robust. If you remember that global and $GLOBALS are, in general, “bad words,” you’ll be fine.

After removing the global variable and creating a new object instead, I get a “Fatal error: Class not found”??

You’ll have to make sure that the PHPMailer class definition is loaded before your function tries to create an instance of it. You could use an autoloader (you will learn to love these), or do something like require_once "/path/to/PHPMailer/definition.php" inside the function.

would you recommend having a class just for emails? At the moment it is just in a General class

Yes. I would probably recommend not having a “General” class at all. An object represents one “thing”: an item, a task, etc.. Each “thing” should have its own class, even if there is only one method involved.

[Author]

Posts