First WordPress template

Question

I wrote my first template, but didn't include any styling. What do you think? What's good what's bad?

 <?php
/*
Template Name: Email product info
*/


//checks required fields
$allFormPosts = array("address", "email", "xsize", "ysize", "zsize");
foreach($allFormPosts as $formPost){
    if(isset($_POST[$formPost])){
        ${$formPost} = trim($_POST[$formPost]);
    }else{
        $formNotValid = true;
        break;
    }
}

if(!isset($formNotValid)){
    //checks address
    if(!strlen($address)){
        $hasError = true;
        $addressError = "You forgot to enter address.";
    }
    //checks email
    if(!strlen($email)){
        $hasError = true;
        $emailError = "You didn't enter email address";
    }
    else if(!is_email($email)){
        $hasError = true;
        $emailError = "Email address is not valid.";
    }
    //check x
    if(!strlen($xsize)){
        $hasError = true;
        $xsizeError = "You forgot to enter \"x\" size.";
    }
    else if(!is_numeric($xsize) || $xsize <= 0){
        $hasError = true;
        $xsizeError = "\"$xsize\" is not valid.";
    }
    //checks y
    if(!strlen($ysize)){
        $hasError = true;
        $ysizeError = "You forgot to enter \"y\" size.";
    }
    else if(!is_numeric($ysize) || $ysize <= 0){
        $hasError = true;
        $ysizeError = "\"$ysize\" is not valid.";
    }
    //checks z
    if(!strlen($zsize)){
        $hasError = true;
        $zsizeError = "You forgot to enter \"z\" size.";
    }
    else if(!is_numeric($zsize) || $zsize <= 0){
        $hasError = true;
        $zsizeError = "\"$zsize\" is not valid.";
    }
    //sends email if form is valid
    if(!isset($hasError)){
        $subject = "Package";
        $body = "Package is being sent to $address. Package size is $xsize-$ysize-$zsize cm";
        mail($email, $subject, $body);
    }
}
?>


<?php get_header(); ?>

<form action="<?php the_permalink(); ?>" id="productForm" method="post">

<?php
if(isset($hasError) || isset($formNotValid))
    echo "<p>There has been an error.</p>";
?>
    <table>
        <tr>
            <td>Address*:</td>
            <td><input type="text" name="address" value="<?php if(isset($address) && !isset($addressError)) echo $address;?>" required="required" placeholder="Enter place">
                <?php if(isset($addressError)){
                    echo $addressError;
                }
                ?>
            </td>
        </tr>

        <tr>
            <td>Email*:</td>
            <td><input type="email" name="email" value="<?php if(isset($email) && !isset($emailError)) echo $email;?>" required="required" placeholder="Enter email">
                <?php if(isset($emailError)){
                    echo $emailError;
                }
                ?>
            </td>
        </tr>

        <tr>
            <td>Size*:</td>
            <td><input type="number" size="3" placeholder="x" name="xsize" required="required" min="1" value="<?php if(isset($xsize) && !isset($xsizeError)) echo $xsize;?>">
                <input type="number" size="3" placeholder="y" name="ysize" required="required" min="1" value="<?php if(isset($ysize) && !isset($ysizeError)) echo $ysize;?>">
                <input type="number" size="3" placeholder="z" name="zsize" required="required" min="1" value="<?php if(isset($zsize) && !isset($zsizeError)) echo $zsize;?>">cm
                <?php if(isset($xsizeError)){
                    echo $xsizeError;
                }
                if(isset($ysizeError)){
                    echo $ysizeError;
                }
                if(isset($zsizeError)){
                    echo $zsizeError;
                }
                ?>
            </td>
        </tr>
   </table>
    <button type="submit" value="submitted">Submit</button>
</form>

<?php get_footer(); ?>

Answer 1

It's quite fine. Something I don't like though is the mild overuse / abuse of isset and strlen. Take this code for example:

$allFormPosts = array("address", "email", "xsize", "ysize", "zsize");
foreach($allFormPosts as $formPost){
    if(isset($_POST[$formPost])){
        ${$formPost} = trim($_POST[$formPost]);
    }else{
        $formNotValid = true;
        break;
    }
}

if(!isset($formNotValid)){

The main purpose of isset is to check if something was set at all, and is best used to check things not within your control. isset($_POST[$formPost]) is an appropriate use case: indeed, you have no control over what was sent to your script, whether from a proper user input form, or possibly from a curl script.

On the other hand, you do have control over $formNotValid; it's your own variable you defined yourself, and you should use it better. This variable should be a boolean: it should be either true or false, there is no point in permitting "not set".

It would be better to initialize this variable yourself, and eliminate the possibility of it not being set. Initialize it to false, check your form with the loop as before, updating $formNotValid as needed, and then do a boolean check instead of an isset:

$formNotValid = false;
foreach($allFormPosts as $formPost){
    // ...
}

if(!$formNotValid){

You cannot know what scripts get included before your script is called. Some other script could possibly set a value to $formNotValid, breaking your script. By initializing it explicitly to a meaningful value yourself, you eliminate an ambiguity, and with it a bug waiting to happen.

I recommend the same treatment for $hasError as well. Initialize it first to false, and check it with if ($hasError) { ... } instead of resorting to isset.

---

Although it's true that strlen should always return 0 or higher, it would be more natural to write conditions on strlen like this:

// this is kind of "clever"
//if(!strlen($address)){

// this is more natural
if (strlen($address) == 0) {

This may be a matter of taste though.

---

Borrowing from the standard in other languages, I recommend using more spaces in loops and conditionals, like this, notice the difference from the original:

foreach ($allFormPosts as $formPost) {
    if (isset($_POST[$formPost])) {
        // ...
    } else {
        // ...
    }
}

Answer 2

XSS

XSS via Post is possible, so you shouldn't just echo $address in the form, and you should also not put the invalid input unsanitized in the error message ($xsize, etc).

Validation

You check strlen on every input field. You can remove all those checks and just move the strlen checks to your foreach loop (you can still set the error messages there).

Misc

• comments: your checks [something] comments are not really needed, it's obvious from the code what is happening.
• I'm not an HTML expert, but as far as I can tell, using a table to format a form is a bit outdated (see here and here for example). A better approach would be to use label.
• you have formNotValid and hasError, but the names don't really tell a reader what the difference between them is. I would just get rid of hasError and use formNotValid in all cases.