PHP contact form script

Question

I have created a contact form that originally did not have any mechanism for sending out the email with whatever the user inputted in the fields. I have added it in here along with ReCaptcha check. I would like to get some feedback on the code, and I am especially looking at upgrading the email body to send out emails that are styled and look better.

<?php $yourEmail = "[email protected]"; // <== Your Email $secret = 'LALALALAALALALALALALA'; // <==Your recaptcha Privte Key $errors = array(); // array to hold validation errors $data = array(); // array to pass back data // validate the variables ====================================================== // if any of these variables don't exist, add an error to our $errors array // ---------------------Start the recaptcha ------------------------------------// if(isset($_POST['g-recaptcha-response']) && ($_POST['g-recaptcha-response'])){ session_start(); $ip = $_SERVER['REMOTE_ADDR']; $captcha = $_POST['g-recaptcha-response']; $response = file_get_contents("https://www.google.com/recaptcha/api/siteverify?secret=$secret&response=$captcha&remoteip=$ip"); $result = json_decode($response,TRUE); if($result['success'] == 1){ $_SESSION['result'] = $result['success']; } // --------------------End Of the Captcha Check------------------------- // if (empty($_POST['name'])) $errors['name'] = 'Name is required.'; if (empty($_POST['email'])) $errors['email'] = 'Email is required.'; if (empty($_POST['phone'])) $errors['phone'] = 'Phone is required.'; // ---------------------Start the recaptcha ------------------------------------// if(!isset($_SESSION['result']) || $_SESSION['result'] == 0){ $formerrors[] = 'Captcha Error'; } // --------------------End Of the Captcha Check------------------------- // // return a response =========================================================== // if there are any errors in our errors array, return a success boolean of false if ( ! empty($errors)) { // if there are items in our errors array, return those errors $data['success'] = false; $data['errors'] = $errors; } else { // if there are no errors process our form, then return a message // DO ALL YOUR FORM PROCESSING HERE // THIS CAN BE WHATEVER YOU WANT TO DO (LOGIN, SAVE, UPDATE, WHATEVER) // show a message of success and provide a true success variable $data['success'] = true; $data['message'] = 'Success!'; } // return all our data to an AJAX call echo json_encode($data); //Start of Sending Email $to = $yourEmail; // Email to receive contacts $from = $email; $subject = 'Contact Form Email : ' . $title; $message = '<style> body{background-color:#fefefe} .email-style {padding: 30px;background: #fafafa;font-size: 18px;border: 1px solid #ddd;width: 60%;margin: auto;} p {padding: 15px 0px;} </style> <div class="email-style"><p> '.$title . '</p> <p>Contact Full Name : '.$name . ' </p> <p>Contact Email : '.$email . ' </p> <p>Contact Phone Number : '.$phone . '</p> <p>Message : '.$message . ' </p> <p>Cheers,</p> <p>'.$name.' Via Contact Form</p></div>'; $headers = "From: $from\n"; $headers .= "MIME-Version: 1.0\n"; $headers .= "Content-type: text/html; charset=iso-8859-1\n"; if( mail($to, $message, $headers) ){ echo "sent"; session_unset(); session_destroy(); } else { echo "The server failed to send the message. Please try again later."; } } ?> 

Answer 1

I would personally consider the following in order to improve your PHP scripting:

1. You are checking the $_POST values in your script like this:

   if (empty($_POST['name'])) $errors['name'] = 'Name is required.'; 

   Whereas, further down, you have this:

   if(!isset($_SESSION['result']) || $_SESSION['result'] == 0){ $formerrors[] = 'Captcha Error'; } 

   It's perfectly fine not to wrap single-lined if conditions with braces, but if you're going to do it that way, don't wrap single-lined if conditions with braces in the same script or project. This is simply for consistency reasons. Choose one or the other and stick to it.

2. In a similar fashion, use consistent spacing throughout your project, for instance:

    <p>'.$name.' Via Contact Form</p></div> 

   has no spaces between the . to concatenate your strings, but just above, you have this:

   $subject = 'Contact Form Email : ' . $title; 

   Again, choose one and stick to it. Same with opening brackets/braces, Consistency is readability.

3. Don't over-comment. If you have to use in-line comments then your script isn't clear enough. Therefore, each time you feel you need to add a comment, or several comments, consider putting the proceeding logic into a function or a method. For instance, you could have a function (as you're using procedural scripting) called checkRecaptcha(), like this (this is a very quick refactor and adds in a check to see if your file_get_contents() works as well):

   function checkRecaptcha(){ $ip = $_SERVER['REMOTE_ADDR']; $captcha = $_POST['g-recaptcha-response']; $response = file_get_contents("https://www.google.com/recaptcha/api/siteverify?secret=$secret&response=$captcha&remoteip=$ip"); $result = !empty($response) ? json_decode($response, TRUE) : array(); if(isset($result['success']) && $result['success'] == 1){ $_SESSION['result'] = $result['success']; return true; } return false; ) 

   I've added in a sanity check to the $response variable and therefore stopped some PHP warnings in your error logs.

   Using methods/functions, the start of your script is much simpler and more readable, because you've given the function a meaningful name (meaningful names reduce the need for comments), and becomes:

   if(isset($_POST['g-recaptcha-response']) && ($_POST['g-recaptcha-response'])){ session_start(); if(checkRecaptcha() === false){ // Recaptcha didn't work so we can stop here exit; } // Rest of PHP } 

   I've added comments (but would not usually do so) just for readability. Generally, I would throw an Exception rather than exiting, and catch the Exception further up the heap. But that is for another time.

4. Where you have repeating string literals, you could define them at the top of your script, or (as I do), in a flat Library file. That means that you will only need to change something once in your definition, and it will change everywhere. This is handy if you make a typo, things are easier to trace. See the PHP documentation for define() for details.