Can you review my code please?

[LearningCoder]

Hello.

I think I have completed the validation of my code and was wondering if anyone can kindly give my any criticism as to anything I have missed or anything I can do better, which I'm sure both will generate some posts.

There are little things I want to tweak but I wanted to know what some of the pros think (please go easy, im rubbish).

Here is my contact.template.htm:

PHP Code:

<p id="contact_intro">It is a long established fact that a reader will be distracted by the readable content of a page when looking at its 
                      layout. The point of using Lorem Ipsum is that it has a more-or-less normal distribution of letters. It is a long 
                      established fact that a reader will be distracted by the readable content of a page when looking at its layout. 
                      The point of using Lorem Ipsum is that it has a more-or-less normal distribution of letters</p>
                      
<form method="post" action="index.php?page=contact">
     <fieldset>
         <legend>Gardenable Contact Form</legend>
         
             <p class="form_heading">Your Details</p>
             <p class="form_instructions">Please leave us your details so we can contact you back!</p>
             <hr class="form_hr" />
             <p><label for="name">Name:</label><input type="text" name="name" id="name" size="36" maxlength="36" /><span class="red">*</span></p> 
             <p><label for="email">Email:</label><input type="text" name="email" id="email" size="36" maxlength="70" /></p> 
             <p><label for="phone">Phone:</label><input type="text" name="phone" id="phone" size="36" maxlength="16" /><span class="red">*</span></p>
             <p><label for="user_comments">Additional Comments:</label><textarea name="user_comments" id="user_comments" rows="5" cols="34" maxlength="400"></textarea></p>
             
             <hr />
         
             <p class="form_heading">Product Details</p>
             <p class="form_instructions">If you wish to <span class="italic">order</span> or <span class="italic">query</span> a product, please specify below.</p>
             <hr class="form_hr" />
             
             <p><label for="product">Product:</label>
                 <select name="product_options">
                 <option value="default">Choose a product...</option>
                     <option value="benches">Benches</option>
                     <option value="bin_stores">Bin Stores</option>
                     <option value="bird_housing">Bird Housing</option>
                     <option value="gates">Gates</option>
                     <option value="pet_housing">Pet Housing</option>
                     <option value="planters">Planters</option>
                     <option value="sheds">Sheds</option>
                     <option value="tables">Tables</option>
                 </select>
             </p>
             <p><label for="product_ref">Product ID:</label><input type="text" name="product_ref" id="product_ref" size="20" maxlength="7" />
             <p><label for="product_comments">Product Comments:</label><textarea name="product_comments" id="product_comments" rows="5" cols="34" maxlength="400"></textarea></p>
             
             <p><input type="submit" name="submit" value="Submit" />
                <input type="reset" name="reset" value="Reset" />
             </p>
             <span id="form_required">Fields marked with a red asterix (<span class="red">*</span>) are required.</span>
     </fieldset>
</form>

<div id="error_div">
     <?php if(isset($output)){ print_r($output);} ?>
</div>

Here is validation relating to it:

PHP Code:

if(isset($_POST['submit'])){
         
     //if script gets here, the user submitted the form. delete last element (submit button) as we do not need it.
     array_pop($_POST);
     
     //create array to hold any errors.
     $errors = array();
    
     //firstly, check to see if my required fields contain any data. if they dont we add errors to the error array.
     if(empty($_POST['name']) || empty($_POST['phone'])){
         $errors[] = "You must fill in the required fields marked with a RED asterix(*).";
     }

     //check to see if the errors array contains anything. if it does, we need to send the user back to the form and display the error.
     //do not carry on if the if statement executes because we dont want to process any more as we know we are going to have to send them back anyway.
     if(!empty($errors)){
         $output = $errors;
     }
     else{
     //if the code reaches here, we have data inside the two required fields so carry on processing all of the data now.
     //pass a reference of the value so that if any ARE set to string NULL, it also changes the original $_POST value.
     foreach ($_POST as $post => &$value) {
         if($value == ""){
             $value = "NULL";
         }
         else{
             switch ($post) {
                 
                 case "name": 
                     if(!ctype_alpha($value)){
                         $errors[] = "The name field can only contain alphabetical characters.";//specify just a first name in form
                     }
                 break;
                 
                 case "email": 
                     if(!filter_var($value,FILTER_VALIDATE_EMAIL)){
                         $errors[] = "You did not enter a valid email address.";//give an example of an email someone@provider.com in form
                     }
                 break;
                 
                 case "phone":
                     //replaces all characters that are NOT digits 0-9.
                     $value = preg_replace("/\D/","",$value);
                     
                     //we need to check if it is not equal to an empty string again because if they entered all letters, the preg_replace will replace them
                     //and my second if statement here will show an undefined index error. if it is an empty string, add to error array and break out of case
                     //prematurely.
                     if($value == ""){ $errors[] = "You did not enter a phone number."; break;}
                     
                     //checks to see if the first character of the string is not equal to a 0 or if the length of the string isn't 11 (which means its not valid).
                     if($value[0] != "0" || strlen($value) != 11){
                          $errors[] = "You did not enter a valid phone number.";
                     }
                 break;
                 
                 case "user_comments":
                     $len = strlen($value);
                     
                     if ($len > 400){
                          $less = ($len - 400);
                          $errors[] = "You must enter {$less} LESS characters in the 'Additional Comments' field.";
                     }
                 break;
                 
                 case "product_options":
                     //not sure how to validate the <select> options. dont think I need to as it will always be 'default' or a product name.
                     
                 break;
                 
                 case "product_ref":
                     
                     //checks to see if the length of the string is not equal to 7
                     if(strlen($value) != 7) {
                        $errors[] = "The product id you entered was not long enough, must be 7 numbers.";
                         
                     }
                     //checks to see if any of the characters entered were not digits. if this executes, we know that the user entered something different
                     //than 7 digits so there is no need to carry on and check the ref no against the records so we break out of case prematurely.
                     if(!ctype_digit($value)){
                        $errors[] = "Product id's can only contain numbers.";
                        break;
                     }
                     
                     //prepared statement which checks the product ref no submitted against a product ref in the database. 
                     require("core/prepared_select_pref.php");
                     
                     if($row != 1){
                         $errors[] = "Your Product ID did not match one of our products.";
                     }
                     
                 break;
                 
                 case "product_comments":
                     $len = strlen($value);
                     
                     if($len > 400){
                         $less = ($len - 400);
                         $errors[] = "You must enter {$less} LESS characters in the 'Product Comments' field.";
                     }
                 break;
             }
         }
         
     }
     }
     
     //if the error array contains data, we had some errors during validation, so we display all of these error(s) to the user.
     if (!empty($errors)){
        
        $output = "<ul>";
            foreach ($errors as $err => $error_value){
                $output .= "<li>".$error_value."</li>";
                $output .= "<hr>";
            }
        $output .= "</ul>";
     }
     else{//if there were no errors after all the validation, insert data to database.
        require("core/prepared_insert.php");
        if($row >= 1){
            $output = "Your information has successfully sent!";
        }
        else{
            //maybe send their information to my email instead if there is an issue with insert....probably the best idea rather than displaying an error.
            $output = "There was an error receiving your information.";
        }
     }
     
} 


Thanks very much for any help/tips you can give me.

Kind regards,

LC..

[LearningCoder]

Really, no one willing to give it a brief scan to see if I've missed anything?

I've never actually built a robust script, just want opinions. I've tried entering crap into the form every single possible way and my errors seem to deal with them all.

Regards,

LC.

[tangoforce]

I've quickly scanned it and stopped on the very first line.

I'll let you work it out from my sig - I've replied to your threads more than enough times for you to have seen it

[LearningCoder]

Aha been a time!

You have indeed! I'll give your sig a read.

Kind regards,

LC.

[LearningCoder]

Ok just read your topic in your signature. Notice you check for other form fields and not the submit button. That seems straight forward enough for me. I also noticed Fou stated we should check for every single field (which is what I have done in my other scripts for that demo site when I first started it). Think I might go with your solution of checking another field is set for example my $_POST['name'] field.

I'm thinking of slightly modifying my code to this:

PHP Code:

if (isset($_POST['name'])){
    //start my processing
    (isset($_POST['submit'])) ? array_pop($_POST['submit']) : ""; 


} 


If you relate that to the code above to my original code post, do you think this will now cover all eventualities?

Edit: Just changed my code to what I wrote above and it works great.

I'm actually on XP so I can't even get IE9, which in my opinion is an absolute joke. I have an MS system which can't run the default MS browsers latest version, what kinda crap are they pulling? Anyway the highest version I have is 8. I'll play around with the submit thing and see the result I get to gain a better understanding.

Plus, would you be willing to just briefly scan my code and see what you think? It seems a little basic to me and I feel there is more I can do to secure it. Like I've said I've tried entering crap into the fields every way possible and my processing seems to catch every possible error. So I know that I'm on the way to having a good script, I just want some opinions on how to improve it.

It works as I want so it's more of a 'you could do this' or change that foreach loop etc type of answer im looking for. Had over 100 views with 1 reply....bad times. Is reviewing completed code something that isn't done here I think I've seen on other forums a section for 'code critique'?

Thanks for your reply!

Regards,

LC.