PHP email script

Question

This is my first OOP script, a PHP email script designed for a site I'm working on: ndkutz(.)net. I want a user to be able to send an email to the barbershop owner from the website. I'm self taught and even though I know I'm on the right track I feel absolutely lost. Is my code any good?

<?php

 $error = '';
 $errormsg = '';
 $finalMessage = '';
 $finalName = '';
 $finalSubject = '';
 $finalTo = '';
 $finalHeader = '';
 $sendingEmail = '';

class emailConstruction 
{
    private $from = "";
    private $name = "";
    private $message = "";

    public function scrubAll($data) {
        $data = htmlspecialchars($data);
        $data = trim($data);
        $data = strip_tags($data);
        return $data;
    }

    public function setfrom($from){
        $this->from = stripslashes($from);
        $this->from = $from;
    }

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

    public function setName($name){
        $this->name = scrubAll($name);
        $this->name = $name;
    }

    public function getName(){
        return $this->name;
    }


    public function setMessage($message){
        $this->message = scrubAll($data);
        $this->message = wordwrap($data,70,"<br />");
        $this->message = $message;
    }

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

if(isset($_POST['submit']))
    {
        if(empty($_POST['uname']))
        {
            $error = 1;
            $errormsg = "Your name is required.";
            return false;
        }else{
            $error = 0;
            $createEmail = new emailConstruction;
            $createEmail->setName($_POST['uname']);
        }
        if(empty($_POST['umail']))
        {
            $error = 1;
            $errormsg = "Email address required.";
            return false;
        }else {
            $error = 0;
            $createEmail =  new emailConstruction;
            $createEmail->setTo($_POST['umail']);
        }
        if(empty($_POST['umsg']))
        {
            $error = 1;
            $errormsg = "Message is required";
            return false;
        }else{
            $error = 0;
            $createEmail = new emailConstruction;
            $createEmail->setMessage($_POST['umsg']);
        }
        if($error = 0)
        {   $finalHeader = 'from:' . $finalFrom;
            $finalHeader .='MIME-Version: 1.0\r\n';
            $finalHeader .='Content-type: text/html\r\n';
            $finalMessage = $createEmail->getMessage();
            $finalName = $createEmail->getName();
            $finalSubject = 'New potiential client by the name of ' . $finalName;
            $finalTo = $createEmail->getTo();

            $sendingEmail = mail($finalTo,$finalSubject,$finalMessage,$finalHeader);
            if($sendingEmail == true)
            {
                $emailMessageS = 'Email sent successfully!';
            }else{
                $emailMessageF = 'Error. Please try again!';
            }
        }
    }
?> 

Answer 1

There are several issues to review

OOP

First off, it's a good intention but a very bad implementation.
A class should be made on purpose, but this class' purpose is uncertain. Why would you need a class that just prepares the data but not a class that sends the actual email?

In your place I would create a class that has methods like setSubject(), setTo(), setBody() and - most important one - send(). Such a class would have a very good use.

Cargo cult code

No offense, but every operator in your code should be justified. Writing a certain operator only because you've seen it used somewhere makes a cargo cult code (the name is from the story about savages on the Pacific islands creating straw planes during WWII in hopes those will bring cargo as good as real ones). Unfortunately, almost none of them are. Take scrubAll() method for example.

• htmlspecialchars() and strip_tags() are mutual exclusive functions. Once you run the former, the latter will find nothing to strip. you should apply only one of them and it should be htmlspecialchars() as it does less harm
• trim() could be useful, but I don't think it's necessary in this case

so it makes your scrubAll function rather useless.

• stripslashes() used in setfrom() is absolutely of no use. It could have been used under some conditions 10 years ago but in 2018 it makes no sense to call it just in case. I had to use it only once in the recent 5 years, to fix a malformed JSON string.

Security.

Ironically, but despite all these preparations your code is still vulnerable to Mail Injection attack. With user input injected right into headers it's just a textbook example.

At best, you should never put anything from the user input into mail headers. Let alone "From:" header which will likely get your email in spam.

If you want a neat way to reply, make it "Reply-to:" header and validate the entered email, an example could be taken straight from the manual page.

Conclusion.

So, take out all getters from your class, call it "sendMail", remove useless functions and add a send() method which should assume the code which is in the global scope now

For the model example you may want to take a look at PHPMailer's usage examples. It is not that I ask you to write something similar but just to look at the way it is called in these examples.

Answer 2

One point that should be made about your code is this:

public function setfrom($from){
    $this->from = stripslashes($from);
    $this->from = $from;
}

Your setfrom() method is receiving the $from variable. You strip the slashes from it and assign the result to $this->from.

But then, you reassign $this->from to be equal to the original $from. So that essentially ignored the first line.

It's like this:

• I email you a Word document.
• You edit the document and save it as a new file. (in line 1 of the method)
• You take my original document and paste it over the copy you just edited. (in line 2)

You do this a couple more times:

public function setName($name){
    $this->name = scrubAll($name);
    $this->name = $name;
}

public function setMessage($message){
    $this->message = scrubAll($data);
    $this->message = wordwrap($data,70,"<br />");
    $this->message = $message;
}

In each case, remove the last line of each function. You have already defined $this->name or $this->message.

Also, you can use multiple functions on a single variable. For that last chunk of code, you could write just this to achieve the same result:

public function setMessage($message) {
    $this->message = wordwrap(scrubAll($data), 70, "<br>");
}