4
\$\begingroup\$

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();
\$\endgroup\$
3
\$\begingroup\$

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');
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ perfect!, that seems like a more logic and better way to handle this. Would you have any advice how to handle the PDOException in this example? Would you add it as a function('s) to getStockById? \$\endgroup\$ – Rob Oct 31 '11 at 18:42
  • \$\begingroup\$ I'd probably log the exception in a logger and in turn throw another exception that my outer code could catch. Your outer code shouldn't need to know that the stock mapper is internally using a DB or PDO as it's data storage/retrieval. \$\endgroup\$ – Zak Oct 31 '11 at 19:04
  • \$\begingroup\$ The one other thing I would probably do is create a "Stock" class that my mapper could populate with the data array from the database. Then I get a first class object instead of a PHP array as a return value. \$\endgroup\$ – Zak Oct 31 '11 at 19:05
0
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I see the logic in there, but how do you deal with the different query's you need? like: SELECT stock.size people.age from sizes LEFT JOIN people ON stock.id = people.id? \$\endgroup\$ – Rob Oct 31 '11 at 18:19
  • \$\begingroup\$ Well you can have queries as complex as you wish but the user parameters will ultimately become ?'s. You can also create a query builder where you can say $test->leftjoin(table) and build up SQL. This will slow you down a bit though if the server is getting a lot of hits. \$\endgroup\$ – Len Oct 31 '11 at 18:26

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.