Is it ok to put an insert statement inside a foreach?

[bugzy]

Just a beginner question guys..

 

I have this code

 

<?php

 

                            $query = "Insert INTO item(item_code, item_description, item_price, item_stock, item_sale, item_reg) VALUES('{$item_code}','{$item_description}','{$item_price}','{$item_stock}', '{$item_sale}', NOW())";

 

                              $last_id = mysql_insert_id();

 

foreach($_POST['item_cat'] as $cat_get_ins)

{

$query1 = "Insert into item_category (category_id, item_id) VALUES ('{$cat_get_ins}','{$last_id}')";

$result1 = mysql_query($query1,$connection);

}

?>

 

 

Just want to be sure if is it ok?

 

another question is, is  mysql_insert_id(); safe to use? as you can see on the code above I've use it.

 

[xyph]

No, it's not okay to use mysql_query in a foreach - In this case anyways. Check out the syntax to insert multiple rows in a single query.

http://www.electrictoolbox.com/mysql-insert-multiple-records/

 

Yes, that's what mysql_insert_id was meant for.

[ManiacDan]

You can form one big INSERT query with multiple values and insert it in one block.  that's more efficient:

 

 

<?php

            $query = "Insert INTO item(item_code, item_description, item_price, item_stock, item_sale, item_reg) VALUES('{$item_code}','{$item_description}','{$item_price}','{$item_stock}', '{$item_sale}', NOW())";

            $last_id = mysql_insert_id();
            
            $query1 = "Insert into item_category (category_id, item_id) VALUES ";
            foreach($_POST['item_cat'] as $cat_get_ins)
            {
               $query1 .= "('{$cat_get_ins}','{$last_id}'),";   
               
            }
            $result1 = mysql_query(trim($query1, ','),$connection);
?>

 

mysql_insert_id is the perfect function for this situation, yes.

 

[requinix]

Besides the INSERT and mysql_insert_id() you've got a larger problem: what you're doing with $cat_get_ins is not safe. Ironically it's the one thing you didn't ask about.

 

Stuff in $_POST, $_GET, ($_REQUEST,) and $_COOKIE come from the user and should never be used directly in anything without validating and/or sanitizing it first. For example, you may think $_POST['item_cat'] is an array of integers but that's not necessarily true. It may be a single text field. Yeah sure, your form says one thing, but there's no way to make sure that the form wasn't tampered with before it was submitted to you.

 

Blah blah blah SQL injection and XSS.

 

$_POST['item_cat'] = (array)$_POST['item_cat'];
foreach($_POST['item_cat'] as $cat_get_ins) {
    $cat_get_ins = (int)$cat_get_ins;
    // ...
}

[scootstah]

No, that really isn't okay. What also is not okay is that you are using untrusted user input directly in the query. I don't believe there are any security problems with mysql_insert_id().

 

First and foremost you need to be escaping user input with mysql_real_escape_string to prevent SQL injection. If the column type is an integer, you can safely typecast the data to integer - removing any non-int characters.

 

Where do these variables come from: '{$item_code}','{$item_description}','{$item_price}','{$item_stock}', '{$item_sale}'? Are they escaped?

 

Anyways, MySQL supports multiple INSERT values in the query, so that is what you should be using. This way you have one query, but many inserted rows. Something like this ought to work:

$query = "Insert INTO item(item_code, item_description, item_price, item_stock, item_sale, item_reg) VALUES('{$item_code}','{$item_description}','{$item_price}','{$item_stock}', '{$item_sale}', NOW())";

mysql_query($query);

$last_id = mysql_insert_id();

$insert_data = array();

foreach($_POST['item_cat'] as $item_cat)
{
$item_cat = (int) $item_cat;

$insert_data[] = "($item_cat, $last_id)";
}

$query = "INSERT INTO item_category (category_id, item_id) VALUES " . implode(',', $insert_data);

mysql_query($query);

 

Basically, this just loops through the $_POST array and adds the set of values to an array. Later, the array is imploded to create the SQL syntax. Which, in case you don't know, the multiple insert value syntax looks like this:

INSERT INTO item_category (category_id, item_id) VALUES (1, 1), (2, 1), (3, 1) ... 

[bugzy]

Thank you very much guys for your advises.

 

I just put the "values" inside the foreach just what you guys said.

 

as for using mysql_real_escape_string(), I have a function for that, I just didn't put it on my post above so you can guys see it more clearly.

 

 

 

Juts last question..

 

Is this the same thing?

 

(int)$cat_ins

(is_numeric)$cat_ins

 

?

 

Because I'm filtering it using is_numeric

 

Anyone?

[scootstah]

(int)$cat_ins

(is_numeric)$cat_ins

 

No, and that's not how you use is_numeric().

 

is_numeric() returns true if the input is a number, or false if it is not. Note that any number will return true, even if it is a decimal. If you only want integers, you should be using is_int().

 

On the other hand, (int) is for type casting. It is used when you want to explicitly convert one data type to another. In this case, you would end up with an integer no matter what the data type started out as.

 

as for using mysql_real_escape_string(), I have a function for that

 

Does your function actually use mysql_real_escape_string()? If not, it probably isn't secure.

[bugzy]

(int)$cat_ins

(is_numeric)$cat_ins

 

No, and that's not how you use is_numeric().

 

is_numeric() returns true if the input is a number, or false if it is not. Note that any number will return true, even if it is a decimal. If you only want integers, you should be using is_int().

 

On the other hand, (int) is for type casting. It is used when you want to explicitly convert one data type to another. In this case, you would end up with an integer no matter what the data type started out as.

 

as for using mysql_real_escape_string(), I have a function for that

 

Does your function actually use mysql_real_escape_string()? If not, it probably isn't secure.

 

Thanks for that scootash

 

btw here's my function

 

<?php

function me_mysql_prep( $value ) 
{
	$magic_quotes_active = get_magic_quotes_gpc();
	$new_enough_php = function_exists( "mysql_real_escape_string" ); 
		if( $new_enough_php ) 
			{
				if( $magic_quotes_active ) 
					{ 
						$value = stripslashes( $value ); 
					}
				$value = mysql_real_escape_string( $value );
			} 
			else 
			{
				if( !$magic_quotes_active ) 
				{ 
					$value = addslashes( $value ); 
				}

			}
			return $value;
}

?>

 

 

Is that safe already?

[scootstah]

Yeah, that should work.

 

Although if you find a webserver running a version of PHP that doesn't have mysql_real_escape_string(), something is very, very wrong.

[bugzy]

Yeah, that should work.

 

Although if you find a webserver running a version of PHP that doesn't have mysql_real_escape_string(), something is very, very wrong.

 

 

Thanks guys!

 

I think that's all I need to know here.

 

Solved!