Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is the following PHP laid out fine to go inside the mail function?

$to = "My Name <myemail@mydomain.com>";
$subject = "Contact Form: $name";
$message = "Name: $name\r\nEmail: $email\r\nMessage:\r\n$message";
$headers = "From: Contact Form <contactform@mydomain.com>";

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

Should there be any spaces around the \r\n? Any important headers to include?

Any tips on how to improve it are welcome.

If you're interested, please the full script below. It is adapted from here — it's allegedly secure... is it?

Thank you in advance!

<?php

// Clean up the input values
foreach($_POST as $key => $value) {
  if(ini_get('magic_quotes_gpc'))
    $_POST[$key] = stripslashes($_POST[$key]);

  $_POST[$key] = htmlspecialchars(strip_tags($_POST[$key]));
}

// Assign the input values to variables for easy reference
$name = $_POST["name"];
$email = $_POST["email"];
$message = $_POST["message"];

// Test input values for errors
$errors = array();
if(strlen($name) < 2) {
  if(!$name) {
    $errors[] = "You must enter a name.";
  } else {
    $errors[] = "Name must be at least 2 characters.";
  }
}
if(!$email) {
  $errors[] = "You must enter an email.";
} else if(!validEmail($email)) {
  $errors[] = "You must enter a valid email.";
}
if(strlen($message) < 10) {
  if(!$message) {
    $errors[] = "You must enter a message.";
  } else {
    $errors[] = "Message must be at least 10 characters.";
  }
}

if($errors) {
  // Output errors and die with a failure message
  $errortext = "";
  foreach($errors as $error) {
    $errortext .= "<li>".$error."</li>";
  }
  die("<span class='failure'>The following errors occured:<ul>". $errortext ."</ul></span>");
}

// Send the email
$to = "YOUR_EMAIL";
$subject = "Contact Form: $name";
$message = "$message";
$headers = "From: $email";

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

// Die with a success message
die("<span class='success'>Success! Your message has been sent.</span>");

// A function that checks to see if
// an email is valid
function validEmail($email)
{
   $isValid = true;
   $atIndex = strrpos($email, "@");
   if (is_bool($atIndex) && !$atIndex)
   {
      $isValid = false;
   }
   else
   {
      $domain = substr($email, $atIndex+1);
      $local = substr($email, 0, $atIndex);
      $localLen = strlen($local);
      $domainLen = strlen($domain);
      if ($localLen < 1 || $localLen > 64)
      {
         // local part length exceeded
         $isValid = false;
      }
      else if ($domainLen < 1 || $domainLen > 255)
      {
         // domain part length exceeded
         $isValid = false;
      }
      else if ($local[0] == '.' || $local[$localLen-1] == '.')
      {
         // local part starts or ends with '.'
         $isValid = false;
      }
      else if (preg_match('/\\.\\./', $local))
      {
         // local part has two consecutive dots
         $isValid = false;
      }
      else if (!preg_match('/^[A-Za-z0-9\\-\\.]+$/', $domain))
      {
         // character not valid in domain part
         $isValid = false;
      }
      else if (preg_match('/\\.\\./', $domain))
      {
         // domain part has two consecutive dots
         $isValid = false;
      }
      else if(!preg_match('/^(\\\\.|[A-Za-z0-9!#%&`_=\\/$\'*+?^{}|~.-])+$/',
                 str_replace("\\\\","",$local)))
      {
         // character not valid in local part unless
         // local part is quoted
         if (!preg_match('/^"(\\\\"|[^"])+"$/',
             str_replace("\\\\","",$local)))
         {
            $isValid = false;
         }
      }
      if ($isValid && !(checkdnsrr($domain,"MX") || checkdnsrr($domain,"A")))
      {
         // domain not found in DNS
         $isValid = false;
      }
   }
   return $isValid;
}

?>




Update: I ended up using the ValidForm Builder because it has better security, great customizability, and an easy implementation — quoting from their site:

  • The API generates XHTML Strict 1.0 compliant code.
  • Field validation on the client side to minimize traffic overhead.
  • Field validation on the server side to enforce validation rules and prevent tempering with the form through SQL injection.
  • Client side validation displays inline to improve user satisfaction. No more annoying popups that don't really tell you anything.
  • Easy creation of complex form structures.
  • Uses the popular jQuery Javascript library for DOM manipulation.
  • Completely customizable using CSS.
  • Automatic creation of field summaries for form mailers in both HTML and plain text.

Quoting other benefits:

  • First of all, it's open source and therefore completely free!
  • Super fast web form creation.
  • Get rid of SQL injection problems.
  • Create standards based CSS forms. No tables inside.
  • Make form entry fun for the user. More feedback from your website.
share|improve this question
    
What do you mean by secure? The link you posted also does not define secure. –  Rob Apodaca Dec 7 '12 at 1:09
    
Well, for PHP: php.net/manual/en/security.php –  Baumr Dec 7 '12 at 12:11
add comment

2 Answers

up vote 3 down vote accepted

As far i think, your script is good and will achieve the required task. But before finalising on this script, you should read about PHPMAILER. It is eay to implement and will provide you many features like to add CC,BCC,attachments. mail() function is the easiest way, but as far as i think it is a bit limited. ALso PHPMailer is easy available on internet. Thanks.

share|improve this answer
add comment

Before writing your own mailing class, please please please try to not reinvent the wheel and save yourself some time. There are a lot of fundamental problems with this mailing script, and could be resolved by using something premade from one of the major php frameworks;

Either one of those is a testable, and extendable solution to your problem and will be much easier to implement.

share|improve this answer
    
I didn't write this mailing class — please see the link. But what problems are there with it? –  Baumr Dec 9 '12 at 23:22
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.