1
\$\begingroup\$

I am relatively new to OOP. I have an application that requires to send some emails from the server for various use cases. I have moved my code into a Mail class to try to abstract this out. Coming from years as a functional programmer, it is becoming challenging to move into the OOP space. This class is for very generous purposes, and I do not expect to handle attachments, bcc, or any other extra mailer usages. While there are nice and handy Mail packages I can find and implement within a few minutes, I thought it would be best to create this on my own.

My class is as follows:

<?php
/**
 * Mail Class
 */

namespace Utility;


class Mailer
{
    public $sendTo;
    public $sendFrom;
    public $subject;
    public $message;
    public $headers;
    public $error = [];

    public function __construct($sendTo, $sendFrom = null, $subject, $message)
    {
        $this->sendTo = $sendTo;
        $this->sendFrom = ($sendFrom) ? $sendFrom : 'webmaster@shelter.pet';
        $this->subject = $subject;
        $this->message = $message;
        //$this->headers = $this->setHeader();
    }

    public function setTo($email, $name) {
        return $this->sendTo = $email;
    }

    public function getTo() {
        return $this->sendTo;
    }

    public function setFrom($email, $name)  {
        return $this->sendFrom = $email;
    }

    public function setSubject($subject) {
        return $this->subject = $subject;
    }

    public function getSubject() {
        return $this->subject;
    }

    public function setMessage($message) {
        $this->message = $message;
        //@TODO:string repace, etc

        return $this;
    }

    public function getMessage() {
        return $this->message;
    }

    /*public function setHeader($header, $email, $name = null) {
        $address = $this->formatHeader((string) $email, (string) $name);
        $this->headers[] = sprintf('%s: %s', (string) $header, $address);
        return $this;
    }*/

    public function setHeader() {
        $headers = 'From: '.$this->getFrom() . "\r\n" .
            'Reply-To: '.$this->getFrom() . "\r\n" .
            'X-Mailer: PHP/' . phpversion();

        $this->headers = $headers;
        return $this;
    }

    public function getFrom() {
        return $this->sendFrom;
    }

    public function send() {
        $to = $this->sendTo;
        $from = $this->sendFrom;
        $subject = $this->subject;
        $message = $this->message;
        //$headers = $this->headers;
        $headers = 'From: '.$this->getFrom() . "\r\n" .
            'Reply-To: '.$this->getFrom() . "\r\n" .
            'X-Mailer: PHP/' . phpversion();

        return mail($to, $subject, $message, $headers);
    }
}

And to implement the class I do the following in my code:

//alert admin of failed login
                $adminFailedAlert = new Mailer('email@domain.com', null, 'User Failed Login', 'A user has unsuccessfully logged in at '.Utility::getDateTime().'. You may want to look into this.' );
                $adminFailedAlert->send();

This Class works fine for me. The only issue I am having currently is creating dynamic header string - currently I just manually place them into the send() method - which is why you see the commented out header references. I have not implemented the $errors method so will work on that. I guess at this point, am I on the right path? I am also confused why I should have my getter and setter methods when I am not even using it when I implement the Class. I am also trying to overcome to obstacle of knowing when to use private or public, etc method declarations.

\$\endgroup\$
1
\$\begingroup\$

Other, similar libraries don't require you to pass everything to the constructor. What if you want to send an email to more than one person? You should be able to call $mailer->setTo() more than once.

For now, I would suggest getting rid of the constructor entirely, and calling the setters manually as needed. After all, if you're planning on doing the entire operation in one call then you might as well write a function instead of a class.

If you're going to implement setter methods then you should make your properties private or protected so they can't be changed manually by whoever is using your class. Usually, the setters are implemented so you can validate input before setting them, for example,

public method setTo($email){
    if(filter_var($email, FILTER_VALIDATE_EMAIL)) $this->toEmail = $email;
    else $this->lastError = "Invalid to email: {$email}.";
}

Now you can prevent the user from manually setting $mail->toEmail by marking it as private, thus, forcing the input to be validated by your setter.


You're not using your toName parameter and you're only allowing a single receiver. Check out the manual and you'll notice that you can have multiple recievers and you can specify a name for each one in the following format:

User <user@example.com>, Another User <anotheruser@example.com>

Therefore, you should have an array for your toNames and another array for your toEmails.

public method setTo($email, $name=""){
    if(filter_var($email, FILTER_VALIDATE_EMAIL)){
        $this->toEmail[] = $email;
        $this->toName[] = $name;
    }else $this->lastError = "Invalid to email: {$email}.";
}

Then, in your send function...

$to = "";
foreach($this->toEmail as $k=>$v){
    if(!empty($this->toName[$k])) $to .= "{$this->toName[$k]}<$v>,";
    else $to .= "$v,";
}
$to = rtrim($to, ","); // trim off trailing comma

If you're not sending an HTML email you should wordwrap your $message parameter at 70 chars. (You can create a variable to determine if the email will contain HTML).

if(!$this->isHTML) $message = wordwrap($message, 70, "\r\n");

If it is HTML you have to let the client know by adding these headers:

$headers .= 'MIME-Version: 1.0'."\r\n".
    'Content-type: text/html; charset=iso-8859-1';

You can also carbon copy someone with this header:

'Cc: birthdayarchive@example.com'

Anyway, you're on the right track, but make sure you read the manual to see what other cool stuff you could be doing.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

A few pointers:

  • Since you have getters and setters in place, do not declare $sendTo, $subject etc. as public. Instead declare them as protected (for inheritance) or private.
  • SMTP allows for multiple receivers, so maintain sendTo as an array
  • Similar to above, declare $headers as an associative array, and compile them all only when send is called. You can declare addHeader and removeHeader to create and/or remove a specific header field. Your From, cc, bcc support implementation would a trivial supplement after this.
  • Ideally, in constructors, the order of parameters is kept in such a manner such that all mandatory parameters appear first, and those which have defaults declared for them are defined later, in decreasing order of priority. This is not restrictive to constructor definitions alone.
| 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.