4
\$\begingroup\$

I have decided to go PDO and have started implementing it on one of my webprojects. I have limited programing skills but can usually pull it off with some tutorials and forums.

Here is my code this far and it works fine but how is the code correct regarding the picking up errors, syntax, order and begintransaction etc.? Have I missunderstood anything? Is anything unneccesary?

connect.php

 <?php
try {
    $DBH = new PDO("mysql:host=$host;dbname=$database", $username, $password, array(
        PDO::ATTR_PERSISTENT => true
    ));
    $DBH -> exec("set names utf8");
}
catch(PDOException $e) {
    echo $e->getMessage();
}
?>

query.php

try {  
        $DBH->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        $DBH->beginTransaction();   

        $STH = $DBH->prepare('SELECT id from users where uid = :uid');
        $STH->setFetchMode(PDO::FETCH_ASSOC); 
        $STH->bindParam(':uid', $uid); // $uid value is set
        $STH->execute();

        $uid_in_db = $STH->rowCount();
        if($uid_in_db==0){ //=new user, insert info in db.
            $STH = $DBH->prepare("INSERT INTO USERS (uid,namn) VALUES (:uid, :name)");
            $STH->bindParam(':uid', $uid);
            $STH->bindParam(':namn', $_POST['namn']); // a value posted form user input
            $STH->execute();
        }

        $DBH->commit(); // 

    } catch (Exception $e) {
        $DBH->rollBack();
        echo "Fel: " . $e->getMessage();
    }
}else{
    $error=1;
}
\$\endgroup\$
3
  • \$\begingroup\$ I don't see any need for transactions. You only have 1 insert. Do you plan to add more write queries? Note that by beginning a transaction you don't issue any implicit locks on the tables. \$\endgroup\$ – Alin Purcaru Apr 18 '11 at 14:12
  • \$\begingroup\$ You may want to check out the disadvantages of using persistent connections with PDO. You may not really need to use that option. What are the disadvantages of using persistent connection in PDO? \$\endgroup\$ – TaylorOtwell Apr 18 '11 at 14:15
  • \$\begingroup\$ Thanks, I changed that since now. What do you mean by mean with don't issue any implicit locks on the tables? thanks! \$\endgroup\$ – Joseph Apr 18 '11 at 14:31
3
\$\begingroup\$

Instead of checking for a UID collision before an insert, I would put a unique key on that column, and then write fallback code for the case (very very unlikely) when two collide. That way you can simplify your logic and reduce the number of queries to one instead of two. You also don't need a transaction if you implement this change, since there is only 1 query.

//you can remove the outer try/catch since only the execute() should possibly fail
$DBH->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); //<~ put this in the initialize of the db connection
$STH = $DBH->prepare("INSERT INTO USERS (uid,namn) VALUES (:uid, :name)");
$STH->bindParam(':uid', $uid);
$STH->bindParam(':namn', $_POST['namn']); // a value posted form user input
try {
    $STH->execute();
} catch( PDOException $e ) {
    //deal with collision 
}
\$\endgroup\$
5
  • \$\begingroup\$ 1.I put the errormode in under $DBH -> exec("set names utf8"); is that OK? 1b.what is the differende from catch(PDOException $e)and catch(Exception $e)? 2. Ichanged so that the try is only around the $STH->execute();is that something I can always follow? 3.I did not quite understand the uid as key and the fallback case. But I would really like to slim the script. Though I might need to specify that the uid is the uid from FBAPI that is set when a user login to the site and the script should see if the user is in the db or show the form to add info. so the uid will in most cases exist. \$\endgroup\$ – Joseph Apr 18 '11 at 14:35
  • \$\begingroup\$ @Joseph PDOException is more correct, since it would let any other exception (which would be more serious) to be emitted (I updated my example). If you put a unique key on column in the database, the database will reject any duplicate uid entires and cause $STH->execute() to raise an exception. \$\endgroup\$ – Kendall Hopkins Apr 18 '11 at 14:43
  • \$\begingroup\$ Ok now I understand but do you think it is a good solution since as I described the id will in most cases be a duplicate (for every pageload by a returning user) and thus be an error. Also question 1 in comment above, is the catch error line put correctly and should I remove it when in production? Thanks! \$\endgroup\$ – Joseph Apr 18 '11 at 14:58
  • \$\begingroup\$ @Joseph Taking that into consideration, I might actually lean towards you original solution. Since the normal case would be to only run 1 query. I was under the impression that you were creating a user entity, not a session. I wouldn't consider it good practice to use an insert to check if a row already exists. \$\endgroup\$ – Kendall Hopkins Apr 18 '11 at 15:08
  • \$\begingroup\$ Ok thanks, yes I was a bit unclear. Can you please look at the other questions I gave in the comments above regarning your other suggestions. thanks \$\endgroup\$ – Joseph Apr 18 '11 at 15:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.