PDO (MySQL) connection and query class, safety and logic

Question

For the last few day I tried to figure out how to work with MySQL and PDO. Although I've tried to read a lot about the subject, there are still a lot of things I don't understand.

Because of this lack of knowledge, I can't really judge this code (or other example code on-line) on its safety and logic, and therefore was hoping I could get some feedback on it.

The class makes a connection and returns a query:

class Connection
{
private $username = "xxxx";
private $password = "xxxx";
private $dsn = "mysql:host=host;dbname=name_db";
private $sql;
private $DBH;

function setSQL($sql){
    $this->sql = $sql; //if $sql is not set it troughs an error at PDOException
}

public function query()
{
    //the connection will get established here if it hasn't been already
    if (is_null($this->DBH))
        try {  
            $this->DBH = new PDO( $this->dsn, $this->username, $this->password );
            $this->DBH->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

            //query 
            $result = $this->DBH->prepare($this->sql); 
            $result->execute();
            return $result->fetch(PDO::FETCH_ASSOC);                

        } catch(PDOException $e) {  

            echo "I'm afraid I can't do that1.";  
            //file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);  
        }
}

//clear connection and variables
function __destruct(){
        $this->DBH = null;
}

}

Using the class:

$sql = "SELECT * from stock WHERE id = 302"; 
$test = new Connection();
$test->setSQL($sql);
echo $test->query();

Answer 1

1) When using PDO, you want to get the benefits of automatic parameter escaping. By wrapping your PDO class you are limiting it's functionality. Check out this question for a better example:

https://stackoverflow.com/questions/6366661/php-pdo-parameterised-query-for-mysql

2) Every time you run a query you are making a new PDO instance. It might be better to have an application resource pool that you can call to get a preconfigured db handle.

3) You wrote some sql to be able to fetch a stock by ID, that should probably be functionality that is reusable.

combining 1 & 2 & 3 .. code would probably look better as something like this:

class ApplicationResourcePool 
{
    static var $_dbHandle;

    private static $_dbConfig = array(
        'dsn'      => '',
        'username' => '',
        'password' => '',

    );
    public static getDbHandle(){
        if(self::$_dbHandle == null){
            self::$_dbHandle = new PDO(
                self::$_dbConfig['dsn'] , 
                self::$_dbConfig['username'], 
                self::$_dbConfig['password'] 
            );
        }
        return self::$_dbHandle;
    }

}

class StockMapper
{
    protected $_dbh; 
    public __construct($dbh = null)
    {
        if($dbh == null){
            $this->_dbh = ApplicationResourcePool::getDbHandle();
        } else {
            $this->_dbh = $dbh;
        }

    }
    public getStockById($stockId){

        $sth=$this->_dbh->prepare("SELECT * from stock WHERE id = :stockId"); 
        $sth->bindParam(":stockId", $stockId);
        $sth->execute();
        $result = $sth->fetch(PDO::FETCH_ASSOC);
        return $result[0];
    }
}

your codeBlock then becomes:

$stockMapper = new StockMapper();
$stockData = $stockMapper->getStockById('302');

Answer 2

Two issues:

1) I would place the code to open the connection in the constructor.

2) You want to look into parametrized prepared statements.

You want to send a query like:

SELECT * from stock WHERE id = ?

Then send an array of parmeteters:

array(302)

This will prevent SQL injection.