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.

Can you please review my PHP script below? It is for a contact form. Am I breaking any rules? Does it seem okay to you?

<?php
   session_start();

   if ($_SERVER['REQUEST_METHOD'] == 'POST'){
      ob_start();

      if(isset(
         $_REQUEST['name'],
         $_REQUEST['email'],
         $_REQUEST['message'],
         $_REQUEST['number'],
         $_REQUEST['date'],
         $_REQUEST['select'],
         $_REQUEST['radio'],
         $_REQUEST['checkbox'],
         $_REQUEST['token'] )){

         if($_SESSION['token'] != $_POST['token']){ $response = "0";
         } else {
            $_SESSION['token'] = "";
            $name = $_REQUEST['name'];
            $email = $_REQUEST['email'];
            $message = $_REQUEST['message'];
            $number = $_REQUEST['number'];
            $date = $_REQUEST['date'];
            $select = $_REQUEST['select'];
            $radio = $_REQUEST['radio'];
            $checkbox = $_REQUEST['checkbox'];

            switch (true){
            case !filter_var($email, FILTER_VALIDATE_EMAIL):
                  $response = "<p style='color:red'>Invalid Email Address!</p>";
            break;

            default:
            $to = "support@loaidesign.co.uk";
            $subject = "New Message From: $name";
            $message = "Name: $name<br/>
                        Number: $number<br/>
                        Date: $date<br/>
                        Select: $select<br/>
                        Radio: $radio<br/>
                        Checkbox: $checkbox<br/>
                        Email: $email<br/>
                        Message: $message";

            $headers  = 'MIME-Version: 1.0' . "\r\n";
            $headers .= 'Content-type: text/html; charset=utf-8' . "\r\n";
            $headers .= 'From: '.$email . "\r\n";
            $mailed = (mail($to, $subject, $message, $headers));

            if( isset($_REQUEST['ajax'])){ $response = ($mailed) ? "1" : "0";
            }
            else{ $response = ($mailed) ? "<h2>Success!</h2>" : "<h2>Error! There was a problem with sending.</h2>";
            }
         break;
         }
         echo $response;
         }
      } else {
      echo "Error";
      }
   ob_flush();
   die();
   }
?>


<?php

       $token = md5(uniqid(rand(), TRUE));
       $_SESSION['token'] = $token;
    ?>

<!--Contact Form-->
<form id="contactForm" name="contactForm" action="contact.php"  method="post">
   <input name="token" type="hidden" value="<?php echo $token; ?>">
   <input name="ajax" type="hidden" value="1">

   <div class="name">
      <p>Your Name</p>
      <input name="name" class="required" autocomplete="off">
   </div>

   <div class="email-address">
      <p>Email Address</p>
      <input name="email" class="required email" autocomplete="off">
   </div>

   <div class="message">
      <p>Message</p>
      <textarea name="message" rows="5" class="required min3"></textarea>
   </div>

   <div class="number">
      <p>Phone Number</p>
      <input name="number" class="number" autocomplete="off">
   </div>

   <div class="date">
      <p>Date <small>(dd/mm/yyyy)</small></p>
      <input name="date" class="required date calendar" autocomplete="off">
   </div>

   <div class="dropdown">
      <select name="select" class="required">
         <option value="">Select</option>
         <option value="DropdownA">DropdownA</option>
         <option value="DropdownB">DropdownB</option>
      </select>
   </div>

   <div class="radio">
      <p>Radios:</p>
      <label><input name="radio" type="radio" value="male" class="required">Male</label>
      <label><input name="radio" type="radio" value="female" class="required">Female</label>
   </div>

   <div class="checkbox">
      <p>Checkboxs:</p>
      <label><input name="checkbox" type="checkbox" value="OptionA" class="required">Option A</label>
      <label><input name="checkbox" type="checkbox" value="OptionB" class="required">Option B</label>
   </div>

   <div>
      <p></p>
      <input name="" class="required number spamcheck">
   </div>

   <button id="submit" type="submit">Send</button>
</form>
share|improve this question

1 Answer 1

Using the token for CSRF protection and spam prevention is a good idea, especially for a form that will be generating e-mail.

This is puzzling: <input name="ajax" type="hidden" value="1">. If a very basic, non-JavaScript-enabled user agent submits the HTML form, it will have ajax=1 as a parameter. I'd expect such a parameter to be set not by the HTML form, but by the JavaScript code that composes the AJAX request. Even then, it's customary for the JavaScript to set an X-Requested-With header instead of an ajax parameter.

From a zoomed out view, the structure of the program is:

<?php

    if () {
        ob_start();
        if () {
            if () {
            } else {
                switch (true) {
                    blah:
                    default:
                    // The core functionality lives here.
                }
            }
        }
        ob_flush();
        die();
    }
?>
<form>
    …
</form>

That's a lot of nesting, and it's hard to see at a glance how to reach the core functionality. Also, I'm not a fan of ob_start(); …; ob_flush(); die(); — any kind of killing, even if it's suicide, is a bad idea for server-side code. Furthermore, that switch is a weird way to write an if-else.

Instead, I suggest wrapping the core functionality and other tests within a functions.

<?php
    function isCompletePost() {
        // Check for a POST containing the CSRF-prevention token
        // and the expected content fields.  Return TRUE or FALSE.
    }

    function hasValidEmailAddress() {
        // Check for valid $_REQUEST['email']
        // Return TRUE or FALSE
    }

    function sendEmail() {
        // Core functionality here.  Compose mail based on $_REQUEST
        // fields and send it.
    }

    if (isCompletePost()) {
        if (!hasValidEmailAddress()) {
            ?><p style='color:red'>Invalid Email Address!</p><?php
        } else {
            sendEmail();
        }
    } else {
?>
<form>
    …
</form>
<?php
    }
<?php>
share|improve this answer

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.