1
\$\begingroup\$

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!';
            }
        }
    }
?> 
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does \$\endgroup\$ – Dannnno Apr 6 '18 at 2:12
  • \$\begingroup\$ It's for a site I'm working on ndkutz dot net. I want a user to be able to send an email to the barbershop owner from the website. \$\endgroup\$ – Rickie Knight Apr 6 '18 at 3:08
  • \$\begingroup\$ @Dannnno come on, you you really need a context for sending an email? Well, the purpose of this code is... to send an email. \$\endgroup\$ – Your Common Sense Apr 6 '18 at 5:12
  • 1
    \$\begingroup\$ To discuss the code itself: What I find weird is that you have a 'mail' class, but you only use it to store and retrieve 3 values. That's perfectly alright in itself, but why not have a mail class that can actually send a mail for you? In other words, you set the fields needed and then just use a method like: $email->sendTo('receiver@there.you.go'); to send it. \$\endgroup\$ – KIKO Software Apr 6 '18 at 6:56
  • \$\begingroup\$ Reiterates the fact that it's my first OO script but thanks. \$\endgroup\$ – Rickie Knight Apr 6 '18 at 7:59
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I appreciate your constructive criticism and will implement your changes. \$\endgroup\$ – Rickie Knight Apr 7 '18 at 1:37
  • \$\begingroup\$ Hey, @Your Common Sense. Can you do me a huge favor. Can you please give me a beginner PHP project that can be coded OOP style? I've googled but I went to your profile and I see that you are a PHP wiz and I would specifically like your advice. \$\endgroup\$ – Rickie Knight Apr 7 '18 at 20:43
  • \$\begingroup\$ That's a hard task, to assign such a project. I need to think of one. Did you finish with email class yet? \$\endgroup\$ – Your Common Sense Apr 8 '18 at 5:53
0
\$\begingroup\$

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>");
}
\$\endgroup\$
  • \$\begingroup\$ I appreciate your help and will implement your changes. \$\endgroup\$ – Rickie Knight Apr 7 '18 at 1:38

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.