Insert form data into database. (First post, be gentle)

[Scriptmaker]

<?php
if(isset($_POST['submit']))
{

//Define variables
$keys = "";
$values = "";
$error = "";
// Copy post data into another array
$arr_post_data_copy = $_POST;

//designate which form data need to be handled
// differently, if applicable
$badkeys =
array(
"submit", //neccesary, otherwise will try to input data into
//nonexistant 'submit' column.
"data1",
"etc"
);

//remove these keys from arr_post_data_copy

foreach ($badkeys as $b)
{
unset($arr_post_data_copy[$b]);
}


//build an array with all of the keynames
$count = "0";
while ($count< count($arr_post_data_copy))
{

$goodkeys[] = key($arr_post_data_copy);
next($arr_post_data_copy);
$count++;
}

//build key and value strings with post data
$count = "0";
foreach ($arr_post_data_copy as $data)
{
//display values while building the string
echo $goodkeys[$count]." : ".$data."<br>";
//unnecessary checks to prevent comma at beginning
if ($keys != "")
{
$keys .= ",".$goodkeys[$count];
}
else
{
$keys .= $goodkeys[$count];
}

if ($values != "")
{
$values.= ",'".addslashes($data)."'";
}
else
{
$values.= "'".addslashes($data)."'";
}
$count++;
}

//check if data has made it this far
if ($keys == "" OR $values == "")
{
$error .= "<li>No values were passed to the script.";
}


if ($error == "")
{
// Connect to Database
include("connect.php");
// Execute the query
mysql_query("INSERT INTO tablename ($keys) VALUES ($values)")or die(mysql_error());
echo "Entry # ".mysql_insert_id().
" was successfully added to the database";
}

else
{
echo $error;
}

}

else
{

?>
Test Form:<br />
<form method="POST" name="form">
<input type="text" size="25" name="textbox1" />
<input type="text" size="25" name="textbox2" />
<input type="submit" name="submit" value="Submit" />
</form>

<?php
}
?>




I made this script because I've been working on a project that required three separate forms to enter data into three different database tables. I didn't want to have to change the field names and values for each form handler, so I decided make a loop do it for me.

I'm positive it's been done before, but I wanted to see if I could do it on my own.

My own critique:

The script has some limitations, such as:

form field names have to match database column names
you can't have multiple inputs insert only one value into one column (such as multiple checkboxes)


I added a lot of unnecessary bulk by adding all of the checks for empty strings just to make sure a comma wasn't at the beginning. I could have easily just checked for a comma right before querying the db and then removed it.

====

So what do you think? Thanks for your opinions!

p.s. - sorry about the wide code window. I can't figure out which line is making it stretch.

[etully]

In my opinion, the code is decent from a functional perspective. That is, it looks like it does what it's supposed to do. For PHP syntax, I think you have a good understanding of passing variables from your form to your script, you have a good understanding of arrays (though I might do it differently in fewer steps - but you'll get that with experience). You have a good understanding of making it work with a database. These are far more skills than lots of beginners who come to this site looking for help.

Here's my criticism: The code is vulnerable to a security flaw called SQL injection. I'll describe it briefly since it's been discussed a thousand times before in this forum and a million times before all over the Internet.

Basically, you are too trusting of the data that is passed in from the user. In theory, people will only use the form that you supply them, they will fill in the fields with simple data, and your script will work fine. In practice, hackers will build their own forms and post to your script and they will pass dangerous and destructive data to your script. Consider the following example:

$name = $_POST['name']; // this receives the data from the name field in the form
$query = "insert into users (name) values ('$name');

Theoretically, this should just add another name to the database. But what if someone enters the following as their name:

Bob'); delete from users where name='Sue';

Now $query will have the following value:

insert into users (name) values ('Bob'); delete from users where name='Sue';')

And when you perform that command on your database, it will add the name Bob but delete everyone named Sue. With a little creativity, you could change everyone's password, delete everyone's account, etc.

I guess this is what people mean when they say, "I know enough PHP to be dangerous". Your skills are good enough that you can create a site that could backfire horribly! :) Keep practicing, though, you're on the right track.

[Scriptmaker]

Thank you very much for the critique and compliments. I am aware of mysql injection, and have been reading up on it. My "addslashes" on the $data variable was my attempt to offer some protection against this. I think I can see how the $keys would need the same protection too, since someone could make their own form with a field name like the one you mentioned.

I'll keep working on it, since I completely understand that even the cleanest, most efficient code is worthless if it isn't secure.

[etully]

Sorry if I over-explained something you already understood. I'll just mention one other example (then I'm done, I promise). As you said, ID's are a critical place for SQL injection. Since you mentioned "addslashes", I'll point out that if you cookie someone with their userid and then you trust that cookie (which you shouldn't), they could replace their userid (12345) with something like: 12345 or id>0 which is immune to the addslashes command and would have serious consequences.

Otherwise, though, your code is good and you're on the right track.

[Scriptmaker]

There's no such thing as over explaining in my book. I'm always willing to learn from other people's experience. Besides, while I understand what sql injection is, I hardly have a strong grasp of what I need to do to stop it.

Thanks again!

[Weedpacket]

You mention the unnecessary bulk of checking for commas or not; there are a couple of alternatives.
First is to have a variable that has a specific value during the first iteration of the loop, which it won't have on any other iteration. Include the comma if that variable does not have that specific value.

$string = '';
$first = true;
foreach($array as $element)
{
if($first)
{
$first = false;
}
else
{
$string .= ",";
}
$string .= $element;
}

A variation is to use the comma itself as the special variable:

$string = '';
$separator = '';
foreach($array as $element)
{
$string .= $separator.$element;
$separator = ',';
}


The other option is to build an array of all the strings you're going to concatenate together, and then build the whole string in one hit with join().

The array functions may help you streamline some of your code (mainly for cleanness; efficiency is not quite as critical, but my position is that reducing the number of moving parts means that there are fewer places for bugs to hide). For example, there is a built-in function, array_keys that will give you an array containing the keys of another array; array_diff can then be used to strip out the bad keys.

[Scriptmaker]

The second version with the blank separator that gets changed to a comma after one pass looks great! I'll update the script the next chance I get, thanks!

[Scriptmaker]

substr looks like it will work too. Just assume there will always be a comma at the beginning and remove it.

$string = substr($string, 1, strlen($string));

side question: is it wrong, or just bad practice to set a variable equal to itself after a function is performed on it (like above)? Thanks!

[halojoy]

substr looks like it will work too. Just assume there will always be a comma at the beginning and remove it.

$string = substr($string, 1, strlen($string));

side question: is it wrong, or just bad practice to set a variable equal to itself after a function is performed on it (like above)? Thanks!


you also have the functions, for trimming:
trim
ltrim
rtrim

trim ( $str [, $charlist] )
... will trim the specified characters in $charlist,
from BOTH beginning & end of $str

by default, without $charlist, alll these will be removed:
* " " (ASCII 32 (0x20)), an ordinary space.
* "\t" (ASCII 9 (0x09)), a tab.
* "\n" (ASCII 10 (0x0A)), a new line (line feed).
* "\r" (ASCII 13 (0x0D)), a carriage return.
* "\0" (ASCII 0 (0x00)), the NUL-byte.
* "\x0B" (ASCII 11 (0x0B)), a vertical tab.

I suppose, when $charlist is specified, ONLY those will be removed
but not 'spaces' ect., unless $charlist contains the 'space character'
---------------------------

ltrim()
left trim only - only leading spaces and chars will be removed
and of course:
rtrim() ... takes away only from end of $str

<?php

// will remove all eventual leading commas= "," from $string
$string = ltrim( $string, "," );

?>


Regards
halojoy
:)

[Scriptmaker]

Thanks Halo.

I like the left trim function. That works better than the one I posted, since if for some odd reason the string doesn't have a string at the beginning, it won't delete the first letter.

[Weedpacket]

static $quote_and_escape = null;
if(is_null($quote_and_escape))
{
$quote_and_escape = create_function('$a', 'return "\'".mysql_real_escape_string($a)."\'";');
}

$real_data = array();
$badkeys = array('submit', 'data1', 'etc');
$keys = array_diff(array_keys($_POST), $badkeys);
foreach($keys as $key)
{
$real_data[$key] = $_POST[$key];
}
if(!count($real_data))
{
echo 'No values passed';
}
else
{
$keys = join(", ", array_keys($real_data));
$values = join(", ", array_map($quote_and_escape, array_values($real_data)));
include_once('connect.php');
mysql_query("INSERT INTO tablename ($keys) VALUES ($values)")or die(mysql_error());
echo "Entry # ".mysql_insert_id()." was successfully added to the database";
}