3
\$\begingroup\$

I have multiple forms that send an email. As of this moment I have 3 forms that send mail and vary only by the number of inputs:

  • One requires just an email
  • Another requires a name and email
  • Another requires name, email, and a comment

Instead of making 3 separate functions to mail the inputs away, I figured I could consolidate them into 1. The issue here is a lot of if statements. The code all works, but spans around 30 lines, which seems like too much.

function sendMail()
{
$comment = null;
$to =  "destination@mail.com";
$copy = "mymail@mail.com";
$subject = "Email..";
if (isset($_POST["contactSubscribe"]) || isset($_POST["questionSubmit"]) || isset($_POST["submit"])) {  
  foreach ($_POST as $field => $value) {
    if ($field == "contactName") {
      $contactName = $value;
    }
    if ($field == "email") {
      $email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
    }
    if ($field == "comment") {
      $comment = $value;
    }
  }
}
if (!is_null($contactName) && is_null($comment) ) {
  $message = "Name: $contactName Email: $email"; 
}
if (is_null($comment) && is_null($contactName)) {
  $message = "inputname=i_" . $email;
}
if (!is_null($contactName) && !is_null($comment) ) {
  $message = "Name: $contactName Email: $email Message: $comment"; 
}
mail($to, $subject, $message);
mail($copy, $subject, $message);
}
\$\endgroup\$
  • \$\begingroup\$ pastebin.com/jMJAhxw6. Just a suggestion. The helper functions can be hidden. The right variables still need to be globalized like into a class object in order for the code to run properly. \$\endgroup\$ – Eae Mar 16 '15 at 3:59
  • 2
    \$\begingroup\$ You'd probably be much better off using something like swiftmailer library instead of the built in php mail() function also I'd probably write a wrapper class rather than doing it as pure functions like this. Although both the suggestion above and the answer below are big improvements on your code. \$\endgroup\$ – Dave Mar 16 '15 at 15:08
3
\$\begingroup\$

I can slightly improve your function, like this:

function array_whitelist($array,$whitelist) 
// only allow certain keys in an array, in reaction to comment by tim
{
  $list = explode(',',$whitelist);
  return array_intersect_key($array,array_flip($list));
}

function sendMail()
{
  // check form submission
  if (is_array($_POST))
  {
    // initialize
    $to      = 'destination@mail.com';
    $copy    = 'mymail@mail.com';
    $subject = 'Email..';
    $message = '';
    // make all posted input fields into local variables
    $inputs  = array_whitelist($_POST,'email,contactName,comment');
    extract($inputs);
    // filter email address
    $email = filter_var($email,FILTER_SANITIZE_EMAIL);
    // add some fields to message 
    if (isset($contactName)) $message .= "Name: $contactName".PHP_EOL;
                        else $message .= "Name: i_".$email.PHP_EOL;
    if (isset($comment)) $message .= "Message: $comment".PHP_EOL; 
    // send mails
    mail($to, $subject, $message, "BCC: $copy");
  }
}

It is clear you seem to overdo the conditions, they are not used very effectively. But I think you have removed some essential parts, so this is never going to work.

Perhaps it is time you start to think about a more general email function, not just one that will fit your three forms. What if a fourth form comes along? So more than looking at what I did, think about generalizing your email routines. You could make a class. Seperate the building of the mail message from the sending of the emails. You can find enough examples here (search for: 'PHP Email Class').

Also don't forget the next thing you will want to do is send HTML email messages, and add attachments. That's when it becomes really complex. Email HTML and attachments are just plain tricky. Look for a library that can do these things for you, there are some very good ones out there (https://github.com/PHPMailer/PHPMailer, and the like).

\$\endgroup\$
  • \$\begingroup\$ I agree with most of your points, but you really shouldn't use extract on POST, ever. What if an attacker adds a post field such as to=spam@someone.com\ncc=spam@evenmorepeople.com? This would override the $to variable, and thus the form can be used to distribute spam. \$\endgroup\$ – tim Mar 16 '15 at 21:05
  • \$\begingroup\$ @tim: Yes, you're right, I wasn't thinking. It's nice and short though... no really, it's stupid. I will add something I use in these cases. \$\endgroup\$ – KIKO Software Mar 16 '15 at 21:30
0
\$\begingroup\$

Your function seems to do too much. Handling input from three relatively different forms in one function isn't a very good idea.

If you extract the handling of the forms functionality, your code would be a bit cleaner:

function sendMail($email, $contactName = null, $comment = null) 
{
    $to =  "destination@mail.com";
    $copy = "mymail@mail.com";
    $subject = "Email..."

    $email = filter_var($email, FILTER_SANITIZE_EMAIL);

    if (is_null($contactName)) {
        $message = "inputname=i_" . $email;
    } else {        
        $message = "Name: $contactName Email: $email";
        if (!is_null($comment)) {
            $message .= " Message: $comment"; 
        }
    }

    mail($to, $subject, $message);
    mail($copy, $subject, $message);
}

// in your subscribe controller:
if (isset($_POST["contactSubscribe"]) {
    sendMail($_POST['email']);
}

// in your submit controller:
if (isset($_POST["submit"]) {
    sendMail($_POST['email'], $_POST['contactName']);
}

// in your question submit controller:
if (isset($_POST["questionSubmit"]) {
    sendMail($_POST['email'], $_POST['contactName'], $_POST['comment']);
}

I would probably reduce the function even further. The building of the email messages should probably be happening in their own functions, and the sendMail function should only send the email.

Misc

  • use proper indentation.
  • When you have multiple if statements with the same arguments, leave the arguments in the same order (!is_null($contactName) && is_null($comment) vs is_null($comment) && is_null($contactName)).
  • your foreach loop seems overly complex. Why not just ask if (isset($_POST["contactName"]) { $contactName = $_POST["contactName"]; } etc?
  • use elseif for mutually exclusive if statements.
\$\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.