2
\$\begingroup\$

I've been more than likely doing this entirely wrong for quite awhile and would like to correct that.

With the code review I would like to get critiqued on everything. This includes:

  • JQuery syntax/structuring
    • Additionally, if there are different/better/newer libraries let me know
  • HTML5 syntax/structuring
  • PHP structuring when it comes to sending and validating data
    • What would be great is if I could structure it in a way to run unit tests
  • If I should be validating data in a different way all together

TML:

<!DOCTYPE HTML>

<html lang="en">

<head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="description" content="">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Contact</title>

    <!-- Disable tap highlight on IE -->
    <meta name="msapplication-tap-highlight" content="no">

    <!-- Color the status bar on mobile devices -->
    <meta name="theme-color" content="#2F3BA2">

    <!-- Scripts -->
    <script src="https://code.jquery.com/jquery-1.11.1.min.js"></script>
    <script src="https://cdn.jsdelivr.net/jquery.validation/1.15.0/jquery.validate.min.js"></script>
    <script src="https://cdn.jsdelivr.net/jquery.validation/1.15.0/additional-methods.min.js"></script>
    <script src="js/contact.js"></script>
</head>

<body>

    <header>
        <nav>
            <ul>
                <li>Menu Item</li>
            </ul>
        </nav>
    </header>

    <section>
        <h1>Contact Us</h1>
        <p>Please answer a couple of short questions</p>

        <p id="error-msg"></p>
        <form id="email-form" action="email.php" method="POST">
            <input type="text" 
                name="name" 
                pattern="[a-zA-Z\s\-\.]+" 
                placeholder="Name" 
                title="Please only use letters and spaces"
                aria-required="true" 
                required/><br />

            <input type="email" 
                name="email" 
                placeholder="Email address" 
                aria-required="true" 
                required/><br />

            <input type="tel" 
                name="phone" 
                pattern="(?:\(\d{3}\)|\d{3})[- ]?\d{3}[- ]?\d{4}" 
                placeholder="Phone number" 
                title="Example: 123-123-1234" 
                aria-required="true" 
                required/> <br />

            <input type="text" 
                name="business" 
                placeholder="Name of business" 
                aria-required="true" 
                required/><br />

            <input type="text" 
                name="project" 
                placeholder="Tell us about you and your project" 
                aria-required="true" 
                required/><br />

            <select name="budget">
              <option value="" disabled selected>What is your budget?</option>
              <option value="less-than-1000">Less than $1000</option>
              <option value="from-1000-1500">$1000-$1500</option>
              <option value="from-1600-2000">$1600-$2000</option>
              <option value="more-than-2000">More than $2000</option>
            </select><br />

            <input type="text" 
                name="audience" 
                placeholder="Who is your target audience?" 
                aria-required="true" 
                required/><br />

            <input type="submit" value="Submit">
        </form>
    </section>


    <footer>
        <p>Copyright 2016 Me</p>
    </footer>

</body>
</html>

I was planning on moving everything above the opening section> tag into a "header.php" file and including it and everything below the closing </section> tag into a "footer.php" file and including it. Just a thought.

contact.js:

$( document ).ready(function() {

    /*****************************************************
     *                Email Form Submission              *
     *****************************************************/

    $("#email-form").submit( function(e) {
        e.preventDefault();
        var $form = $(this);

        $form.validate();

        // check if the input is valid
        if(! $form.valid()) return false;

        //if valid post to email.php
        $.ajax({
            type: "POST",
            url: "email.php",
            data: {
                'name': $('[name="name"]').val(),       
                'email': $('[name="email"]').val(), 
                'phone': $('[name="phone"]').val(),
                'business': $('[name="business"]').val(),
                'project': $('[name="project"]').val(),
                'budget': $('[name="budget"]').val(),
                'audience': $('[name="audience"]').val()
            },
            success: function(data) {
                data = JSON.parse(data);

                //If emailed successfully by backend, replace form with success message
                if( data["success"] == true ) { 
                    $form.html("<h3>Successfully submitted! We'll get back to you soon!</h3>");
                } 
                //If error occurred with any of the fields, put in error message
                else {
                    $('#error-msg').html("Please fix the follow fields:<br />" + data["error"].join("<br />"));
                }
            },
            error: function(jqXHR, textStatus, errorThrown) {
                $form.html("<center><h3>Oh no! :( Something happened</h3>Please let us know at [email protected]</center>");
            }
        });//EO ajax
    });//EO submit
});

I'm not sure if I should have a p tag for an error, but let me know.

email.php:

<?php

//email variables
$to = "[email protected]";
$body = "";
$subject = "";
$headers = "From: [email protected]";

//form fields
$name   = filter_var($_POST['name'], FILTER_SANITIZE_STRING);
$email  = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
$phone  = preg_replace('/[^0-9]/', '', $_POST['phone']);
$business = filter_var($_POST['business'], FILTER_SANITIZE_STRING);
$project = filter_var($_POST['project'], FILTER_SANITIZE_STRING);
$budget = $_POST['budget'];
$audience =filter_var($_POST['audience'], FILTER_SANITIZE_STRING);


/********************************************
 * Check that all fields are filled out     *
 ********************************************/
$errFields = array();

if($name == "") {
    array_push($errFields, "name");
}

if($email == "") {
    array_push($errFields, "email");
}

if($phone == "") {
    array_push($errFields, "phone");
}

if($business == "") {
    array_push($errFields, "business");
}

if($project == ""){
    array_push($errFields, "project");
}

if($budget == "") {
    array_push($errFields, "budget");
}

if($audience == "") {
    array_push($errFields, "audience");
}


//If something went wrong
if($errFields) {
   echo json_encode(array("success" => False, "error" => $errFields)); 
}
//Send inquiry information to me and response to sender
else {
    /************************
     * Send email to me     *
     ************************/
    $subject = "New Inquire from $business";

    // the message
    $body = <<<EOT
  NAME: $name
  EMAIL: $email 
  PHONE: $phone
  BUSINESS: $email
  PROJECT: $project
  BUDGET: $budget
  AUDIENCE: $audience
EOT;

    // send email to myself
    mail($to, $subject, $body, $headers);


    /************************
     * Send email to sender *
     ************************/
    $to = $email;
    $subject = "Thank you!";
    $body = <<<EOT
 Hi $name,

 Thank you for your recent message!

 We look forward to speaking with you soon,
 Beckah
EOT;

    mail($to, $subject, $body, $headers);
    echo json_encode(array("success" => True));
}


?>

I know my PHP needs a lot of work, so I would love input on how to structure this correctly. Please let me know of any structuring, syntax, or best practices I could change anywhere in my code.

\$\endgroup\$
1
  • \$\begingroup\$ Just a note: I see in the edit revisions that you removed some information that might be personal. All revisions of a post are public, if this is an issue you might be able to contact one of the site's moderators in our chatroom. \$\endgroup\$
    – jacwah
    Commented Nov 1, 2016 at 20:55

1 Answer 1

4
\$\begingroup\$

Generally, I think this code looks really good for a pretty basic example of web form handling, particularly for someone who may be earlier in learning process. I think you have really easy to read code, with good use of whitespace, indentation, breaking code across lines, and similar.


From a security standpoint, you should get in the habit of always embedding session tokens in hidden form fields so you can validate that an incoming request is coming from a user with a valid session as opposed to some automated post. This is known as a cross-site request forgery (CSRF) attack. You should be able to find good example for how to prevent these attacks from Google search.


I think you are missing an opportunity in only validating your form at submit time. You are using an excellent jQuery validation plug-in that will let you do things like validate individual fields when their values change, giving more immediate feedback to the user around validation errors. I also think you may not be taking full advantage on the configuration options that this plug-in gives you such that you might better separate your validation logic/configuration from your static HTML.


Consider moving contact.js include to footer, since all activity there is going to be executed on document ready anyway. This may help slightly with apparent rendering speed of page to browser. The browser could be using a thread to load an image or similar static asset that is more visible to the end user rather than blocking on retrieval of this script (browsers typically download all static assets based on X number of subrequest threads).


    // check if the input is valid
    if(! $form.valid()) return false;

Here where you check to see if form is valid, you do nothing other than return false. How to you perform user messaging as to the validation failure?


Since you are just POSTing url-encoded query string, can you not simplify setting of data for AJAX call to:

data: $(form).serialize(),

data = JSON.parse(data);

If you set your dataType .ajax() option to json this parsing step can happen for you automatically.


if( data["success"] == true ) { 

Try to avoid loose comparisons like this. Here values like 1, 'true', true','1'` would all evaluate truthily. What value are you ACTUALLY expecting from your API? Be specific.


            else {
                $('#error-msg').html("Please fix the follow fields:<br />" + data["error"].join("<br />"));
            }

Are you sure this is reasonable error messaging? If you get to this state, then you likely have incompatible rules defined between the client and the server. Just listing the names of the fields that failed on the server may not be useful for the user in understanding how to address the problem in the client (or can they?). Is this even a recoverable error from a user standpoint? Perhaps an error like shown in the ajax error handler is more appropriate.


I like you use of filter_var. You might want to consider filter_input_array() here instead. This might allow you to more easily decouple the validation configuration/definitions from the actual validation step. For example, if you add a new field down the line, you can just change validation config instead of adding a new line of code to check that new field.

This would also allow you to potentially skip the steps where build an array of errors.

Usage:

$validation_config = [
    'name' => ...
    ...
];

$filtered_input = filter_input_array(INPUT_POST, $validation_config, true);

$inputs_with_errors = array_filter($filtered_input, function($value) {
    return ($value === false);
});

if(count($inputs_with_errors) > 0) {
    echo json_encode([
        "success" => false,
        "errors" => array_keys($inputs_with_errors)
    ]);
    exit;
}

// logic to validate form token
if($filtered_input['form_token'] !== $_SESSION['form_token']) {
    // perhaps send error message and stop script.
}

// start mailing here

Be careful about exiting the script after you have made your JSON output. There way your code is written now, you might introduce spurious output after the code is executed. A god example of this is on validation error condition. You should immediately exit the script after rendering the JSON. There is no reason to keep evaluating code after this point. That also totally eliminates the need for the entire else block that takes place after it, simplifying the code.


Consider working with a more reliable mailer than PHP's mail() function, which is notoriously bad in terms of generating useful errors and in terms of configuration. As it stands right now, you are not even handling the error path if mail() fails.


To your comments around unit testing...

I don't think this section of code is a good candidate for that. Typically, when you think of unit testing, you are thinking of unit testing the various methods and functionalities of objects or interfaces in your system. This code is simple procedural code, which does not lend itself to easy unit testing. You would, in fact, probably need to actually complicate this code to a certain degree to make it unit testable. That might not be worth investing time into. On the other hand, if you could envision yourself building out more fully featured server-side validation classes or similar, you would then probably be interested in also adding unit tests into the mix.

\$\endgroup\$
1
  • \$\begingroup\$ @beckah Consider leaving the answer unaccepted for some period of time to help get additional commentary. \$\endgroup\$
    – Mike Brant
    Commented Nov 1, 2016 at 21:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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