0

I have a form that accepts a few names and phone numbers and posts them to a PHP script which then inserts those values into a MySQL table. The code (PHP) is pretty straightforward as you can see here:

$sql = "INSERT INTO Contact_table (PHONE, NAME) VALUES ";
for ($i = 0; $i < $arr; ++$i){
    $sql .= "('".$numbers[$i]."', '".$names[$i]."'),";
}
$sql = substr($sql, 0, -1)." ON DUPLICATE KEY UPDATE name = COALESCE(VALUES(name), name);";
$connect = dbconn(PROJHOST,PROJDB,PROJDBUSER,PROJDBPWD);

$query = $connect->query($sql);
$connect = NULL;

And this works perfect except that it refuses to accept any names like O'Hara or D'Souza. I am vaguely aware of PDO's prepared statement having some use in this scenario but I couldn't make it work. What I tried was this:

$query = $connect->prepare($sql);
$query->execute();

Instead of this:

$query = $connect->query($sql);

Any hints? What am I missing?

TheLearner
  • 2,813
  • 5
  • 46
  • 94
  • 3
    Not only does your code not work for names like O'Hara, but is a huge security hole as well, being prone to SQL injection. Just follow the advice here... http://wiki.hashphp.org/PDO_Tutorial_for_MySQL_Developers . – zgguy May 03 '15 at 20:25
  • That's the exact reason (and of course the apostrophe issue too) I was keen on using the prepared statement but it didn't help, hence here. – TheLearner May 03 '15 at 20:34
  • 2
    There is no point in preparing a SQL statement with hard-coded parameter values in it... you need something like $stmt = $connect->prepare('INSERT INTO Contact_table (PHONE, NAME) VALUES(?, ?) ON DUPLICATE KEY UPDATE name = COALESCE(VALUES(name), name)'); before the loop and then inside the loop: $stmt->execute(array($numbers[$i], $names[$i])); – zgguy May 03 '15 at 20:49
  • @zgguy: Thanks for the explanation. However, won't this result in a lot of queries since we are executing one with each iteration of the loop? My goal is to execute the entire batch as a single query and that's why I've kept the query() function after the loop. Also, I just learned about the addslashes() function. Is that safe enough against injections? I ask this because this function does resolve my apostrophe issue. – TheLearner May 03 '15 at 21:05
  • Yes, it will cause a lot of queries but since you only prepare the statement once and execute it many times, it saves the database a lot of work since doesn't need to parse and compile every time a query is issued. You can also try to prepare a statement of the form INSERT INTO Contact_table (PHONE, NAME) VALUES (?, ?), (?, ?), ... and then bind all the variables at once. addslashes() will resolve you apostrophe issue but prepared statements are definitively the way to go, especially when you take security into account. – zgguy May 03 '15 at 21:13
  • In my scenario, there could be an indefinite number of entries to be inserted, even hundreds. Are you sure running hundreds of queries, albeit smaller ones, would be less expensive than the single large query? That's what worries me. Also, could you please elaborate on this bit: "You can also try to prepare a statement of the form INSERT INTO Contact_table (PHONE, NAME) VALUES (?, ?), (?, ?), ... and then bind all the variables at once."? If you think this can help minimize the number of transactions, please promote your comment as an answer. – TheLearner May 03 '15 at 21:29
  • A few hundred inserts using a prepared statement should not be a big deal. Also, I think MySQL has a limit on how much rows it can accept in a single insert statement. There is not much I can elaborate further; you just use question marks instead of concrete values in the prepare phase, and then bind as many concrete values as you have question marks using $stmt->bind(). – zgguy May 03 '15 at 21:54
  • One `INSERT` can have an almost unlimited number of rows to insert. (Some buffer size, perhaps defaulting to 8MB, is the limit.) – Rick James May 03 '15 at 23:00
  • I just inquired one of the shared hosting providers and they confirmed their default for max_allowed_packet is 256 MB for shared hosting customers which they can increase on request for VPS. Is this what drives the cap on query length? If so, it seems humongous and I wouldn't bother about a single large query ever breaching this cap! – TheLearner May 03 '15 at 23:55
  • + you would probably hit the post_max_size limit before hitting the max_allowed_packet one ;-) – Capsule May 04 '15 at 02:49
  • The benefits to learning PDO aside, it's almost always bad practice to write direct sql. If you were using an abstraction layer, you never would have encountered this problem in the first place. – Ryre May 04 '15 at 02:51

1 Answers1

2

There are several ways to handle your problem. You can escape your strings using real_escape_string however there, as you suggest better ways. The best one is through prepared statements using either mysqli or PDO.

Since you mention PDO, which is an excellent approach, here's how to handle it this way:

The first thing involves running the connection through try and catch and then you run your query with placeholders, and then bind your variables to these placeholders. Here's a simple way of how it works:

try {
$connect = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
 $stmt = $conn->prepare("INSERT INTO Contact_table (PHONE, NAME) VALUES (:PHONE, :NAME)");
 $stmt->bindParam(':PHONE', $PHONE);
 $stmt->bindParam(':NAME', $NAME);
//then name your variables like so
$PHONE = [whatever your phone number variable(s) is/are equal to];
$NAME = [whatever the NAME variable(s) is/are equal to];
$stmt->execute();
//note the last section can occur within a loop, much like you have above.
catch(PDOException $error)
}
{
echo "Error: " . $error->getMessage();
}
$connect = null;
}

Note, not only will this allow any fields with special characters such as apostrophes, it will run much faster for large queries, and have the added bonus of being a lot more secure, as it pretty much eliminates most methods of SQL injection.

You can also accomplish this with mysqli prepared statements, however PDO has the added advantage of being database agnostic, in that there are easy connections to databases other than MySQL.

Also, you can use the ? method for loosely defined parameters as mentioned in one of the comments, but this also lends itself to being able manipulate these as objects.

Here's a good reference if you wish to learn more: PHP Prepared Statements

nomistic
  • 2,902
  • 4
  • 20
  • 36
  • Thanks for the detailed explanation. But this still doesn't address my problem because I am collating multiple (a whole lot actually) insert actions using a single INSERT query built within a loop. If I use the prepare-bind method (which I really do want to), I am unable to retain the query as a single entity as the execute() function would have to run once with each iteration of the loop. This will potentially result in a lot of queries which I want to avoid. Is there any way to use prepare-bind while retaining the single-query goal in my original code? – TheLearner May 04 '15 at 03:49
  • Yes, anytime you call a variable, you can replace it with the bound parameters. For instance `$numbers[$i]` and `$names[$i]` can be turned into parameters,it's a good idea to convert that to a cleaner variable before you call it. You also shouldn't need multiple parameters. You can always run through a loop within the `try` to get multiple values. There is also no limit to the number you can feed in . just run each variable iteration through `$stmt->execute();` in your loop and it will go in fine. It's one the best things about prepared statements; – nomistic May 04 '15 at 03:56
  • To shorten this, one query is all you need. you can get those other items with a `while` or `foreach` loop before executing the query on them. – nomistic May 04 '15 at 03:58
  • If $stmt->execute() is run within a loop, won't it perform more than one query against the database since it will execute once with every iteration? – TheLearner May 04 '15 at 08:56
  • the way that PDO works is that it opens a database connection, opens a query and then puts in all of the rows it needs. The `PDO` is the class. all of this this occurs within the `try` and it doesn't close the connection until you are done. It treats the entire query as an object, and the `execute` is one of the methods within it applied against the properties you are feeding into it. It's actually more efficient, and runs much faster, with the huge positive side effect of being more secure. "try" it out (pun intended). – nomistic May 04 '15 at 12:51
  • Here's an example of how to do this: http://stackoverflow.com/a/15386231/3044080. the execute could come after the loop. – nomistic May 04 '15 at 12:59