HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpModerate

PHP dynamic insert - mysql. Is there a better way than this?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisinsertphpthanwaybettermysqldynamicthere

Problem

I have a dynamic form that users can add extra fields too. The group of fields are added and the input name stays the same but gets a incremental number attached to it. So for my insert php file I just check to see if the next value is set and if so it inserts all the required info. But this code doesn't look very 'streamlined' to me. Is there a better way?

mysql_select_db("inventory", $con);

if( !isset($_POST['productid2']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]')";
}

if( isset($_POST['productid2']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]'),
('$_POST[productid2]','$_POST[type2]','$_POST[client2]','$_POST[product2]','$_POST[color2]','$_POST[quantity2]')";
}

if( isset($_POST['productid3']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]'),
('$_POST[productid2]','$_POST[type2]','$_POST[client2]','$_POST[product2]','$_POST[color2]','$_POST[quantity2]'),
('$_POST[productid3]','$_POST[type3]','$_POST[client3]','$_POST[product3]','$_POST[color3]','$_POST[quantity3]')";
}

    // And I have like this 5 more times, the code gets longer each time!

Solution

Array based approach

Instead of appending a number to the field, use an array based approach:


    Product 1Product Id 
    
    Product Type 
                
    Product 2
    Product Id 
    
    Product Type 


Now if you were to submit this to PHP and do print_r($_POST['products']);, you would get something like the following:

array(
    0 => array(
        'id' => '...',
        'type' => '...',
    ),
    1 => array(
        'id' => '...',
        'type' => '...',
    )
)


This allows you to do the logic for the insertion once, and then use a loop to do all of the fields.

The (pseudo) code would be like:

$products = (isset($_POST['products']) && is_array($_POST['products'])) ? $_POST['products'] : array();

//Note that I've assumed the id column is an int, and the type column is a string.
//In real schema, I would expect type to also be an int, but I want to illustrate proper escaping.
//(There is more on this in a later section)
$query = "INSERT INTO products (id, type) VALUES (%d, '%s')";

foreach ($products as $product) {
    //Ensure that $product contains a valid entry, and if it does not, store errors somewhere
    if (valid entry) {
        $res = mysql_query(sprintf($query, (int) $product['id'], mysql_real_escape_string($product['type'])));
        if (!$res) {
            //Log an error about the query failing
        }
    }
}


SQL Injection

Put all the validation on the client side you want, but at the end of the day, it's bypassable. In fact, a user doesn't even have to use a browser, much less use JavaScript. A user can send your server anything he wants. After all, at the end of the day, HTTP is a text based protocol.

Also, properly escaping isn't just about security. It's also about correctness.

$string = "The other day, someone said, "Hello my name is Fred!""; //Syntax error!


Not escaping SQL is the same concept as this. This isn't a security issue; it's just wrong. Escaping SQL is the same concept as doing:

$string = "The other day, someone said, \"Hello my name is Fred!\"";


The escaping is necessary so that the SQL server can parse the query correctly in the same way that escaping in PHP is necessary so that the PHP engine can parse the script correctly.

The reason that SQL injection is a security concern is user input. If you don't properly escape, users can change your code.

It's hard to make a brief PHP-injection example, but hopefully the concept is there. An attempt at an example:

eval("\$x = 5 + {$_POST['input']};");


That's all good and fine as long as $_POST['input'] === "5"; or some other kind of innocent value. However, consider if $_POST['input'] === "5; evilFunction()". Suddenly the eval is:

eval("\$x = 5 + 5; evilFunction();");


This is the broad concept of SQL injection. (Various articles on the internet will explain it much better than me though :p.)

(eval should very rarely [arguably, never] actually be used -- this is just to illustrate a point)

Anyway, the solution is very simple. Anything that is going to be a string needs to be run through mysql_real_escape_string before being interpolated into a query. Likewise, anything that is not a string should be handled as the proper type:

$query = "INSERT INTO tbl (stringField, intField) VALUES ('%s', '%d')";
$qry = sprintf($query, 
        mysql_real_escape_string($someString), //make sure the string field is properly escaped
        (int) $someInt //make sure the int field is an int
       );


PDO

I would advise everyone to move away from the old mysql extension. The mysql_* functions are a very thin wrapper around the C API, and though there is not anything particularly wrong with them, PDO is just better.

In particular, PDO:

  • Offers a cleaner API



  • Offers better error handling (the ability to have failed queries throw exceptions is wonderful)



  • Handles transactions SQL-dialect agnostically (for the most part)



  • Different RDBMS can be changed out relatively easily (switching from mysql_* to Postresql is extremely painful. Switching from PDO with MySQL to PDO with Postresql isn't pleasant, but is care is taken to stay as dialect-agnostic as possible, it's reasonably easy)



  • Prepared statements



Prepared statements deserve their own sublist:

  • 100% safe from SQL injection (if used correctly, of course)



  • Better performance for batched queries



  • Various other minor things



Prepared statements work vaguely like:
* Create a parameterized statement
* Let the SQL server handle putting the variables into place

The second point is the important one. Since the server is putting the paramters into place, it's impossbile for something to be misinterpretted.

An example will be a lot more useful:

```
//create the connection
$db = new PDO("mysql:host=localhost;dbname=database_name", "username", "password");
//I like for failed queries to throw exceptions.
//If you're not comfortable with exception handling, you woul

Code Snippets

<form action="" method="POST">
    <b>Product 1</b><br
    <label>Product Id <input type="text" name="products[0][id]"></label>
    <br>
    <label>Product Type <input type="text" name="products[0][type]"></label>
    <br>            
    <b>Product 2</b><br>
    <label>Product Id <input type="text" name="products[1][id]"></label>
    <br>
    <label>Product Type <input type="text" name="products[1][type]"></label>
</form>
array(
    0 => array(
        'id' => '...',
        'type' => '...',
    ),
    1 => array(
        'id' => '...',
        'type' => '...',
    )
)
$products = (isset($_POST['products']) && is_array($_POST['products'])) ? $_POST['products'] : array();

//Note that I've assumed the id column is an int, and the type column is a string.
//In real schema, I would expect type to also be an int, but I want to illustrate proper escaping.
//(There is more on this in a later section)
$query = "INSERT INTO products (id, type) VALUES (%d, '%s')";

foreach ($products as $product) {
    //Ensure that $product contains a valid entry, and if it does not, store errors somewhere
    if (valid entry) {
        $res = mysql_query(sprintf($query, (int) $product['id'], mysql_real_escape_string($product['type'])));
        if (!$res) {
            //Log an error about the query failing
        }
    }
}
$string = "The other day, someone said, "Hello my name is Fred!""; //Syntax error!
$string = "The other day, someone said, \"Hello my name is Fred!\"";

Context

StackExchange Code Review Q#12647, answer score: 12

Revisions (0)

No revisions yet.