From 0111b98b18a273ca5c37013d880c5d21f94396af Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Thu, 4 Oct 2018 16:13:32 +1300 Subject: [PATCH] FIX: Ensure that types are preserved fetching from database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures that numeric fields appear in PHP as int/float values rather than strings, which allows the development of more type-safe PHP code. This doesn’t work on the legacy mysql driver and this will now throw a notice-level error. It requires mysqlnd. --- src/ORM/Connect/MySQLiConnector.php | 12 +++++++ src/ORM/Connect/PDOConnector.php | 34 ++++++++++++++++---- src/ORM/Connect/PDOQuery.php | 42 +++++++++++++++++++++++-- tests/php/ORM/DatabaseTest.php | 34 ++++++++++++++++++++ tests/php/ORM/DatabaseTest/MyObject.php | 5 ++- 5 files changed, 118 insertions(+), 9 deletions(-) diff --git a/src/ORM/Connect/MySQLiConnector.php b/src/ORM/Connect/MySQLiConnector.php index 43805c705..ba118db81 100644 --- a/src/ORM/Connect/MySQLiConnector.php +++ b/src/ORM/Connect/MySQLiConnector.php @@ -78,6 +78,18 @@ class MySQLiConnector extends DBConnector $this->dbConn = mysqli_init(); + // Use native types (MysqlND only) + if (defined('MYSQLI_OPT_INT_AND_FLOAT_NATIVE')) { + $this->dbConn->options(MYSQLI_OPT_INT_AND_FLOAT_NATIVE, true); + + // The alternative is not ideal, throw a notice-level error + } else { + user_error( + 'mysqlnd PHP library is not available, numeric values will be fetched from the DB as strings', + E_USER_NOTICE + ); + } + // Set SSL parameters if they exist. All parameters are required. if (array_key_exists('ssl_key', $parameters) && array_key_exists('ssl_cert', $parameters) && diff --git a/src/ORM/Connect/PDOConnector.php b/src/ORM/Connect/PDOConnector.php index 7d9f1af4a..6cea5590e 100644 --- a/src/ORM/Connect/PDOConnector.php +++ b/src/ORM/Connect/PDOConnector.php @@ -64,6 +64,12 @@ class PDOConnector extends DBConnector */ protected $cachedStatements = array(); + /** + * Driver + * @var string + */ + private $driver = null; + /** * Flush all prepared statements */ @@ -113,10 +119,11 @@ class PDOConnector extends DBConnector { $this->flushStatements(); - // Build DSN string // Note that we don't select the database here until explicitly // requested via selectDatabase - $driver = $parameters['driver'] . ":"; + $this->driver = $parameters['driver']; + + // Build DSN string $dsn = array(); // Typically this is false, but some drivers will request this @@ -195,13 +202,18 @@ class PDOConnector extends DBConnector $options[PDO::MYSQL_ATTR_SSL_CIPHER] = array_key_exists('ssl_cipher', $parameters) ? $parameters['ssl_cipher'] : self::config()->get('ssl_cipher_default'); } - if (self::is_emulate_prepare()) { - $options[PDO::ATTR_EMULATE_PREPARES] = true; + // Set emulate prepares (unless null / default) + $isEmulatePrepares = self::is_emulate_prepare(); + if (isset($isEmulatePrepares)) { + $options[PDO::ATTR_EMULATE_PREPARES] = (bool)$isEmulatePrepares; } + // Disable stringified fetches + $options[PDO::ATTR_STRINGIFY_FETCHES] = false; + // May throw a PDOException if fails $this->pdoConnection = new PDO( - $driver . implode(';', $dsn), + $this->driver . ':' . implode(';', $dsn), empty($parameters['username']) ? '' : $parameters['username'], empty($parameters['password']) ? '' : $parameters['password'], $options @@ -213,6 +225,16 @@ class PDOConnector extends DBConnector } } + + /** + * Return the driver for this connector + * E.g. 'mysql', 'sqlsrv', 'pgsql' + */ + public function getDriver() + { + return $this->driver; + } + public function getVersion() { return $this->pdoConnection->getAttribute(PDO::ATTR_SERVER_VERSION); @@ -383,7 +405,7 @@ class PDOConnector extends DBConnector } elseif ($statement) { // Count and return results $this->rowCount = $statement->rowCount(); - return new PDOQuery($statement); + return new PDOQuery($statement, $this); } // Ensure statement is closed diff --git a/src/ORM/Connect/PDOQuery.php b/src/ORM/Connect/PDOQuery.php index f5a9739b3..826432219 100644 --- a/src/ORM/Connect/PDOQuery.php +++ b/src/ORM/Connect/PDOQuery.php @@ -22,16 +22,54 @@ class PDOQuery extends Query * Hook the result-set given into a Query class, suitable for use by SilverStripe. * @param PDOStatement $statement The internal PDOStatement containing the results */ - public function __construct(PDOStatement $statement) + public function __construct(PDOStatement $statement, PDOConnector $conn) { $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(PDO::FETCH_ASSOC); + + // Special case for Postgres + if ($conn->getDriver() == 'pgsql') { + $this->results = $this->fetchAllPgsql($statement); + } else { + $this->results = $statement->fetchAll(PDO::FETCH_ASSOC); + } $statement->closeCursor(); } + /** + * Fetch a record form the statement with its type data corrected + * Necessary to fix float data retrieved from PGSQL + * Returns data as an array of maps + * @return array + */ + protected function fetchAllPgsql($statement) + { + $columnCount = $statement->columnCount(); + $columnMeta = []; + for ($i = 0; $i<$columnCount; $i++) { + $columnMeta[$i] = $statement->getColumnMeta($i); + } + + // Re-map fetched data using columnMeta + return array_map( + function ($rowArray) use ($columnMeta) { + $row = []; + foreach ($columnMeta as $i => $meta) { + // Coerce floats from string to float + // PDO PostgreSQL fails to do this + if (isset($meta['native_type']) && strpos($meta['native_type'], 'float') === 0) { + $rowArray[$i] = (float)$rowArray[$i]; + } + $row[$meta['name']] = $rowArray[$i]; + } + return $row; + }, + $statement->fetchAll(PDO::FETCH_NUM) + ); + } + public function seek($row) { $this->rowNum = $row - 1; diff --git a/tests/php/ORM/DatabaseTest.php b/tests/php/ORM/DatabaseTest.php index b9a6fca31..d05a44fc8 100644 --- a/tests/php/ORM/DatabaseTest.php +++ b/tests/php/ORM/DatabaseTest.php @@ -231,4 +231,38 @@ class DatabaseTest extends SapphireTest $this->assertInstanceOf('Exception', $ex); $this->assertEquals('error', $ex->getMessage()); } + + + public function testFieldTypes() + { + // Scaffold some data + $obj = new MyObject(); + $obj->MyField = "value"; + $obj->MyInt = 5; + $obj->MyFloat = 6.0; + $obj->MyBoolean = true; + $obj->write(); + + $record = DB::prepared_query( + 'SELECT * FROM "DatabaseTest_MyObject" WHERE "ID" = ?', + [ $obj->ID ] + )->record(); + + // IDs and ints are returned as ints + $this->assertInternalType('int', $record['ID']); + $this->assertInternalType('int', $record['MyInt']); + + $this->assertInternalType('float', $record['MyFloat']); + + // Booleans are returned as ints – we follow MySQL's lead + $this->assertInternalType('int', $record['MyBoolean']); + + // Strings and enums are returned as strings + $this->assertInternalType('string', $record['MyField']); + $this->assertInternalType('string', $record['ClassName']); + + // Dates are returned as strings + $this->assertInternalType('string', $record['Created']); + $this->assertInternalType('string', $record['LastEdited']); + } } diff --git a/tests/php/ORM/DatabaseTest/MyObject.php b/tests/php/ORM/DatabaseTest/MyObject.php index 83d7fced1..3d09cb1cf 100644 --- a/tests/php/ORM/DatabaseTest/MyObject.php +++ b/tests/php/ORM/DatabaseTest/MyObject.php @@ -13,6 +13,9 @@ class MyObject extends DataObject implements TestOnly private static $create_table_options = array(MySQLSchemaManager::ID => 'ENGINE=InnoDB'); private static $db = array( - 'MyField' => 'Varchar' + 'MyField' => 'Varchar', + 'MyInt' => 'Int', + 'MyFloat' => 'Float', + 'MyBoolean' => 'Boolean', ); }