Critique my code please. Say nasty things I don't care.

[OldWest]

Hello,

My script below IS finally working, but I was hoping for some aggressive, anal comments for critique.

 

Keep in mind, I am developing for a php4 platform otherwise I would have used a newer php5 validation function.

 

<?php
  if (isset($_POST['btnSubmit'])) {
      $first_name = mysql_real_escape_string($_POST['fname']);
      $last_name = mysql_real_escape_string($_POST['lname']);
      $title = mysql_real_escape_string($_POST['title']);
      $company = mysql_real_escape_string($_POST['company']);
      $address1 = mysql_real_escape_string($_POST['address1']);
      $address2 = mysql_real_escape_string($_POST['address2']);
      $city = mysql_real_escape_string($_POST['city']);
      $zip = mysql_real_escape_string($_POST['zip']);
      $phone = mysql_real_escape_string($_POST['phone']);
      $fax = mysql_real_escape_string($_POST['fax']);
      $email = mysql_real_escape_string($_POST['email']);
      
      if (!preg_match("/^[A-Za-z' -]{1,75}$/", $first_name)) {
          $error[] = "Please enter a valid first name.";
      }
      
      if (!preg_match("/^[A-Za-z' -]{1,75}$/", $last_name)) {
          $error[] = "Please enter a valid last name.";
      }
  
  if ($first_name === $last_name && $first_name != "") {
          $error[] = "First Name and Last Name cannot be the same.";
      }
      
      if (!preg_match("/^[A-Za-z' -]{1,150}$/", $company)) {
          $error[] = "Please enter a valid company name.";
      }
      
      if (!preg_match("/^[A-Za-z' -.]{1,150}$/", $title)) {
          $error[] = "Please enter a valid Title.";
      }
      
      if (!preg_match("/^[A-Za-z0-9' - . ]{1,150}$/", $address1)) {
          $error[] = "Please enter a valid mailing address.";
      }
  
  if (!preg_match("/^[A-Za-z0-9' - . ]{1,150}$/", $city)) {
          $error[] = "Please enter a valid city.";
      }
  
  if (!preg_match("/^[0-9' - . ( ) ]{1,150}$/", $phone)) {
          $error[] = "Please enter a valid phone number.";
      }
  
  if (!preg_match("/^[0-9' - . ( ) ]{1,150}$/", $fax)) {
          $error[] = "Please enter a valid fax number.";
      }
      
      if (!preg_match("/([a-z][a-z0-9_.-\/]*@[^\s\"\)\?<>]+\.[a-z]{2,6})/i", $email)) {
          $error[] = "Please enter a valid email address in the format: [email protected].";
      }
      
      if (is_array($error)) {
          echo "<div id='errorWrapper'><h2>There are errors in your input. Please correct the following fields:</h2>";
          foreach ($error as $err_message) {
              echo "<span class='errorText'> >> $err_message" . "</span><br />";
          }
          echo "</div>";
          
          include('../includes/attendee_registration_form.php'); // this is the form
          exit();
      }
      
      else {
          include('../includes/attendee_registration_mailer.php'); // this send the email and populates the table
      }
  } else {
      include('../includes/attendee_registration_form.php'); // this is the form
      exit();
  }
?>

[Zurev]

Hello,

My script below IS finally working, but I was hoping for some aggressive, anal comments for critique.

 

Keep in mind, I am developing for a php4 platform otherwise I would have used a newer php5 validation function.

 

<?php
  if (isset($_POST['btnSubmit'])) {
      $first_name = mysql_real_escape_string($_POST['fname']);
      $last_name = mysql_real_escape_string($_POST['lname']);
      $title = mysql_real_escape_string($_POST['title']);
      $company = mysql_real_escape_string($_POST['company']);
      $address1 = mysql_real_escape_string($_POST['address1']);
      $address2 = mysql_real_escape_string($_POST['address2']);
      $city = mysql_real_escape_string($_POST['city']);
      $zip = mysql_real_escape_string($_POST['zip']);
      $phone = mysql_real_escape_string($_POST['phone']);
      $fax = mysql_real_escape_string($_POST['fax']);
      $email = mysql_real_escape_string($_POST['email']);
      
      if (!preg_match("/^[A-Za-z' -]{1,75}$/", $first_name)) {
          $error[] = "Please enter a valid first name.";
      }
      
      if (!preg_match("/^[A-Za-z' -]{1,75}$/", $last_name)) {
          $error[] = "Please enter a valid last name.";
      }
  
  if ($first_name === $last_name && $first_name != "") {
          $error[] = "First Name and Last Name cannot be the same.";
      }
      
      if (!preg_match("/^[A-Za-z' -]{1,150}$/", $company)) {
          $error[] = "Please enter a valid company name.";
      }
      
      if (!preg_match("/^[A-Za-z' -.]{1,150}$/", $title)) {
          $error[] = "Please enter a valid Title.";
      }
      
      if (!preg_match("/^[A-Za-z0-9' - . ]{1,150}$/", $address1)) {
          $error[] = "Please enter a valid mailing address.";
      }
  
  if (!preg_match("/^[A-Za-z0-9' - . ]{1,150}$/", $city)) {
          $error[] = "Please enter a valid city.";
      }
  
  if (!preg_match("/^[0-9' - . ( ) ]{1,150}$/", $phone)) {
          $error[] = "Please enter a valid phone number.";
      }
  
  if (!preg_match("/^[0-9' - . ( ) ]{1,150}$/", $fax)) {
          $error[] = "Please enter a valid fax number.";
      }
      
      if (!preg_match("/([a-z][a-z0-9_.-\/]*@[^\s\"\)\?<>]+\.[a-z]{2,6})/i", $email)) {
          $error[] = "Please enter a valid email address in the format: [email protected].";
      }
      
      if (is_array($error)) {
          echo "<div id='errorWrapper'><h2>There are errors in your input. Please correct the following fields:</h2>";
          foreach ($error as $err_message) {
              echo "<span class='errorText'> >> $err_message" . "</span><br />";
          }
          echo "</div>";
          
          include('../includes/attendee_registration_form.php'); // this is the form
          exit();
      }
      
      else {
          include('../includes/attendee_registration_mailer.php'); // this send the email and populates the table
      }
  } else {
      include('../includes/attendee_registration_form.php'); // this is the form
      exit();
  }
?>

 

I'll throw in a quick note, instead of calling mysql_real_escape_string to every single post variable manually, something like this is easier on the fingers and eyes!

foreach ($_POST as $key => $val)
{
$$key = mysql_real_escape_string($val);
}

 

That should make each post name be a variable equivalent to it's escaped version, i.e. the $_POST['fname'] will then create a variable named $fname with the contents of

mysql_real_escape_string($_POST['fname']);

[OldWest]

I'll throw in a quick note, instead of calling mysql_real_escape_string to every single post variable manually, something like this is easier on the fingers and eyes!

foreach ($_POST as $key => $val)
{
$$key = mysql_real_escape_string($val);
}

 

Zurev,

 

Thanks for the idea.

 

I like that a lot. The variable variable put to use to tidy up code. I'll see if I can implement it.

 

That should make each post name be a variable equivalent to it's escaped version, i.e. the $_POST['fname'] will then create a variable named $fname with the contents of

mysql_real_escape_string($_POST['fname']);

[Maknib]

I like this. i was just looking at his code and wondering, surely there's a way to not have to be so redundant.

 

the question i have is.. am i correct in thinking that you physically don't have a $fname, $lname $email in your code anywhere and have to rely on your memory? if so i'm outta luck hehe

[OldWest]

I like this. i was just looking at his code and wondering, surely there's a way to not have to be so redundant.

 

the question i have is.. am i correct in thinking that you physically don't have a $fname, $lname $email in your code anywhere and have to rely on your memory? if so i'm outta luck hehe

 

Yes. You are thinking correct. Read up on: variable variables

 

Basically the variable variable works like this:

 

$variable = "ABC";
$$variable = "CDE";
echo $ABC; // CDE

The value assigned to $variable becomes the variable itself. So ABC becomes _$ABC becomes CDE in that case when the variable variable $$variable is used.

 

There are  LOT of uses for the variable variable I am finding lately that really strip out code redundancy! I still have a lot to learn with it, but it's worth the time to read up on.

[dragon_sa]

Just a note on your error checking, using mysql_real_escape_string would return a value of FALSE if some one was to try and insert escaped values, meaning that on your error check the word FALSE for $first_name for example would validate and be accepted as the first name. So perhaps adding a check for FALSE to output the error in each value that accepts A-Z would be beneficial

[the182guy]

Hello,

My script below IS finally working, but I was hoping for some aggressive, anal comments for critique.

 

Keep in mind, I am developing for a php4 platform otherwise I would have used a newer php5 validation function.

 

<?php
  if (isset($_POST['btnSubmit'])) {
      $first_name = mysql_real_escape_string($_POST['fname']);
      $last_name = mysql_real_escape_string($_POST['lname']);
      $title = mysql_real_escape_string($_POST['title']);
      $company = mysql_real_escape_string($_POST['company']);
      $address1 = mysql_real_escape_string($_POST['address1']);
      $address2 = mysql_real_escape_string($_POST['address2']);
      $city = mysql_real_escape_string($_POST['city']);
      $zip = mysql_real_escape_string($_POST['zip']);
      $phone = mysql_real_escape_string($_POST['phone']);
      $fax = mysql_real_escape_string($_POST['fax']);
      $email = mysql_real_escape_string($_POST['email']);
      
      if (!preg_match("/^[A-Za-z' -]{1,75}$/", $first_name)) {
          $error[] = "Please enter a valid first name.";
      }
      
      if (!preg_match("/^[A-Za-z' -]{1,75}$/", $last_name)) {
          $error[] = "Please enter a valid last name.";
      }
  
  if ($first_name === $last_name && $first_name != "") {
          $error[] = "First Name and Last Name cannot be the same.";
      }
      
      if (!preg_match("/^[A-Za-z' -]{1,150}$/", $company)) {
          $error[] = "Please enter a valid company name.";
      }
      
      if (!preg_match("/^[A-Za-z' -.]{1,150}$/", $title)) {
          $error[] = "Please enter a valid Title.";
      }
      
      if (!preg_match("/^[A-Za-z0-9' - . ]{1,150}$/", $address1)) {
          $error[] = "Please enter a valid mailing address.";
      }
  
  if (!preg_match("/^[A-Za-z0-9' - . ]{1,150}$/", $city)) {
          $error[] = "Please enter a valid city.";
      }
  
  if (!preg_match("/^[0-9' - . ( ) ]{1,150}$/", $phone)) {
          $error[] = "Please enter a valid phone number.";
      }
  
  if (!preg_match("/^[0-9' - . ( ) ]{1,150}$/", $fax)) {
          $error[] = "Please enter a valid fax number.";
      }
      
      if (!preg_match("/([a-z][a-z0-9_.-\/]*@[^\s\"\)\?<>]+\.[a-z]{2,6})/i", $email)) {
          $error[] = "Please enter a valid email address in the format: [email protected].";
      }
      
      if (is_array($error)) {
          echo "<div id='errorWrapper'><h2>There are errors in your input. Please correct the following fields:</h2>";
          foreach ($error as $err_message) {
              echo "<span class='errorText'> >> $err_message" . "</span><br />";
          }
          echo "</div>";
          
          include('../includes/attendee_registration_form.php'); // this is the form
          exit();
      }
      
      else {
          include('../includes/attendee_registration_mailer.php'); // this send the email and populates the table
      }
  } else {
      include('../includes/attendee_registration_form.php'); // this is the form
      exit();
  }
?>

 

I'll throw in a quick note, instead of calling mysql_real_escape_string to every single post variable manually, something like this is easier on the fingers and eyes!

foreach ($_POST as $key => $val)
{
$$key = mysql_real_escape_string($val);
}

 

That should make each post name be a variable equivalent to it's escaped version, i.e. the $_POST['fname'] will then create a variable named $fname with the contents of

mysql_real_escape_string($_POST['fname']);

 

There are problems with that, you're not validating the key so if it someone posts a key with bad chars, it will cause a fatal error.

 

You should create an array of allowed form fields that are posted, check each key against the list. This will also prevent anybody setting whatever variable they want in your script, e.g. post db=false and you could kill the db connection for example.

 

My second pointer is don't use if(isset($_POST['submit'])) because it's not compatible with Internet Explorer. Use if($_SERVER['REQUEST_METHOD'] == 'POST')

[dragon_sa]

My second pointer is don't use if(isset($_POST['submit'])) because it's not compatible with Internet Explorer. Use if($_SERVER['REQUEST_METHOD'] == 'POST')

 

are you sure about that?, or do mean only with particular versions of IE. I just ran some tests of if(isset($_POST['submit'])) in IE8 and it functioned the same as FF

[the182guy]

My second pointer is don't use if(isset($_POST['submit'])) because it's not compatible with Internet Explorer. Use if($_SERVER['REQUEST_METHOD'] == 'POST')

 

are you sure about that?, or do mean only with particular versions of IE. I just ran some tests of if(isset($_POST['submit'])) in IE8 and it functioned the same as FF

 

Yes I am sure, press the enter key to submit the form and the submit button will not be sent as a post variable and thus anycode that uses isset($_POST['submit']) will not detect a posted form.

[Anti-Moronic]

And you've already marked this as 'solved'? But there are so many other pointers.

 

First, yeh, don't use variable variables, that's sloppy. Use an array:

 

foreach($_POST as $key => $val){
  
  $requestPost[$key] = mysql_real_escape_string($val);

}

 

..and yes you will want to validate the $key name. You can even use another array to exclude certain keys from being included. Now with your request variables in an array you can far better manage them. Imagine you had to send them over to a function? As an array you can send them anywhere you like as a single entity and also manage the individual elements of the array.

 

You might also find it useful to move your validation controls over to a class of some sort. That way you can run

 

$myValidationClass::isValidUsername($requestPost['username']);

 

You can now very easily include this class any where in any other project to call upon these validation functions and all you have to do is change the preg_match once. Currently if you had to change the preg_match you will have to hunt down *every* place you have validated the input and change that. Include in another project you now have to go copy and paste code from within files instead of simply including your validation class.

 

Your error handling is good enough but again can definitely benefit from being converted into a class. Using br to produce a list is sloppy html. Use a ul for that, that's what it's for.

 

Including all these files as a way of controlling the flow of the application can get very messy. You should rethink the structure. Use redirects. Currently you are allowing yourself to depend too much on other files for this script to function at all.

 

use a different email validation match.

 

..and instead of having longwided if statements you can instead do this:

 

if(!isset($_POST['btnSubmit'])){
  // now redirect or include your files
}

 

If you do things this way your code will be more readable and maintainable (albeit a very small amount more).

 

I don't think I could pick anything else apart from the script

 

Oh..don't use php4! Hope that helps.

[dragon_sa]

My second pointer is don't use if(isset($_POST['submit'])) because it's not compatible with Internet Explorer. Use if($_SERVER['REQUEST_METHOD'] == 'POST')

 

are you sure about that?, or do mean only with particular versions of IE. I just ran some tests of if(isset($_POST['submit'])) in IE8 and it functioned the same as FF

 

Yes I am sure, press the enter key to submit the form and the submit button will not be sent as a post variable and thus anycode that uses isset($_POST['submit']) will not detect a posted form.

 

I have a page with 3 submit buttons on it each with different values, how would you go about detecting which 1 was selected?

In my particular example after the first form is submitted a second 1 displays from the same page then a third after the second is submitted, but the code checks which one was submitted to determine which functions to run at the top of the page.

 

Playing with it i found that for the first form would not submit pressing enter in IE, but once I clicked the first submit, and the second form was displayed, it functions with the enter button being press. This is strange.

Both forms use the if(isset($_POST['variable_submit'])) to process.

 

Usually I like to disable the enter key on forms, but that relies on javascript and depends on weather I have a textarea or not.

[the182guy]

 

I have a page with 3 submit buttons on it each with different values, how would you go about detecting which 1 was selected?

In my particular example after the first form is submitted a second 1 displays from the same page then a third after the second is submitted, but the code checks which one was submitted to determine which functions to run at the top of the page.

 

Playing with it i found that for the first form would not submit pressing enter in IE, but once I clicked the first submit, and the second form was displayed, it functions with the enter button being press. This is strange.

Both forms use the if(isset($_POST['variable_submit'])) to process.

 

Usually I like to disable the enter key on forms, but that relies on javascript and depends on weather I have a textarea or not.

 

It's not strange, this issue has been known for quite some time but due to poor advice and/or bad PHP tutorials most people use the isset() method.

 

Having 3 submit buttons for a single form is generally not logical and there will be better alternatives in most cases. Again, if the enter key is used then none of the buttons were pressed. If it was absolutely necessary then I would do something like

 

if($_SERVER['REQUEST_METHOD'] == 'POST')
{
    // form was submitted

   $btnValue = isset($_POST['submit']) ? $_POST['submit'] : '';

   switch($btnValue)
   {
      case 'first button':
         // first button was clicked
         break;
      case 'second button':
         // second button was clicked
         break;
      case 'third button':
         // third button was clicked
         break;
      default:
         // none were clicked, so enter key used
   }
}

 

You shouldn't disable the enter key for forms, it is an accessibility failure.

[dragon_sa]

Having 3 submit buttons for a single form is generally not logical and there will be better alternatives in most cases. Again, if the enter key is used then none of the buttons were pressed. If it was absolutely necessary then I would do something like

It is 3 separate forms on the same page.

When the page is first loaded it displays form 1

-When form 1 is submitted if the parameters are met in processing it goes to form 2 else goes back to form 1

-When form 2 is submitted, if the parameters are met in processing it displays form 3 else goes back to form 2

-When form 3 is submitted, if the parameters are met in processing it notifies the user everything is accepted

 

each submit button on each different form has a different name to identify it eg

form 1 <input name="emailSUBMIT" type="submit" value="Continue Booking Request" />

form 2 <input name="submitREGISTER" type="submit" value="Register & Continue" />

form 3 <input name="appointmentSUBMIT" type="submit" value="Submit Appointment" />

 

would you still use the same switch in this case?

 

Not trying to hijack the post but more a follow on from the critique previously posted

[the182guy]

In that case with 3 different forms, I would use the same principle of checking if a form as posted using the REQUEST_METHOD, and I would have a hidden field in each form which will be used to identify the submitted form in the PHP code. e.g.

 

<input type="hidden" name="form" value="form1" />

 

Something like that in each form but change the value in each. In the PHP a switch can still be used by using $_POST['form']

 

If the 3 forms are easy to distinguish from eachother based on the submitted form fields then you could do away with the hiddens and detect the form based on the posted field names - if it's simple.

[Pikachu2000]

Why are you developing for a php4 platform?

[OldWest]

I volunteer maintain an old site that was created in early 2000 and the owners refuse to upgrade because of expenses ?!?! Basically the entire site php relies on all the security venerabilities of php4... So I guess some people learn the hard way.

 

Why are you developing for a php4 platform?