patternphpMinor

Is this contact form code breaking any rules?

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

thiscontactanyrulesbreakingcodeform

Problem

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?

Invalid Email Address!";
            break;

            default:
            $to = "support@loaidesign.co.uk";
            $subject = "New Message From: $name";
            $message = "Name: $name
                        Number: $number
                        Date: $date
                        Select: $select
                        Radio: $radio
                        Checkbox: $checkbox
                        Email: $email
                        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) ? "Success!" : "Error! There was a problem with sending.";
            }
         break;
         }
         echo $response;
         }
      } else {
      echo "Error";
      }
   ob_flush();
   die();
   }
?>


   ">
   

   
      Your Name
      
   

   
      Email Address
      
   

   
      Message
      
   

   
      Phone Number
      
   

   
      Date (dd/mm/yyyy)
      
   

   
      
         Select
         DropdownA
         DropdownB
      
   

   
      Radios:
      Male
      Female
   

   
      Checkboxs:
      Option A
      Option B
   

   
      
      
   

   Send

Solution

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: `. 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:


    …


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.

Invalid Email Address!

    …

Code Snippets

<?php

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

<?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>

Context

StackExchange Code Review Q#42481, answer score: 4

Revisions (0)

No revisions yet.