Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to convert myself from using mysql extension to using PDO, with that being said I was curious if I am going about this the right way.

I have a custom logging system that I built I am wondering if the way I implemented this would work or not. I am planning on duplicating the main functions of PDO and doing the same as what I did prepare?

What I wanted to know is if the way I turned on exceptions and internally using try/catch blocks to capture and log errors then rethrowing the exception after I logged the error is the right way to do this or is there another more efficient way of doing this.

class rDB extends PDO {

private $dsn    = "mysql:host=10.10.10.43";
private $user   = "user";
private $pass   = "pass";

/** 
 * @var Logger Writes messages to custom csv log based on debug level of 1-5
 * 1 Being Critical Error, 5 Being a simple Note
 */
private $logger;

public function __construct($logger) {

    $this->logger = $logger;
    $this->logger->log(5, 'Connecting to MySQL DB');

    try {
        /* Call to Parent Function */
        parent::__construct($this->dsn, $this->user, $this->pass);

        $this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        $this->logger->log(5, 'Connected to MySQL DB');
    }
    catch(PDOException $e) {
        $this->logger->log(2, 'Connection to MySQL DB Failed-> '.$e->getPrevious());
        throw $e;
    }
}

public function prepare($statement, array $driver_options = 'array()') {
    try {
        $this->logger->log(5, 'Preparing statement');
        /* Call to Parent Function */
        $value = parent::prepare($statement, $driver_options);
    }
    catch(PDOException $e) {
        $this->logger->log(2, 'Error preparing statement-> '.$e->getMessage());
        throw $e;
    }
    $this->logger->log(5, 'Statement is prepared.');
    return $value;
}
}
share|improve this question
add comment

1 Answer

up vote 3 down vote accepted

This mostly looks good to me. I don't think that logging exceptions should be the concern of your rDB class. Also, you could utilize the PSR logger interface

class rDB extends PDO implements \Psr\Log\LoggerAwareInterface
{
    //do not override the PDO constructor

    public function setLogger(\Psr\Log\LoggerInterface $logger)
    {
        $this->logger = $logger;
    }

    //override the PDO methods as you have done but, call
    //$this->log() instead of $this->logger->log()
    //i.e.
    public function prepare($statement, $driver_options = array())
    {
        $this->log(\Psr\Log\LogLevel::INFO, $statement, array(
          'driver_options' => $driver_options
        ));
        return parent::prepare($statement, $driver_options);
    }

    private function log($level, $message, $context = array())
    {
        //note: only log if a logger is set
        if ($this->logger) {
            $this->logger($level, $message, $context);
        }
    }
}

A factory method added to your rDb class could be useful to construct an instance for you:

public static function createInstance($params)
{
    $dsn = array_key_exists('dsn', $params) ? $params['dsn']: null;
    $user = array_key_exists('user', $params) ? $params['user'] : null;
    $pass = array_key_exists('pass', $params) ? $params['pass']: null;
    $logger = array_key_exists('logger', $params) ? $params['logger']: null;

    $rdb = new rDB($dsn, $user, $pass);
    $rdb->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    if ($logger) {
        $rdb->setLogger($logger);
    }

    return $rdb;
}

EDIT I suggest using a factory method to construct your instance instead of overriding the constructor primarily because 1) Changing the argument list in any overridden method effectively changes the interface. This makes it more difficult to replace one class with another. 2) I feel that constructors should do no more than serve as a way to inject dependencies that are always needed and initialize default values.

In your application, somewhere during initialization/bootstrap:

//Depending on your environment (production, development, etc),
//you can configure different kinds of loggers. For example,
//a development logger would probably log EVERYTHING but a
//production logger might only log error, critical, alert, emergency
$logger = new YourLoggerClassWhichImplementsLoggerInterface();

try {
    $dbConfig = //read in a configuration from somewhere
    $rdb = rDB::createInstance(array(
        'dsn' => $dbConfig->dsn,
        'user' => $dbConfig->user,
        'pass' => $dbConfig->pass,
        'logger' => $logger,
    ));

    //Now pass $rdb to whatever needs it (i.e. a controller).
} catch (Exception $e) {
    $logger->error($e->getMessage(), array('exception' => $e));
    //Display some kind of error page to the user
}
share|improve this answer
    
Thank you for your suggestions, I see your point about logging exceptions in rDB, I found I was tempted to log the exceptions again when i was implementing the rDB class. Is there a reason why you wouldnt override the contructor as long as you called the parent? –  McGee.WIll Feb 24 '13 at 4:23
    
I don't see a reason to override the PDO constructor. My preference is to create factory methods to construct objects in some particular way (as I've shown in the createInstance method above). I find factory methods to be more flexible than overridden constructors. –  Rob Apodaca Feb 24 '13 at 13:27
    
I am not familar with factory methods or how they work, ( I will look into them ) I am still new to PHP, I am using WebORB for PHP to build a dataservice for AS3 project and its been a big learning curve for me. Thanks for your suggestions –  McGee.WIll Feb 25 '13 at 19:45
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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