//array keys and array vaules
//connection
// checks results
}
?>
But I am struggling as i need the PHP to use the command array_keys(vaules) and array_vaules($vaules) to take input of the column and then insert the data. I have an index apge which works fine but needs to call this function
function
[PHP]
function insert($table, array $data)
{
/*
* Check for input errors.
*/
if(empty($data)) {
throw new InvalidArgumentException('Cannot insert an empty array.');
}
if(!is_string($table)) {
throw new InvalidArgumentException('Table name must be a string.');
}
$fields = '`' . implode('`, `', array_keys($data)) . '`';
$placeholders = ':' . implode(', :', array_keys($data));
$sql = "INSERT INTO {$table} ($fields) VALUES ({$placeholders})";
// Prepare new statement
$stmt = $dbconn->prepare($sql);
/*
* Bind parameters into the query.
*
* We need to pass the value by reference as the PDO::bindParam method uses
* that same reference.
*/
foreach($data as $placeholder => &$value) {
// Prefix the placeholder with the identifier
$placeholder = ':' . $placeholder;
// Bind the parameter.
$stmt->bindParam($placeholder, $value);
}
/*
* Check if the query was executed. This does not check if any data was actually
* inserted as MySQL can be set to discard errors silently.
*/
if(!$stmt->execute()) {
throw new ErrorException('Could not execute query');
}
/*
* Check if any rows was actually inserted.
*/
if($stmt->rowCount() == 0) {
var_dump($dbconn->errorCode());
throw new ErrorException('Could not insert data into users table.');
}
return true;
I have not tested what you posted but at first glance it appears there would be an issue not handled.
Assuming your $fields array is actually coming from a form post, you will also have the submit name and value in the array which would be attempted to be inserted causing the query to fail would it not?
Good point, picky point though. My code assumes that the form has already been handled. Probably should have stated that, just gathered it was obvious. It doesn’t have PHP tags either, I assumed those too ;D
Good point, picky point though. My code assumes that the form has already been handled. Probably should have stated that, just gathered it was obvious. It doesn't have PHP tags either, I assumed those too
A failed query is not picky. What may be obvious to you and I wont be to many others. I cant believe I missed the missing Php tags. It was right there in my face and I didn’t even notice. No wonder it didn’t work. Thanks for pointing that out. :o
I forgot to mention you shouldn’t name your columns col_1, col_2 or the values value_1, value_2
[ul][li]Since all identifiers are inserted verbatim, the query is vulnerable to SQL injections. Even worse, it’s impossible to see that without looking at the code, because there’s no indication whatsoever telling the user to take care of the identifiers himself.[/li]
[li]The function cannot handle nonstandard identifiers. If you try to use an identifier like “user name”, the function blows up with some syntax error. It’s not even clear if you want to support nonstandard identifiers or not. On the one hand, you accept arbitrary array keys and quote the column names with backticks. On the other hand, you neither normalize the parameters nor escape backticks. And the table name isn’t quoted at all.[/li]
[li]You neither support multiple rows nor make the prepared statement available for reuse. If the user has to insert 1,000 rows, he has no choice but to create 1,000 prepared statements.[/li]
[li]You claim that it’s possible to make MySQL ignore errors. I’m not aware of any such setting, but if it actually exists, it’s weird to throw an exception when the user has explicitly disabled error handling.[/li][/ul]
So there’s still a lot to do, and the security problems are in fact unsolvable. It’s even harder for the user to write secure code, because he has to understand that some parameters are automatically safe while others aren’t, and he has to basically assemble the query in his mind to understand how the input will affect the resulting query. Doesn’t that defeat the entire purpose of an abstraction layer?
I understand that database helper functions look like a good idea, because they’re seemingly easy to implement, and they allow the user to save a few characters. But I’m afraid this is an illusion. Proper abstraction is damn hard, and the problems can easily outweigh the benefits.
I chime in my .02 cents, the foundation of any database table is having sound column names. For example if you have a table called users then you would expect column names of username, password, city, etc…
You can still have an array, have multiple records entered and still have it safe from the nasties. ;D
For example:
[php]$sql = "INSERT INTO user (username, password, city) VALUES ';
/* THEN THE FUN STUFF STARTS */
foreach ($myArray as $value) {
$query = ‘(:username’ . $n . ‘, :password’ . $n . ‘, :city’ . $n . ‘)’;
$record = (object) $value; // Like working with objects more than arrays:
$iData[‘username’ . $n] = $record->username;
$iData[‘password’ . $n] = $record->password;
$iData[‘city’ . $n] = $record->city;
$n += 1;
}
/* Now the actual insertion of the array values */
if (!empty($query)) {
$sql .= implode(’, ', $query);
$stmt = $pdo->prepare($sql);
$result = $stmt->execute($iData);
}[/php]
You still can had throw exceptions and other security measures, but the point I am trying to get across as long as you have a strong naming foundation for the database table the rest can be easily secured. Though I have to admit I don’t usually use arrays for mass insertions into a database table.
I didn’t know the column names, so I hard coded them in, probably shouldn’t have column names as variables.
If the OP has spaces in his column names, he will have had to already handle errors when making the table so I consider that an assumption.
The OP didn’t ask for multiple inserts, he asked for “An insert function” not “multiple insert function”
Again, assumptions. Based on this point, I raise you "the user has mysql.dll disabled in the ini file… Do we show the OP code with <? or <?php. The above was an example which said “Edit to your liking” with a link to the source.
Well, either implement a proper function which actually works, or don’t implement a function at all. What’s the point of posting some half-finished code which comes with dozens of undocumented assumptions, restrictions and TODOs? If the OP knew how to fix your code, he wouldn’t need it in the first place.
If you want a single [tt]INSERT[/tt] query with hard-coded column names, forget about functions and simply create a prepared statement.
The problem is that your function doesn’t say that anywhere. It seemingly offers an abstraction layer where the user can just enter a bunch of key-value pairs, and then the function automatically updates the table. How on earth is the user supposed to know that he actually has to SQL-escape the array keys before then can pass them to your function? What’s even more confusing is that the array values must not be escaped.
In other words: You expect the user to wade through your source code, recognize the security problems and fix them. And if the user doesn’t do that, they end up with security vulnerabilities. Great.
What error? A space within an identifier is perfectly valid. The problem is that your function cannot handle this, because it will try to use this string as a parameter name (which of course leads to a syntax error).
Then what’s the whole point of having a function?
My point is: Your function contains extra code for the case that the user suppresses all MySQL errors (is that even possible?). In this case, you throw an error nontheless, which is obviously nonsensical. No error means no error.
Ok, enough of trolling which evidently you are. Post your solution. I merely pointed someone in the right direction. I didn’t even write the code myself so I don’t know why I’m even defending myself. You seem like a troll trying to act like DeathShadow. He’s a genius, you are not. Post your solution and let us pick it apart.
If a few technical remarks already hurt your feelings, then appearently you’ve never received a proper code review. That’s too bad, because honest discussions are crucial for learning and making progress.
What’s funny is that you actually seem to have copied and pasted “your” solution from the code review site of StackExchange. So you steal code from somebody who specifically asks for criticizm, and then you tell us not to criticize “your” suggestion. See, that’s the problem of mindless copy-and-paste.
Regarding my solution: I already told you what I would do. I’d create a single prepared statement. This is secure, simple and gets the job done.
At most, I’d create a simple helper function:
[php]/**
Execute a query or prepared statement
@param PDO $connection the database connection
@param string $query the query or query template
@param array $parameters optional parameters for the query template
*/
function query(PDO $connection, $query, array $parameters = [])
{
