Browse files

API Added PDOMySQLDatabase for connectivity to mysql via the PDO data…

…base connector. This includes a refactor of MySQLDatabase to support extension of this class.
  • Loading branch information...
1 parent 28bd939 commit d9a4a628f1e8f6d348c534eca4d9f114ea7d0199 @tractorcow committed Oct 16, 2012
Showing with 227 additions and 32 deletions.
  1. +2 −2 model/Database.php
  2. +60 −30 model/MySQLDatabase.php
  3. +165 −0 model/PDOMySQLDatabase.php
View
4 model/Database.php
@@ -1009,12 +1009,12 @@ public function releaseLock($name) {
* The number of the current row in the interator.
* @var int
*/
- private $rowNum = -1;
+ protected $rowNum = -1;
/**
* Flag to keep track of whether iteration has begun, to prevent unnecessary seeks
*/
- private $queryHasBegun = false;
+ protected $queryHasBegun = false;
/**
* Return an array containing all the values from a specific column. If no column is set, then the first will be
View
90 model/MySQLDatabase.php
@@ -12,23 +12,23 @@ class MySQLDatabase extends SS_Database {
* Connection to the DBMS.
* @var resource
*/
- private $dbConn;
+ protected $dbConn;
/**
* True if we are connected to a database.
* @var boolean
*/
- private $active;
+ protected $active;
/**
* The name of the database.
* @var string
*/
- private $database;
+ protected $database;
- private static $connection_charset = null;
+ protected static $connection_charset = null;
- private $supportsTransactions = true;
+ protected $supportsTransactions = true;
/**
* Sets the character set for the MySQL database connection.
@@ -54,29 +54,55 @@ public static function set_connection_charset($charset = 'utf8') {
* - timezone: (optional) The timezone offset. For example: +12:00, "Pacific/Auckland", or "SYSTEM"
*/
public function __construct($parameters) {
- $this->dbConn = new MySQLi($parameters['server'], $parameters['username'], $parameters['password']);
- if($this->dbConn->connect_error) {
- $this->databaseError("Couldn't connect to MySQL database | " . $this->dbConn->connect_error);
- }
+ $this->dbConn = $this->getConnect($parameters);
- $this->query("SET sql_mode = 'ANSI'");
-
- if(self::$connection_charset) {
- $this->dbConn->set_charset(self::$connection_charset);
+ $this->setSQLMode('ANSI');
+ $this->selectDatabase($parameters['database']);
+
+ if(isset($parameters['timezone'])) {
+ $this->selectTimezone($parameters['timezone']);
}
-
- $this->active = $this->dbConn->select_db($parameters['database']);
- $this->database = $parameters['database'];
-
- if(isset($parameters['timezone'])) $this->query(sprintf("SET SESSION time_zone = '%s'", $parameters['timezone']));
+ }
+
+ public function setSQLMode($mode) {
+ if(empty($mode)) return;
+ $this->query(sprintf(
+ "SET sql_mode = '%s'",
+ $this->addslashes($mode)
+ ));
+ }
+
+ /**
+ * Sets the system timezone for the database connection
+ * @param string $timezone
+ */
+ public function selectTimezone($timezone) {
+ if(empty($timezone)) return;
+ $this->query(sprintf(
+ "SET SESSION time_zone = '%s'",
+ $this->addslashes($timezone)
+ ));
}
/**
- * Not implemented, needed for PDO
+ * Generates a new connection with the given parameters
+ * @see MySQLDatabase::__construct() for a description of the parameters
+ * @parameters array
+ * @return resource
*/
public function getConnect($parameters) {
- return null;
+ $connection = new MySQLi($parameters['server'], $parameters['username'], $parameters['password']);
+
+ if($connection->connect_error) {
+ $this->databaseError("Couldn't connect to MySQL database | " . $connection->connect_error);
+ }
+
+ if(self::$connection_charset) {
+ $connection->set_charset(self::$connection_charset);
+ }
+
+ return $connection;
}
/**
@@ -124,7 +150,7 @@ public function query($sql, $errorLevel = E_USER_ERROR) {
Debug::message("\n$sql\n{$endtime}ms\n", false);
}
- if(!$handle && $errorLevel) $this->databaseError("Couldn't run query: $sql | " . $this->dbConn->error, $errorLevel);
+ if(!$handle && $errorLevel) $this->databaseError("Couldn't run query: $sql | " . $this->getLastError(), $errorLevel);
return new MySQLQuery($this, $handle);
}
@@ -137,13 +163,8 @@ public function isActive() {
}
public function createDatabase() {
- $this->query("CREATE DATABASE \"$this->database\"");
- $this->query("USE \"$this->database\"");
-
- $this->tableList = $this->fieldList = $this->indexList = null;
-
- $this->active = $this->dbConn->select_db($this->database);
- return $this->active;
+ $this->query("CREATE DATABASE \"{$this->database}\"");
+ return $this->selectDatabase($this->database);
}
/**
@@ -187,8 +208,8 @@ public function selectDatabase($dbname) {
* Returns true if the named database exists.
*/
public function databaseExists($name) {
- $SQL_name = Convert::raw2sql($name);
- return $this->query("SHOW DATABASES LIKE '$SQL_name'")->value() ? true : false;
+ $SQL_name = $this->quote($name);
+ return $this->query("SHOW DATABASES LIKE $SQL_name")->value() ? true : false;
}
/**
@@ -557,6 +578,10 @@ public function tableList() {
public function affectedRows() {
return $this->dbConn->affected_rows;
}
+
+ public function getLastError() {
+ return $this->dbConn->error;
+ }
public function databaseError($msg, $errorLevel = E_USER_ERROR) {
// try to extract and format query
@@ -925,6 +950,11 @@ public function dbDataType($type){
public function addslashes($value){
return $this->dbConn->real_escape_string($value);
}
+
+ public function quote($value) {
+ $value = $this->addslashes($value);
+ return "'$value'";
+ }
/*
* This changes the index name depending on database requirements.
View
165 model/PDOMySQLDatabase.php
@@ -0,0 +1,165 @@
+<?php
+
+/**
+ * MySQL connector class using PDO
+ *
+ * @package framework
+ * @subpackage model
+ */
+class PDOMySQLDatabase extends MySQLDatabase {
+ /**
+ * The most recent statement returned from PDOMySQLDatabase->query
+ * @var PDOStatement
+ */
+ protected $lastStatement = null;
+
+ public function getConnect($parameters) {
+
+ // Build DSN string
+ $dsn = sprintf("mysql:host=%s;dbname=%s", $parameters['server'], $parameters['database']);
+ if (!empty($parameters['port'])) {
+ $dsn .= ";port=" . $parameters['port'];
+ }
+
+ if (!empty(self::$connection_charset)) {
+ $dsn .= ';charset=' . self::$connection_charset;
+ }
+
+ // Connection commands to be run on every re-connection
+ $options = array(
+ PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8',
+ );
+
+ // May throw a PDOException if fails
+ return new PDO($dsn, $parameters['username'], $parameters['password'], $options);
+ }
+
+ /**
+ * Get the version of MySQL.
+ * @return string
+ */
+ public function getVersion() {
+ return $this->dbConn->getAttribute(PDO::ATTR_SERVER_VERSION);
+ }
+
+ public function addslashes($value) {
+ $value = $this->quote($value);
+ // Since the PDO library quotes the value, we should remove this to maintain
+ // consistency with MySQLDatabase::addslashes
+ if (preg_match('/^\'(?<value>.+)\'$/', $value, $matches)) {
+ $value = $matches['value'];
+ }
+ return $value;
+ }
+
+ public function quote($value) {
+ return $this->dbConn->quote($value);
+ }
+
+ public function query($sql, $errorLevel = E_USER_ERROR) {
+ if (isset($_REQUEST['previewwrite']) && in_array(strtolower(substr($sql, 0, strpos($sql, ' '))), array('insert', 'update', 'delete', 'replace'))) {
+ Debug::message("Will execute: $sql");
+ return;
+ }
+
+ if (isset($_REQUEST['showqueries']) && Director::isDev(true)) {
+ $starttime = microtime(true);
+ }
+
+ $this->lastStatement = $this->dbConn->query($sql);
+
+ if (isset($_REQUEST['showqueries']) && Director::isDev(true)) {
+ $endtime = round(microtime(true) - $starttime, 4);
+ Debug::message("\n$sql\n{$endtime}ms\n", false);
+ }
+
+ if (!$this->lastStatement && $errorLevel) {
+ $this->databaseError("Couldn't run query: $sql | " . $this->getLastError(), $errorLevel);
+ }
+
+ return new PDOMySQLStatement($this->lastStatement);
+ }
+
+ function getLastError() {
+ $error = $this->dbConn->errorInfo();
+ if ($error)
+ return sprintf("%s-%s: %s", $error[0], $error[1], $error[2]);
+ }
+
+ public function getGeneratedID($table) {
+ return $this->dbConn->lastInsertId();
+ }
+
+ /**
+ * Return the number of rows affected by the previous operation.
+ * @return int
+ */
+ public function affectedRows() {
+ if(empty($this->lastStatement)) return 0;
+ return $this->lastStatement->rowCount();
+ }
+
+ /**
+ * Switches to the given database.
+ * If the database doesn't exist, you should call createDatabase() after calling selectDatabase()
+ */
+ public function selectDatabase($dbname) {
+ $this->database = $dbname;
+ $this->tableList = $this->fieldList = $this->indexList = null;
+ if($this->active = $this->databaseExists($this->database)) {
+ $this->query("USE \"{$this->database}\"");
+ }
+ return $this->active;
+ }
+}
+
+/**
+ * A result-set from a MySQL database.
+ * @package framework
+ * @subpackage model
+ */
+class PDOMySQLStatement extends SS_Query {
+ /**
+ * The internal MySQL handle that points to the result set.
+ * @var PDOStatement
+ */
+ protected $statement = null;
+
+ protected $results = null;
+
+ /**
+ * Hook the result-set given into a Query class, suitable for use by SilverStripe.
+ * @param handle the internal mysql handle that is points to the resultset.
+ */
+ public function __construct(PDOStatement $statement) {
+ $this->statement = $statement;
+ // Since no more than one PDOStatement for any one connection can be safely
+ // traversed, each statement simply requests all rows at once for safety.
+ // This could be re-engineered to call fetchAll on an as-needed basis
+ $this->results = $statement->fetchAll();
+ }
+
+ public function __destruct() {
+ $this->statement->closeCursor();
+ }
+
+ public function seek($row) {
+ $this->rowNum = $row - 1;
+ return $this->nextRecord();
+ }
+
+ public function numRecords() {
+ return count($this->results);
+ }
+
+ public function nextRecord() {
+ $index = $this->rowNum + 1;
+
+ if (isset($this->results[$index])) {
+ return $this->results[$index];
+ } else {
+ return false;
+ }
+ }
+
+}

8 comments on commit d9a4a62

@halkyon

Looks good, seems to work similar to how SQLite3Database and SQLitePDODatabase are setup in the sqlite3 module.

Have you done any preliminary performance checks of PDOMySQLDatabase, compared to the original MySQLDatabase adapter?

@tractorcow
Owner

No, I haven't, and I apologise for not checking the SQLLite adaptor first. I didn't realise that there was an existing PDO implementation. I'll check this out and get back to you.

I still need to follow up with additional test cases, and implementation of paramaterised queries, so it'll be good to see how SQLLitePDO does this.

@sminnee

We might want to consider refactoring the database class to have to pieces:

  • A query generator
  • A query executor

The latter would have "PDO" as a single option that can be used, as well as executors using the native methods. For MSSQL, we would have one based on sqlsrv_ methods and another based on mssql_ methods.

Instead of being an abstract base class, the Database class would then be a shell class that you pass the two arguments into:

DB::setConn(new Database(
    new MySQLQueryGenerator(),
    new PDOQueryExecutor("mysql:host=localhost;dbname=SS_bla", "username", "password")
));

To preserve backward compatibility, we'd probably modify DB::connect() to construct the relevant pair of objects based on the $databaseConfig arrays being used.

The advantage of this approach is that you could separate the PDO part from appearing both in the subclass of SQLite3Database and in the subclass of MySQLDatabase.

@hafriedlander, @chillu, @nyeholt - does this make sense to you guys?

@tractorcow
Owner

Wow, that's a great idea. I really did struggle with the breakup of both the querying (pdo/mysql) as well as the server specific functions (management of indexes, database specific syntax). I'll look at this another day.

@sminnee

Potentially, you could even have a separate object for schema management (creation, inspection, and amendment of schema changes). It would reduce the "god-object"-ness of the current database classes.

@tractorcow
Owner

Hi Sam, I had a massive think about your suggestions over the evening;

I feel that we should at least in part refactor out the connectivity side of the database API into a set of encapsulated objects, that can be used by the SS_Database subclasses to control the way that they connect to the database. E.g. the MySQLDatabase could use either MySQLiConnector or PDOConnector. The PDOConnector could be re-used across other database modules to provide PDO encapsulation in a database agnostic manner.

I however do not feel that it would be wise to interact with the database exclusively through an instance of SS_Database, with a "schema management" object, as you put it. I feel that doing this through subclasses of SS_Database instead is probably the more appropriate way, as it better supports the Open/Closed Principle of OO development. I feel that trying to force all the functionality into a non-extendable class has these downsides:

  • Probably will result in over-engineering to make a one-size fits all class.
  • No matter how generic the code in SS_Database will be, everything should be overridable.
  • The current database connector modules would all need to be re-written at once (can't stagger implementation)

The current implementation works pretty well at providing for database specific functionality. It's probably best to have the parent object act as the "controller", with the connector injected as a dependency.

I'll work on another implementation which refactors out the code I've written above into a connector class. I suggest the name PDOConnector, and subsequently I can refactor the MySQLi code into a MySQLiConnector.

During installation, or within the database configuration file, we could probably specificy the connector mechanism in a way similar to the below:

$databaseConfig = array(
    "type" => 'MySQLDatabase',
    "connector" => 'PDOConnector', // Or MySQLiConnector
    "server" => 'localhost',
    "username" => 'db_dbuser',
    "password" => 'XXXXXXXXX',
    "database" => 'db',
    "path" => '',
);
@sminnee

Good analysis. It's also a smaller single change. Im in favour of your approach :-)

@nyeholt

Someone recently posted about having a hierarchy that looked like

  • PDODatabase extends SS_Database -- MySQLDatabase extends PDODatabase

What happened with that discussion? I seem to remember there being a few points in there that were interesting, but can't find it now.

Anyway - I like the idea of separating querying from the management side of things; SS_Database has always struck me as trying to do too many things

I however do not feel that it would be wise to interact with the database exclusively through an instance of SS_Database, with a "schema management" object, as you put it. I feel that doing this through subclasses of SS_Database instead is probably the more appropriate way, as it better supports the Open/Closed Principle of OO development. I feel that trying to force all the functionality into a non-extendable class has these downsides:

But it breaks the single responsibility principle because it's trying to do several things (eg schema management, query management) which in my opinion is a far bigger problem. And it doesn't necessarily break Open/Closed assuming the way other code refers to objects of the classes can be changed appropriately.

I would rather there be a discrete separation of SS_Database being responsible for querying, and a separate SchemaManager being responsible for manipulation of db structure (or querying about the db structure) that uses an instance of SS_Database for actually performing the queries. It would also potentially help refactor some of the code to do with table management out of DataObject (eg requireTable).

The following is just some hypothetical code changes that would make things nicer to work with (from my point of view)

class DataObject {
    public static $depenencies = array(
        'db'            => '%$PrimaryDatabase'
        'schemaManager'     => '%$SchemaManager'
    );

    public $db;
    public $schemaManager;

    public function requireTable() {
        // ...
        $this->schemaManager->requireTable("{$this->class}_$relationship", $manymanyFields, $manymanyIndexes, true, null, $extensions);
        // ...
    }

    public function write() {
        $this->db->query("INSERT INTO \"{$baseTable}\" (\"Created\") VALUES (" . DB::getConn()->now() . ")");
    }
}


class SchemaManager {
    protected $db;

    public function __construct(SS_Database $db) {
        $this->db = $db;
    }

    public function renameTable($oldTableName, $newTableName) {
        $this->db->query("ALTER TABLE \"$oldTableName\" RENAME \"$newTableName\"");
    }
}

class MySQLSchemaManager extends SchemaManager {

}

then some configuration

Injector
  PrimaryDatabase:
    class: MySQLDatabase
    constructor:
      - username
      - password
      - server
  SchemaManager:
    class: MySQLSchemaManager
    constructor:
      - %$PrimaryDatabase

From this point, if I wanted to have a secondary DB connection (ie maybe I want to split connections across to a read-only DB) I only need to change the configuration around (and write the splitting database implementation of course).

Injector
  WriteDatabase:
    class: MySQLDatabase
    constructor:
      - username
      - password
      - server
  ReadDatabase:
    class: MySQLDatabase
    constructor:
      - username
      - password
      - server
  PrimaryDatabase:
    class: QuerySplittingDatabase
    constructor:
      - %$WriteDatabase
      - %$ReadDatabase
  SchemaManager:
    class: MySQLSchemaManager
    constructor:
      - %$WriteDatabase

Point being, I've fundamentally changed the way SS interacts with the database, but the existing codebase doesn't need to change at all because it's just referring to its established dependencies.

Please sign in to comment.