Merge pull request #8448 from sminnee/int-types

FIX: Ensure that types are preserved fetching from database

.travis.yml

@@ -20,20 +20,20 @@ matrix:
   include:
     - php: 5.6
       env:
-        - DB=MYSQL
+        - DB=PGSQL
         - PHPCS_TEST=1
         - PHPUNIT_TEST=framework
 
     - php: 7.0
       env:
         - DB=PGSQL
+        - PDO=1
         - PHPUNIT_TEST=framework
 
     - php: 7.1
       if: type IN (cron)
       env:
         - DB=MYSQL
-        - PDO=1
         - PHPUNIT_COVERAGE_TEST=framework
 
     - php: 7.2
@@ -50,7 +50,6 @@ matrix:
     - php: 7.3.0RC1
       env:
         - DB=MYSQL
-        - PDO=1
         - PHPUNIT_TEST=framework
       sudo: required
       dist: xenial
@@ -74,7 +73,7 @@ before_script:
 # Install composer dependencies
   - composer validate
   - mkdir ./public
-  - if [[ $DB == PGSQL ]]; then composer require silverstripe/postgresql:2.1.x-dev --no-update; fi
+  - if [[ $DB == PGSQL ]]; then composer require silverstripe/postgresql:2.2.x-dev --no-update; fi
   - if [[ $DB == SQLITE ]]; then composer require silverstripe/sqlite3:2.0.x-dev --no-update; fi
   - composer require silverstripe/recipe-testing:^1 silverstripe/recipe-core:4.4.x-dev silverstripe/admin:1.4.x-dev silverstripe/versioned:1.4.x-dev --no-update
   - if [[ $PHPUNIT_TEST == cms ]]; then composer require silverstripe/recipe-cms:4.4.x-dev --no-update; fi

docs/en/02_Developer_Guides/00_Model/08_SQL_Select.md

@@ -291,6 +291,18 @@ $players = Player::get();
 $map = $players->map('Name', 'NameWithBirthyear');
 ```
 
+### Data types
+
+As of SilverStripe 4.4, the following PHP types will be used to return datbase content:
+
+ * booleans will be an integer 1 or 0, to ensure consistency with MySQL that doesn't have native booleans.
+ * integer types returned as integers
+ * floating point / decimal types returned as floats
+ * strings returned as strings
+ * dates / datetimes returned as strings
+
+Up until SilverStripe 4.3, bugs meant that strings were used for every column type.
+
 ## Related Lessons
 * [Building custom SQL](https://www.silverstripe.org/learn/lessons/v4/beyond-the-orm-building-custom-sql-1)
 

docs/en/04_Changelogs/4.4.0.md

@@ -0,0 +1,14 @@
+# 4.4.0
+
+## Overview {#overview}
+
+ - [Correct PHP types are now returned from database queries](/developer_guides/model/sql_select#data-types)
+ 
+## Upgrading {#upgrading}
+
+tbc
+
+## Changes to internal APIs
+
+ - `PDOQuery::__construct()` now has a 2nd argument. If you have subclassed PDOQuery and overridden __construct() 
+   you may see an E_STRICT error

src/ORM/Connect/Database.php

@@ -583,6 +583,17 @@ abstract class Database
      */
     abstract public function supportsTransactions();
 
+    /**
+     * Does this database support savepoints in transactions
+     * By default it is assumed that they don't unless they are explicitly enabled.
+     *
+     * @return boolean Flag indicating support for savepoints in transactions
+     */
+    public function supportsSavepoints()
+    {
+        return false;
+    }
+
     /**
      * Invoke $callback within a transaction
      *

src/ORM/Connect/MySQLDatabase.php

@@ -21,7 +21,7 @@ use Exception;
  * You are advised to backup your tables if changing settings on an existing database
  * `connection_charset` and `charset` should be equal, similarly so should `connection_collation` and `collation`
  */
-class MySQLDatabase extends Database
+class MySQLDatabase extends Database implements TransactionManager
 {
     use Configurable;
 
@@ -49,6 +49,13 @@ class MySQLDatabase extends Database
      */
     private static $charset = 'utf8';
 
+    /**
+     * Cache for getTransactionManager()
+     *
+     * @var TransactionManager
+     */
+    private $transactionManager = null;
+
     /**
      * Default collation
      *
@@ -57,11 +64,6 @@ class MySQLDatabase extends Database
      */
     private static $collation = 'utf8_general_ci';
 
-    /**
-     * @var bool
-     */
-    protected $transactionNesting = 0;
-
     public function connect($parameters)
     {
         // Ensure that driver is available (required by PDO)
@@ -298,73 +300,64 @@ class MySQLDatabase extends Database
         return $list;
     }
 
+
+    /**
+     * Returns the TransactionManager to handle transactions for this database.
+     *
+     * @return TransactionManager
+     */
+    protected function getTransactionManager()
+    {
+        if (!$this->transactionManager) {
+            // PDOConnector providers this
+            if ($this->connector instanceof TransactionManager) {
+                $this->transactionManager = new NestedTransactionManager($this->connector);
+            // Direct database access does not
+            } else {
+                $this->transactionManager = new NestedTransactionManager(new MySQLTransactionManager($this));
+            }
+        }
+        return $this->transactionManager;
+    }
     public function supportsTransactions()
     {
         return true;
     }
+    public function supportsSavepoints()
+    {
+        return $this->getTransactionManager()->supportsSavepoints();
+    }
 
     public function transactionStart($transactionMode = false, $sessionCharacteristics = false)
     {
-        if ($this->transactionNesting > 0) {
+        $this->getTransactionManager()->transactionStart($transactionMode, $sessionCharacteristics);
-            $this->transactionSavepoint('NESTEDTRANSACTION' . $this->transactionNesting);
-        } else {
-            // This sets the isolation level for the NEXT transaction, not the current one.
-            if ($transactionMode) {
-                $this->query('SET TRANSACTION ' . $transactionMode);
-            }
-
-            $this->query('START TRANSACTION');
-
-            if ($sessionCharacteristics) {
-                $this->query('SET SESSION TRANSACTION ' . $sessionCharacteristics);
-            }
-        }
-        ++$this->transactionNesting;
     }
 
     public function transactionSavepoint($savepoint)
     {
-        $this->query("SAVEPOINT $savepoint");
+        $this->getTransactionManager()->transactionSavepoint($savepoint);
     }
 
     public function transactionRollback($savepoint = false)
     {
-        // Named transaction
+        return $this->getTransactionManager()->transactionRollback($savepoint);
-        if ($savepoint) {
-            $this->query('ROLLBACK TO ' . $savepoint);
-            return true;
-        }
-
-        // Fail if transaction isn't available
-        if (!$this->transactionNesting) {
-            return false;
-        }
-        --$this->transactionNesting;
-        if ($this->transactionNesting > 0) {
-            $this->transactionRollback('NESTEDTRANSACTION' . $this->transactionNesting);
-        } else {
-            $this->query('ROLLBACK');
-        }
-        return true;
     }
 
     public function transactionDepth()
     {
-        return $this->transactionNesting;
+        return $this->getTransactionManager()->transactionDepth();
     }
 
     public function transactionEnd($chain = false)
     {
-        // Fail if transaction isn't available
+        $result = $this->getTransactionManager()->transactionEnd();
-        if (!$this->transactionNesting) {
+
-            return false;
+        if ($chain) {
+            Deprecation::notice('4.4', '$chain argument is deprecated');
+            return $this->getTransactionManager()->transactionStart();
         }
-        --$this->transactionNesting;
+
-        if ($this->transactionNesting <= 0) {
+        return $result;
-            $this->transactionNesting = 0;
-            $this->query('COMMIT AND ' . ($chain ? '' : 'NO ') . 'CHAIN');
-        }
-        return true;
     }
 
     /**
@@ -372,6 +365,12 @@ class MySQLDatabase extends Database
      */
     protected function resetTransactionNesting()
     {
+        // Check whether to use a connector's built-in transaction methods
+        if ($this->connector instanceof TransactionalDBConnector) {
+            if ($this->transactionNesting > 0) {
+                $this->connector->transactionRollback();
+            }
+        }
         $this->transactionNesting = 0;
     }
 

src/ORM/Connect/MySQLTransactionManager.php

@@ -0,0 +1,100 @@
+<?php
+
+namespace SilverStripe\ORM\Connect;
+
+use SilverStripe\Dev\Deprecation;
+
+/**
+ * TransactionManager that executes MySQL-compatible transaction control queries
+ */
+class MySQLTransactionManager implements TransactionManager
+{
+    protected $dbConn;
+
+    protected $inTransaction = false;
+
+    public function __construct(Database $dbConn)
+    {
+        $this->dbConn = $dbConn;
+    }
+
+    public function transactionStart($transactionMode = false, $sessionCharacteristics = false)
+    {
+        if ($transactionMode || $sessionCharacteristics) {
+            Deprecation::notice(
+                '4.4',
+                '$transactionMode and $sessionCharacteristics are deprecated and will be removed in SS5'
+            );
+        }
+
+        if ($this->inTransaction) {
+            throw new DatabaseException(
+                "Already in transaction, can't start another. Consider decorating with NestedTransactionManager."
+            );
+        }
+
+        // This sets the isolation level for the NEXT transaction, not the current one.
+        if ($transactionMode) {
+            $this->dbConn->query('SET TRANSACTION ' . $transactionMode);
+        }
+
+        $this->dbConn->query('START TRANSACTION');
+
+        if ($sessionCharacteristics) {
+            $this->dbConn->query('SET SESSION TRANSACTION ' . $sessionCharacteristics);
+        }
+
+        $this->inTransaction = true;
+        return true;
+    }
+
+    public function transactionEnd($chain = false)
+    {
+        if (!$this->inTransaction) {
+            throw new DatabaseException("Not in transaction, can't end.");
+        }
+
+        if ($chain) {
+            user_error(
+                "transactionEnd() chain argument no longer implemented. Use NestedTransactionManager",
+                E_USER_WARNING
+            );
+        }
+
+        $this->dbConn->query('COMMIT');
+
+        $this->inTransaction = false;
+        return true;
+    }
+
+    public function transactionRollback($savepoint = null)
+    {
+        if (!$this->inTransaction) {
+            throw new DatabaseException("Not in transaction, can't roll back.");
+        }
+
+        if ($savepoint) {
+            $this->dbConn->query("ROLLBACK TO SAVEPOINT $savepoint");
+        } else {
+            $this->dbConn->query('ROLLBACK');
+            $this->inTransaction = false;
+        }
+
+        return true;
+    }
+
+    public function transactionSavepoint($savepoint)
+    {
+        $this->dbConn->query("SAVEPOINT $savepoint");
+    }
+
+    public function transactionDepth()
+    {
+        return (int)$this->inTransaction;
+    }
+
+    public function supportsSavepoints()
+    {
+        return true;
+    }
+}

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) &&

src/ORM/Connect/NestedTransactionManager.php

@@ -0,0 +1,127 @@
+<?php
+
+namespace SilverStripe\ORM\Connect;
+
+/**
+ * TransactionManager decorator that adds virtual nesting support.
+ * Because this is managed in PHP and not the database, it has the following limitations:
+ *   - Committing a nested transaction won't change anything until the parent transaction is committed
+ *   - Rolling back a nested transaction means that the parent transaction must be rolled backed
+ *
+ * DBAL describes this behaviour nicely in their docs: https://www.doctrine-project.org/projects/doctrine-dbal/en/2.8/reference/transactions.html#transaction-nesting
+ */
+
+class NestedTransactionManager implements TransactionManager
+{
+
+    /**
+     * @var int
+     */
+    protected $transactionNesting = 0;
+
+    /**
+     * @var TransactionManager
+     */
+    protected $child;
+
+    /**
+     * Set to true if all transactions must roll back to the parent
+     * @var boolean
+     */
+    protected $mustRollback = false;
+
+    /**
+     * Create a NestedTransactionManager
+     * @param TransactionManager $child The transaction manager that will handle the topmost transaction
+     */
+    public function __construct(TransactionManager $child)
+    {
+        $this->child = $child;
+    }
+
+    /**
+     * Start a transaction
+     * @throws DatabaseException on failure
+     * @return bool True on success
+     */
+    public function transactionStart($transactionMode = false, $sessionCharacteristics = false)
+    {
+        if ($this->transactionNesting <= 0) {
+            $this->transactionNesting = 1;
+            $this->child->transactionStart($transactionMode, $sessionCharacteristics);
+        } else {
+            if ($this->child->supportsSavepoints()) {
+                $this->child->transactionSavepoint("nesting" . $this->transactionNesting);
+            }
+            $this->transactionNesting++;
+        }
+    }
+
+    public function transactionEnd($chain = false)
+    {
+        if ($this->mustRollback) {
+            throw new DatabaseException("Child transaction was rolled back, so parent can't be committed");
+        }
+
+        if ($this->transactionNesting < 1) {
+            throw new DatabaseException("Not within a transaction, so can't commit");
+        }
+
+        $this->transactionNesting--;
+
+        if ($this->transactionNesting === 0) {
+            $this->child->transactionEnd();
+        }
+
+        if ($chain) {
+            return $this->transactionStart();
+        }
+    }
+
+    public function transactionRollback($savepoint = null)
+    {
+        if ($this->transactionNesting < 1) {
+            throw new DatabaseException("Not within a transaction, so can't roll back");
+        }
+
+        if ($savepoint) {
+            return $this->child->transactionRollback($savepoint);
+        }
+
+        $this->transactionNesting--;
+
+        if ($this->transactionNesting === 0) {
+            $this->child->transactionRollback();
+            $this->mustRollback = false;
+        } else {
+            if ($this->child->supportsSavepoints()) {
+                $this->child->transactionRollback("nesting" . $this->transactionNesting);
+                $this->mustRollback = false;
+
+            // Without savepoints, parent transactions must roll back if a child one has
+            } else {
+                $this->mustRollback = true;
+            }
+        }
+    }
+
+    /**
+     * Return the depth of the transaction.
+     *
+     * @return int
+     */
+    public function transactionDepth()
+    {
+        return $this->transactionNesting;
+    }
+
+    public function transactionSavepoint($savepoint)
+    {
+        return $this->child->transactionSavepoint($savepoint);
+    }
+
+    public function supportsSavepoints()
+    {
+        return $this->child->supportsSavepoints();
+    }
+}

src/ORM/Connect/PDOConnector.php

@@ -10,7 +10,7 @@ use InvalidArgumentException;
 /**
  * PDO driver database connector
  */
-class PDOConnector extends DBConnector
+class PDOConnector extends DBConnector implements TransactionManager
 {
 
     /**
@@ -21,6 +21,15 @@ class PDOConnector extends DBConnector
      */
     private static $emulate_prepare = false;
 
+    /**
+     * Should we return everything as a string in order to allow transaction savepoints?
+     * This preserves the behaviour of <= 4.3, including some bugs.
+     *
+     * @config
+     * @var boolean
+     */
+    private static $legacy_types = false;
+
     /**
      * Default strong SSL cipher to be used
      *
@@ -64,6 +73,18 @@ class PDOConnector extends DBConnector
      */
     protected $cachedStatements = array();
 
+    /**
+     * Driver
+     * @var string
+     */
+    protected $driver = null;
+
+    /*
+     * Is a transaction currently active?
+     * @var bool
+     */
+    protected $inTransaction = false;
+
     /**
      * Flush all prepared statements
      */
@@ -113,10 +134,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 +217,23 @@ 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()) {
+        if (static::config()->get('legacy_types')) {
+            $options[PDO::ATTR_STRINGIFY_FETCHES] = true;
             $options[PDO::ATTR_EMULATE_PREPARES] = true;
+        } else {
+            // 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 +245,18 @@ class PDOConnector extends DBConnector
         }
     }
 
+
+    /**
+     * Return the driver for this connector
+     * E.g. 'mysql', 'sqlsrv', 'pgsql'
+     *
+     * @return string
+     */
+    public function getDriver()
+    {
+        return $this->driver;
+    }
+
     public function getVersion()
     {
         return $this->pdoConnection->getAttribute(PDO::ATTR_SERVER_VERSION);
@@ -383,7 +427,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
@@ -468,4 +512,60 @@ class PDOConnector extends DBConnector
     {
         return $this->databaseName && $this->pdoConnection;
     }
+
+    public function transactionStart($transactionMode = false, $sessionCharacteristics = false)
+    {
+        $this->inTransaction = true;
+
+        if ($transactionMode) {
+            $this->query("SET TRANSACTION $transactionMode");
+        }
+
+        if ($this->pdoConnection->beginTransaction()) {
+            if ($sessionCharacteristics) {
+                $this->query("SET SESSION CHARACTERISTICS AS TRANSACTION $sessionCharacteristics");
+            }
+            return true;
+        }
+        return false;
+    }
+
+    public function transactionEnd()
+    {
+        $this->inTransaction = false;
+        return $this->pdoConnection->commit();
+    }
+
+    public function transactionRollback($savepoint = null)
+    {
+        if ($savepoint) {
+            if ($this->supportsSavepoints()) {
+                $this->exec("ROLLBACK TO SAVEPOINT $savepoint");
+            } else {
+                throw new DatabaseException("Savepoints not supported on this PDO connection");
+            }
+        }
+
+        $this->inTransaction = false;
+        return $this->pdoConnection->rollBack();
+    }
+
+    public function transactionDepth()
+    {
+        return (int)$this->inTransaction;
+    }
+
+    public function transactionSavepoint($savepoint = null)
+    {
+        if ($this->supportsSavepoints()) {
+            $this->exec("SAVEPOINT $savepoint");
+        } else {
+            throw new DatabaseException("Savepoints not supported on this PDO connection");
+        }
+    }
+
+    public function supportsSavepoints()
+    {
+        return static::config()->get('legacy_types');
+    }
 }

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;

src/ORM/Connect/Query.php

@@ -6,12 +6,24 @@ use SilverStripe\Core\Convert;
 use Iterator;
 
 /**
- * Abstract query-result class.
+ * Abstract query-result class. A query result provides an iterator that returns a map for each record of a query
+ * result.
+ *
+ * The map should be keyed by the column names, and the values should use the following types:
+ *
+ *  - boolean returned as integer 1 or 0 (to ensure consistency with MySQL that doesn't have native booleans)
+ *  - integer types returned as integers
+ *  - floating point / decimal types returned as floats
+ *  - strings returned as strings
+ *  - dates / datetimes returned as strings
+ *
+ * Note that until SilverStripe 4.3, bugs meant that strings were used for every column type.
+ *
  * Once again, this should be subclassed by an actual database implementation.  It will only
  * ever be constructed by a subclass of SS_Database.  The result of a database query - an iteratable object
  * that's returned by DB::SS_Query
  *
- * Primarily, the SS_Query class takes care of the iterator plumbing, letting the subclasses focusing
+ * Primarily, the Query class takes care of the iterator plumbing, letting the subclasses focusing
  * on providing the specific data-access methods that are required: {@link nextRecord()}, {@link numRecords()}
  * and {@link seek()}
  */

src/ORM/Connect/TransactionManager.php

@@ -0,0 +1,67 @@
+<?php
+
+namespace SilverStripe\ORM\Connect;
+
+/**
+ * Represents an object that is capable of controlling transactions.
+ *
+ * The TransactionManager might be the database connection itself, calling queries to orchestrate
+ * transactions, or a connector such as the PDOConnector.
+ *
+ * Generally speaking you should rely on your Database object to manage the creation of a TansactionManager
+ * for you; unless you are building new database connectors this should be treated as an internal API.
+ */
+interface TransactionManager
+{
+    /**
+     * Start a prepared transaction
+     *
+     * @param string|boolean $transactionMode Transaction mode, or false to ignore. Deprecated and will be removed in SS5.
+     * @param string|boolean $sessionCharacteristics Session characteristics, or false to ignore. Deprecated and will be removed in SS5.
+     * @throws DatabaseException on failure
+     * @return bool True on success
+     */
+    public function transactionStart($transactionMode = false, $sessionCharacteristics = false);
+
+    /**
+     * Complete a transaction
+     *
+     * @throws DatabaseException on failure
+     * @return bool True on success
+     */
+    public function transactionEnd();
+
+    /**
+     * Roll-back a transaction
+     *
+     * @param string $savepoint If set, roll-back to the named savepoint
+     * @throws DatabaseException on failure
+     * @return bool True on success
+     */
+    public function transactionRollback($savepoint = null);
+
+    /**
+     * Create a new savepoint
+     *
+     * @param string $savepoint The savepoint name
+     * @throws DatabaseException on failure
+     */
+    public function transactionSavepoint($savepoint);
+
+    /**
+     * Return the depth of the transaction
+     * For unnested transactions returns 1 while in a transaction, 0 otherwise
+     *
+     * @return int
+     */
+    public function transactionDepth();
+
+    /**
+     * Return true if savepoints are supported by this transaction manager.
+     * Savepoints aren't supported by all database connectors (notably PDO doesn't support them)
+     * and should be used with caution.
+     *
+     * @return boolean
+     */
+    public function supportsSavepoints();
+}

tests/php/ORM/DatabaseTest.php

@@ -187,48 +187,36 @@ class DatabaseTest extends SapphireTest
         $this->assertTrue($db->canLock('DatabaseTest'), 'Can lock again after releasing it');
     }
 
-    public function testTransactions()
+    public function testFieldTypes()
     {
-        $conn = DB::get_conn();
+        // Scaffold some data
-        if (!$conn->supportsTransactions()) {
+        $obj = new MyObject();
-            $this->markTestSkipped("DB Doesn't support transactions");
+        $obj->MyField = "value";
-            return;
+        $obj->MyInt = 5;
-        }
+        $obj->MyFloat = 6.0;
+        $obj->MyBoolean = true;
+        $obj->write();
 
-        // Test that successful transactions are comitted
+        $record = DB::prepared_query(
-        $obj = new DatabaseTest\MyObject();
+            'SELECT * FROM "DatabaseTest_MyObject" WHERE "ID" = ?',
-        $failed = false;
+            [ $obj->ID ]
-        $conn->withTransaction(
+        )->record();
-            function () use (&$obj) {
-                $obj->MyField = 'Save 1';
-                $obj->write();
-            },
-            function () use (&$failed) {
-                $failed = true;
-            }
-        );
-        $this->assertEquals('Save 1', DatabaseTest\MyObject::get()->first()->MyField);
-        $this->assertFalse($failed);
 
-        // Test failed transactions are rolled back
+        // IDs and ints are returned as ints
-        $ex = null;
+        $this->assertInternalType('int', $record['ID']);
-        $failed = false;
+        $this->assertInternalType('int', $record['MyInt']);
-        try {
+
-            $conn->withTransaction(
+        $this->assertInternalType('float', $record['MyFloat']);
-                function () use (&$obj) {
+
-                    $obj->MyField = 'Save 2';
+        // Booleans are returned as ints – we follow MySQL's lead
-                    $obj->write();
+        $this->assertInternalType('int', $record['MyBoolean']);
-                    throw new Exception("error");
+
-                },
+        // Strings and enums are returned as strings
-                function () use (&$failed) {
+        $this->assertInternalType('string', $record['MyField']);
-                    $failed = true;
+        $this->assertInternalType('string', $record['ClassName']);
-                }
+
-            );
+        // Dates are returned as strings
-        } catch (Exception $ex) {
+        $this->assertInternalType('string', $record['Created']);
-        }
+        $this->assertInternalType('string', $record['LastEdited']);
-        $this->assertTrue($failed);
-        $this->assertEquals('Save 1', DatabaseTest\MyObject::get()->first()->MyField);
-        $this->assertInstanceOf('Exception', $ex);
-        $this->assertEquals('error', $ex->getMessage());
     }
 }

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',
     );
 }

tests/php/ORM/TransactionTest.php

@@ -5,16 +5,33 @@ namespace SilverStripe\ORM\Tests;
 use SilverStripe\ORM\DB;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Dev\Deprecation;
 use SilverStripe\ORM\Tests\TransactionTest\TestObject;
 
 class TransactionTest extends SapphireTest
 {
     protected $usesDatabase = true;
 
+    protected $usesTransactions = false;
+
     protected static $extra_dataobjects = [
         TransactionTest\TestObject::class,
     ];
 
+    private static $originalVersionInfo;
+
+    protected function setUp()
+    {
+        parent::setUp();
+        self::$originalVersionInfo = Deprecation::dump_settings();
+    }
+
+    protected function tearDown()
+    {
+        Deprecation::restore_settings(self::$originalVersionInfo);
+        parent::tearDown();
+    }
+
     public static function setUpBeforeClass()
     {
         parent::setUpBeforeClass();
@@ -23,8 +40,57 @@ class TransactionTest extends SapphireTest
         }
     }
 
+    public function testTransactions()
+    {
+        $conn = DB::get_conn();
+        if (!$conn->supportsTransactions()) {
+            $this->markTestSkipped("DB Doesn't support transactions");
+            return;
+        }
+
+        // Test that successful transactions are comitted
+        $obj = new TestObject();
+        $failed = false;
+        $conn->withTransaction(
+            function () use (&$obj) {
+                $obj->Title = 'Save 1';
+                $obj->write();
+            },
+            function () use (&$failed) {
+                $failed = true;
+            }
+        );
+        $this->assertEquals('Save 1', TestObject::get()->first()->Title);
+        $this->assertFalse($failed);
+
+        // Test failed transactions are rolled back
+        $ex = null;
+        $failed = false;
+        try {
+            $conn->withTransaction(
+                function () use (&$obj) {
+                    $obj->Title = 'Save 2';
+                    $obj->write();
+                    throw new \Exception("error");
+                },
+                function () use (&$failed) {
+                    $failed = true;
+                }
+            );
+        } catch (\Exception $ex) {
+        }
+        $this->assertTrue($failed);
+        $this->assertEquals('Save 1', TestObject::get()->first()->Title);
+        $this->assertInstanceOf('Exception', $ex);
+        $this->assertEquals('error', $ex->getMessage());
+    }
+
     public function testNestedTransaction()
     {
+        if (!DB::get_conn()->supportsSavepoints()) {
+            static::markTestSkipped('Current database does not support savepoints');
+        }
+
         $this->assertCount(0, TestObject::get());
         try {
             DB::get_conn()->withTransaction(function () {
@@ -51,6 +117,7 @@ class TransactionTest extends SapphireTest
 
     public function testCreateWithTransaction()
     {
+        // First/Second in a successful transaction
         DB::get_conn()->transactionStart();
         $obj = new TransactionTest\TestObject();
         $obj->Title = 'First page';
@@ -59,10 +126,10 @@ class TransactionTest extends SapphireTest
         $obj = new TransactionTest\TestObject();
         $obj->Title = 'Second page';
         $obj->write();
+        DB::get_conn()->transactionEnd();
 
-        //Create a savepoint here:
+        // Third/Fourth in a rolled back transaction
-        DB::get_conn()->transactionSavepoint('rollback');
+        DB::get_conn()->transactionStart();
-
         $obj = new TransactionTest\TestObject();
         $obj->Title = 'Third page';
         $obj->write();
@@ -70,11 +137,8 @@ class TransactionTest extends SapphireTest
         $obj = new TransactionTest\TestObject();
         $obj->Title = 'Fourth page';
         $obj->write();
+        DB::get_conn()->transactionRollback();
 
-        //Revert to a savepoint:
-        DB::get_conn()->transactionRollback('rollback');
-
-        DB::get_conn()->transactionEnd();
 
         $first = DataObject::get(TransactionTest\TestObject::class, "\"Title\"='First page'");
         $second = DataObject::get(TransactionTest\TestObject::class, "\"Title\"='Second page'");
@@ -85,8 +149,48 @@ class TransactionTest extends SapphireTest
         $this->assertTrue(is_object($first) && $first->exists());
         $this->assertTrue(is_object($second) && $second->exists());
 
-        //These pages should NOT exist, we reverted to a savepoint:
+        //These pages should NOT exist, we rolled back
         $this->assertFalse(is_object($third) && $third->exists());
         $this->assertFalse(is_object($fourth) && $fourth->exists());
     }
+
+    public function testReadOnlyTransaction()
+    {
+        if (!DB::get_conn()->supportsTransactions()) {
+            $this->markTestSkipped('Current database is doesn\'t support transactions');
+            return;
+        }
+
+        // This feature is deprecated in 4.4, but we're still testing it.
+        Deprecation::notification_version('4.3.0');
+
+        $page = new TestObject();
+        $page->Title = 'Read only success';
+        $page->write();
+
+        DB::get_conn()->transactionStart('READ ONLY');
+
+        try {
+            $page = new TestObject();
+            $page->Title = 'Read only page failed';
+            $page->write();
+            DB::get_conn()->transactionEnd();
+        } catch (\Exception $e) {
+            //could not write this record
+            //We need to do a rollback or a commit otherwise we'll get error messages
+            DB::get_conn()->transactionRollback();
+        }
+
+        DataObject::flush_and_destroy_cache();
+
+        $success = DataObject::get_one(TestObject::class, "\"Title\"='Read only success'");
+        $fail = DataObject::get_one(TestObject::class, "\"Title\"='Read only page failed'");
+
+        //This page should be in the system
+        $this->assertInternalType('object', $success);
+        $this->assertTrue($success->exists());
+
+        //This page should NOT exist, we had 'read only' permissions
+        $this->assertNull($fail);
+    }
 }