Update data using PDO why it does not work

Question

<?php
$sTable = "mytable";    

$colUpdate = $_GET['field'];// database field name
$valUpdate = $_GET['val']; // the long string ,can be non-English   
$rowID = $_GET['id']; //number

    $pdo = PDO2::getInstance();
    $pdo->exec('SET NAMES utf8'); // for utf-8

    $sql = "UPDATE $sTable
            SET $colUpdate =:valUpdate
            WHERE id =:rowID ";      
    $req = $pdo->prepare($sql);
    $req->bindValue(":valUpdate",  $valUpdate);
    $req->bindValue(":rowID",  $rowID);

    $req->execute();
    $req->closeCursor();        
   ?>

What did I wrong here?Because it works if I did like this:

 <?php
    $sTable = "mytable";
    $pdo = PDO2::getInstance();
    $colUpdate = $_GET['field'];
    $valUpdate = $_GET['val'];      
    $rowID = $_GET['id'];

    $sQuery = " UPDATE $sTable SET  $colUpdate = '$valUpdate' WHERE  id = $rowID";
    $req = $pdo->prepare($sQuery);
    $req->execute();
    $req->closeCursor();    
?>

What you did wrong: you posted a question without describing the problem and you did not include any errors you're getting. — lanzz, Jun 20 '12 at 07:48
You are not doing any error checking for your query in your code. It is hence no wonder the code breaks when your query fails. See http://stackoverflow.com/questions/3726505/how-to-squeeze-error-message-out-of-pdo on how to get an error message out of PDO — Pekka, Jun 20 '12 at 07:48
make sure that `colUpdate` is not empty; also it seems that one of your columns is meant to be called `order` wich is both misspelled in your code, and reserved word in `MySQL`. If that is the case, you should use backticks around the table name — Maxim Krizhanovsky, Jun 20 '12 at 07:50
why are you using `bindvalue` when you can simply use those variables in the statement itself. — hjpotter92, Jun 20 '12 at 07:51
@T-ShirtDude using the bindParam() method the string will be escaped etc. @ user777297 Did you create a good instance of PDO? I don't see it in your code. — stUrb, Jun 20 '12 at 07:58
@stUrb you see my second option I used ,it works fines.thanks — sophie, Jun 20 '12 at 08:03
Between the last `bindValue` and `execute()` lines in the first codebox, put in this: `$err = $req->errorInfo(); echo $err[2];` and tell us what it says. — Zane Bien, Jun 20 '12 at 08:12
Did you try to specify the kind of parameter you bind with: `PDO::PARAM_STR` or `PDO::PARAM_INT` ? — stUrb, Jun 20 '12 at 08:21
okay ,now it WORKS,thanks all of you.the problem is that I have to put REQUEST: "&val="+encodeURIComponent(sValue)).so it will work all kind of string . — sophie, Jun 20 '12 at 08:24
By the way, the fact that you are inserting `$colUpdate` directly into the query completely defeats the purpose of using prepared statements. It is derived straight from `$_GET` without any filtration or validation, and thus you are a wide open call for SQL injection attack. The best thing to do for this would be to pass `$_GET['field']` through a `switch()` statement with the `cases` being the names of your tables. This would essentially force `$colUpdate` to be either of the `cases` in the switch and nothing else. Please do this for the sake of your security! — Zane Bien, Jun 20 '12 at 08:48
@ZaneBien: It's only an example to show that without binding the values to placeholders, his query works... — Madara's Ghost, Jun 20 '12 at 08:57
@Truth no, it is also in his first codebox which I'm assuming is ***not*** an "example"... — Zane Bien, Jun 20 '12 at 08:58

score 3 · Accepted Answer · edited Jun 20 '20 at 09:12

There are several issues in your code:

You are using a Singleton
You aren't checking for errors
You're passing GET variables directly.

Let's address each, shall we?

1. You are using a Singleton

Singletons are evil, they are set in the global space, which makes your application unstable, unreliable and untestable. Besides, what would you do if you needed another database connection?

Solution

Use a new PDO instance.

2. You aren't checking for errors

There aren't any error checking in your code, so if an error does come up, it is silently ignored.

Solution

Set PDO::ATTR_ERRMODE to PDO::ERRMODE_EXCEPTION in the constructor of PDO or using setAttribute. It also helps setting PDO::EMULATE_PREPARES to false.

3. You're passing GET variables directly into your query

You're passing $colUpdate directly inside your query, even if you are preparing the statement, variables passed directly into the query strings are not escaped.

Solution

Pass it in a placeholder, and bind the value. Also, your structure is most likely flawed if you need user input to determine the column you're updating.

After all of those, I come to the following code:

<?php
/*
 * Variable Initialization
 */
/** @var $table_name string Name of the table to insert */
$table_name = "mytable";

/**
 * @var $field_to_update string Name of field to update
 * @deprecated Should not be used! Database restructuring needed!
 */
$field_to_update = mysql_real_escape_string($_GET['field']); //At least escape it!

/** @var $value_to_insert string */
$value_to_insert = $_GET['val'];

/** @var $row_id integer */
$row_id = $_GET['id'];

$pdo = new PDO("mysql:host=localhost;dbname=database_name", "user", "password");
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
$pdo->exec('SET NAMES utf8'); // for utf-8

$sql = <<<MySQL
UPDATE $table_name
    SET $field_to_update = :valUpdate
    WHERE id = :rowID
MySQL;

$req = $pdo->prepare($sql);
$req->bindValue(":valUpdate", $value_to_insert, PDO::PARAM_STR);
$req->bindValue(":rowID", $row_id, PDO::PARAM_INT);

$req->execute();

Update data using PDO why it does not work

1 Answers1

1. You are using a Singleton

Solution

2. You aren't checking for errors

Solution

3. You're passing GET variables directly into your query

Solution