Looking for code critique

jmbrink26

Thanks for the quick reply. 🆒
Rather interesting... I just pasted the changes onto the page, and the $end parse error goes away. But what is really different?

The only major change I see is using empty($var) instead of just !($var). Did the rules of that change in PHP5?

I have used that same identical code (with different variables, and without any MySQL DB connections) with no problems before... That is strange.

As far as the date(r), php.net references:

 r  	» RFC 2822 formatted date  	Example: Thu, 21 Dec 2000 16:01:07 +0200

I know you referenced that, so I thought I'd clarify. 🙂

bradgrafelman

jmbrink26 wrote:
Did the rules of that change in PHP5?

No... as far as I know, using if(!$var) has been a bad practice since PHP4 or earlier.

And as far as date(r) goes, it's incorrect because you didn't define a constant 'r' anywhere (e.g. by using [man]define/man). Now, if you meant the string 'r', that's a different story. :p

jmbrink26

bradgrafelman;10903984 wrote:
No... as far as I know, using if(!$var) has been a bad practice since PHP4 or earlier.

And as far as date(r) goes, it's incorrect because you didn't define a constant 'r' anywhere (e.g. by using [man]define/man). Now, if you meant the string 'r', that's a different story. :p

I was not aware that was bad practice. I'll have to keep that in mind, so I use empty for now on. Thanks for the FYI 🙂

Now, what I am seeing is my generated error message comes up saying that something was not entered in the form, being triggered by one of the empty() clauses. One question I have when dealing with more than one page of a form, what is the proper way to pass variable data from page 1 to page 3 so to speak.. ?

I am fairly sure the empty() is being met because the variables passed from page 1 to page 2 aren't being passed to page 3 after the submit button is hit on page 2. I just assumed all variables would be passed through to the next sequential page (but you know what that does of course... 😉 ).

Would putting an in include('page1.php') or require('page1.php') do the trick? Or maybe I need to do the include/require for page2.php, but I'm not if by doing that, if that breaks the data flow of the entire form...

As far as the 'r', I was using it as a string, not a constant. 😃

djjjozsi

its empty, becouse i've inserted this piece of code to test if you fill the form or not:

if(empty($_POST))
die("Form is empty!!!");

Remove if it's needed.

The very simple way to give vars into next pages is the session. And if its needed use database to store user data.

You see, there are undefined indexes in your code. Just see into your mysql query.

The proper technique is to write a function to collect the required fields with syntax check, then you can use one function to build the error message.

bradgrafelman

jmbrink26 wrote:
I was not aware that was bad practice. I'll have to keep that in mind, so I use empty for now on.

Eh, no offense, but there are also several other 'bad/non-recommended practices' in the script above - feel free to post in Code Critique/General Help for others to peruse/make suggestions.

jmbrink26 wrote:
what is the proper way to pass variable data from page 1 to page 3 so to speak

Either re-post the values from page 2 to page 3 using hidden form fields (note that this is insecure, however - the data can still be changed even though they are "hidden" fields), or better yet start a [man]session[/man] and keep the data in there.

jmbrink26 wrote:
As far as the 'r', I was using it as a string, not a constant.

I was looking at this code when I made that comment:

$date = date( r );

In that code, r is used as a constant. Failing to find a defined [man]constant[/man] named 'r', PHP will throw a warning message and then trying using r as a string, "r".

jmbrink26

djjjozsi;10903986 wrote:
its empty, becouse i've inserted this piece of code to test if you fill the form or not:
if(empty($_POST))
die("Form is empty!!!"); 
Remove if it's needed.

The very simple way to give vars into next pages is the session. And if its needed use database to store user data.

You see, there are undefined indexes in your code. Just see into your mysql query.

The proper technique is to write a function to collect the required fields with syntax check, then you can use one function to build the error message.

I was able to get that figured out after nosing around Google some more. It's just a matter of typing the right thing into Google as always... 😉. Yeah I used hidden form fields for now. I will most likely create a session however to handle these. I do agree that it is insecure.

jmbrink26

bradgrafelman;10903988 wrote:
Eh, no offense, but there are also several other 'bad/non-recommended practices' in the script above - feel free to post in Code Critique/General Help for others to peruse/make suggestions.

Either re-post the values from page 2 to page 3 using hidden form fields (note that this is insecure, however - the data can still be changed even though they are "hidden" fields), or better yet start a [man]session[/man] and keep the data in there.

I was looking at this code when I made that comment:
$date = date( r );
In that code, r is used as a constant. Failing to find a defined [man]constant[/man] named 'r', PHP will throw a warning message and then trying using r as a string, "r".

I will keep that in mind. Perhaps I will post the code into the other section as well. I am always open to finding better, more efficient ways to write my code. If there is a way to make it better, I am open to it. 😉. The original script I wrote 2 years ago was just a quick and dirty script to handle a 5 field mail form for a client of mine, and I never bothered to look much into the code as far as inefficiencies, but now is as good a time as ever to do that.

I am going to create a session to store these. I just tested it just previously with hidden form fields, but being the forms contain relatively personal data once its placed on the destined page, http://www.stevenshenager.edu/request_info.html, I am definitely going to want to have it secure from the front and back end. I am probably going to want to set up a hash for some of the data, but the downfall I see with the unintelligent corporate people is they will try to pull data out of the database, and it will be hashed, and' they'll whining about it, so I probably won't go that far unfortunately 🙁.

As far as the date(r), I've never had any problem. I don't have a problem defining the constant, have just never had to. Like if you were to enter dummy data into the form, the 3rd page now, will have a timestamp generated by that variable, and it even worked just now.

In any case, I thank you for all your insight and input. I suppose if we were all perfect developers, we wouldn't need validators and such... 😉

jmbrink26

Hi All,

I was advised by another member to have some critique on my code to make it more efficient, and thought I'd get some critique on my code.

Here is the code from a page that I am using in a 2 page request form:

<?php
require( 'form.inc' );
// Declare Form Variables

$to = "example@example.com";
$subject = "Request Information from SHC Website";
$date = date( r );  //<-------- WHAT DO YOU EXPECT??? read the date function in php.net
$hostname = gethostbyaddr( $_SERVER['REMOTE_ADDR'] );
        $connect = mysql_connect( $host, $user, $pass );
        if ( !$connect )
        {
                die( "Could not connect to database: " . mysql_error() );
        }
        mysql_select_db( $db, $connect );
if(empty($_POST))
die("Form is empty!!!");
$_POST=array_map("mysql_real_escape_string",$_POST);

$email = $_POST['email'];
$cell = $_POST['cell'];
$address = $_POST['address'];
$city = $_POST['city'];
$state = $_POST['state'];
$zip = $_POST['zip'];
$message = $_POST['message'];
$headers = 'From: ' . $email . "\r\n" . 'REPLY TO: ' . $email . "\r\n" . 'Date: ' . $date . "\r\n" . 'Sender Name: ' . $first . $last . "\r\n" . 'Sender Phone: ' . $phone . "\r\n" . 'Sender Cell: ' . $cell . "\r\n" . 'Sender Address: ' . $address . "\r\n" . 'Sender City: ' . $city . "\r\n" . 'Sender Zipcode: ' . $zip . "\r\n" . 'Area of Study: ' . $aoStudy . "\r\n" . 'Program of Interest: ' . $program . "\r\n" . 'Campus of Choice: ' . $campus . "\r\n" . 'Sender Hostname: ' . $hostname . "\r\n";

if (  empty($first ) || empty( $last ) || empty( $phone ) || empty( $campus ) || empty( $aoStudy ) || empty( $program ) ||  empty($email ) || empty( $cell ) || empty( $address ) || empty( $city ) || empty( $state ) || empty( $zip ) )
{
        echo "<p class='intro'><span class='urgent'>ERROR</span> Information was not entered into required fields marked with a red asterisk (<span class='urgent'>*</span>. Please go back and resubmit the necessary information. We apologize for the inconvenience. Have a great day.</p>";
}
else
{
        $insert = "INSERT INTO requests (fname, lname, phone, campus, aostudy, program, email, cell, address, city, state, zip) VALUES ('$first', '$last', '$phone', '$campus', '$aoStudy', '$program', '$email', '$cell', '$address', '$city', '$state', '$zip');";

/* UNDEFINED INDEXES IN YOUR CODE    */

    if ( !mysql_query( $insert, $connect ) )
    {
            die( "Error: " . mysql_error() );
    }
    mysql_close( $connect );
    if ( mail( $to, $subject, $message, $headers ) )
    {
            echo "<p class='intro'>The e-mail message has been sent. We will reply to your inquiry as soon as possible. Thank you, and have a great day. <br />
           <a href='home.html'>Click here to return to homepage.</a></p>";

            echo "Dated $date";
    }
    else
    {
            echo "<p class ='intro'>The e-mail could not be delivered to $email. Please<a href='contact.html'> return to the previous page</a> to try to resend the message. If you have confirmed that you are entering in the correct information, and are seeing this message more than once, please notify us at example@example.com. Thank you, and have a great day.</p>";

            echo "Dated $date";
    }
}

?>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
<title>Untitled Document</title>
</head>

<body>
</body>
</html>

Thank you for your critique and input.

jmbrink26

One question I have about using these as session variables is, what is the proper way to convert form data into session variables?

So you of course take form output and put into a variable...

<?php $var = $_POST['var']; ?>

But how would you go about converting into a session variable?

If my presumptions are correct, doing:

<?php $var = $_SESSION['var']; ?>

Would then replace the value of $var. And I am fairly positive that:

<?php $var2 = $_SESSION['var2'] = $var; ?>

Would throw a parse error without question.

So what is the proper way to go about this? I haven't had much of an opportunity to play with session variables enough to know my way around these sort of dillemas...

NogDog

jmbrink26;10903994 wrote:
...And I am fairly positive that:
<?php $var2 = $_SESSION['var2'] = $var; ?>
Would throw a parse error without question.

So what is the proper way to go about this? I haven't had much of an opportunity to play with session variables enough to know my way around these sort of dillemas...

You would be wrong. The following is perfectly legal, syntactically, though I doubt I would ever actually use it:

$foo = $bar = $_SESSION['foobar'] = $_POST['foobar']

dalecosp

jmbrink26;10903994 wrote:
One question I have about using these as session variables is, what is the proper way to convert form data into session variables?

So you of course take form output and put into a variable...
<?php $var = $_POST['var']; ?>
But how would you go about converting into a session variable?

Well, it can be straightforward:

session_start();
$_SESSION['foo']=$_POST['foo'];

In the days of auld, 'twas:

session_start();
$foo=$_POST['foo'];
session_register($foo);

But, generally, you want to check any input coming from a form; perhaps, even, you wish to process it somehow:

session_start();
$foo=$_POST['foo'];
$foo=my_cleanup_routine($foo);
$foo=some_processing($foo);
$_SESSION['foo']==$foo;

Weedpacket

Um, the HTML page doesn't seem to do much 🙂

;
$date = date( r );  //<-------- WHAT DO YOU EXPECT??? read the date function in php.net

You mean

$date = date('r');

Moved critique from Parse Error thread into this one.

bradgrafelman

A few things...

To quote Weedpacket from a different thread regarding the same issue:

Weedpacket;10904131 wrote:
On a live site "die(mysql_error())" is (a) lame, and (b) a security risk.
So, if the above script is what appears on your live production server, then you shouldn't be outputting mysql_error() anywhere.
If you do something like:
```
$_POST=array_map("mysql_real_escape_string",$_POST); 
```
then you're destroying data integrity. If I entered my message as "It's a wonderful day", you would likely get an e-mail saying "It\'s a wonderful day." Use the DBMS' escaping method only when sending data to the DBMS - don't do it at the global level.
The "From" header should always be an e-mail address on your own domain; placing the user-supplied e-mail address in this field makes you an open relay and very likely to be blacklisted by e-mail hosts around the globe (e.g. via RBL's).
You don't sanitize (e.g. remove line breaks) any of the user-supplied data before putting it into the e-mail headers, leaving your script vulnerable to header injection attacks.
Why do you place all of that data in the mail headers? "REPLY TO:" isn't even a valid mail header to begin with. In addition, you usually shouldn't supply the "Date" header - your MTA should handle that automatically.

This is just a personal preference, but if you have a lot of variables/escape sequences, why not just stick to double quotes? E.g. this string:

$headers = 'From: ' . $email . "\r\n" . 'REPLY TO: ' . $email . "\r\n" . 'Date: ' . $date . "\r\n" . 'Sender Name: ' . $first . $last . "\r\n" . 'Sender Phone: ' . $phone . "\r\n" . 'Sender Cell: ' . $cell . "\r\n" . 'Sender Address: ' . $address . "\r\n" . 'Sender City: ' . $city . "\r\n" . 'Sender Zipcode: ' . $zip . "\r\n" . 'Area of Study: ' . $aoStudy . "\r\n" . 'Program of Interest: ' . $program . "\r\n" . 'Campus of Choice: ' . $campus . "\r\n" . 'Sender Hostname: ' . $hostname . "\r\n";

could be compressed down to:

$headers = "From: $email\r\nREPLY TO: $email\r\nDate: $date\r\nSender Name: $first $last\r\nSender Phone: $phone\r\nSender Cell: $cell\r\nSender Address: $address\r\nSender City:$city\r\nSender Zipcode:$zip\r\nArea of Study:$aoStudy\r\nProgram of Interest:$program\r\nCampus of Choice:$campus\r\nSender Hostname:$hostname\r\n";

without losing much readability, IMHO.

Your use of empty() still isn't quite right: On the lines before that if() statement, you still attempt to access indexes of the $_POST that might not exist, so the error will already be thrown by the time your if() statement runs.