From 8d7d18a860ea24ad20198b1bdd786db002b618cf Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Thu, 2 Jan 2020 22:50:30 -0800 Subject: [PATCH] Removed the OCI8Connection::getExecuteMode() method The existing relationship between the connection and its statement violate the ISP principle. Instead of having access only to the execution mode of the connection, statements have access to the entire connection API. Having a method which is not defined in the driver connection interface makes it impossible to mark the method `final` (#3590). --- .../DBAL/Driver/OCI8/ExecutionMode.php | 29 ++++++++++++++++ .../DBAL/Driver/OCI8/OCI8Connection.php | 25 +++++--------- .../DBAL/Driver/OCI8/OCI8Statement.php | 24 +++++++++----- .../DBAL/Driver/OCI8/ExecutionModeTest.php | 33 +++++++++++++++++++ .../DBAL/Driver/OCI8/OCI8StatementTest.php | 12 ------- 5 files changed, 86 insertions(+), 37 deletions(-) create mode 100644 lib/Doctrine/DBAL/Driver/OCI8/ExecutionMode.php create mode 100644 tests/Doctrine/Tests/DBAL/Driver/OCI8/ExecutionModeTest.php diff --git a/lib/Doctrine/DBAL/Driver/OCI8/ExecutionMode.php b/lib/Doctrine/DBAL/Driver/OCI8/ExecutionMode.php new file mode 100644 index 00000000000..aa16cdd35d4 --- /dev/null +++ b/lib/Doctrine/DBAL/Driver/OCI8/ExecutionMode.php @@ -0,0 +1,29 @@ +isAutoCommitEnabled = true; + } + + public function disableAutoCommit() : void + { + $this->isAutoCommitEnabled = false; + } + + public function isAutoCommitEnabled() : bool + { + return $this->isAutoCommitEnabled; + } +} diff --git a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php index c2eac6d92a5..9887201d826 100644 --- a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php +++ b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php @@ -9,9 +9,7 @@ use Doctrine\DBAL\Driver\ServerInfoAwareConnection; use Doctrine\DBAL\Driver\Statement as DriverStatement; use UnexpectedValueException; -use const OCI_COMMIT_ON_SUCCESS; use const OCI_DEFAULT; -use const OCI_NO_AUTO_COMMIT; use function addcslashes; use function oci_commit; use function oci_connect; @@ -31,8 +29,8 @@ class OCI8Connection implements Connection, ServerInfoAwareConnection /** @var resource */ protected $dbh; - /** @var int */ - protected $executeMode = OCI_COMMIT_ON_SUCCESS; + /** @var ExecutionMode */ + private $executionMode; /** * Creates a Connection to an Oracle Database using oci8 extension. @@ -55,7 +53,8 @@ public function __construct( throw OCI8Exception::fromErrorInfo(oci_error()); } - $this->dbh = $dbh; + $this->dbh = $dbh; + $this->executionMode = new ExecutionMode(); } /** @@ -98,7 +97,7 @@ public function requiresQueryForServerVersion() : bool */ public function prepare(string $sql) : DriverStatement { - return new OCI8Statement($this->dbh, $sql, $this); + return new OCI8Statement($this->dbh, $sql, $this->executionMode); } /** @@ -151,20 +150,12 @@ public function lastInsertId(?string $name = null) : string return $result; } - /** - * Returns the current execution mode. - */ - public function getExecuteMode() : int - { - return $this->executeMode; - } - /** * {@inheritdoc} */ public function beginTransaction() : void { - $this->executeMode = OCI_NO_AUTO_COMMIT; + $this->executionMode->disableAutoCommit(); } /** @@ -176,7 +167,7 @@ public function commit() : void throw OCI8Exception::fromErrorInfo(oci_error($this->dbh)); } - $this->executeMode = OCI_COMMIT_ON_SUCCESS; + $this->executionMode->enableAutoCommit(); } /** @@ -188,6 +179,6 @@ public function rollBack() : void throw OCI8Exception::fromErrorInfo(oci_error($this->dbh)); } - $this->executeMode = OCI_COMMIT_ON_SUCCESS; + $this->executionMode->enableAutoCommit(); } } diff --git a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php index 17e464d9c1e..a5b224457c9 100644 --- a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php +++ b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php @@ -15,9 +15,11 @@ use const OCI_B_BIN; use const OCI_B_BLOB; use const OCI_BOTH; +use const OCI_COMMIT_ON_SUCCESS; use const OCI_D_LOB; use const OCI_FETCHSTATEMENT_BY_COLUMN; use const OCI_FETCHSTATEMENT_BY_ROW; +use const OCI_NO_AUTO_COMMIT; use const OCI_NUM; use const OCI_RETURN_LOBS; use const OCI_RETURN_NULLS; @@ -57,8 +59,8 @@ class OCI8Statement implements IteratorAggregate, Statement /** @var resource */ protected $_sth; - /** @var OCI8Connection */ - protected $_conn; + /** @var ExecutionMode */ + protected $executionMode; /** @var int[] */ protected static $fetchModeMap = [ @@ -96,17 +98,17 @@ class OCI8Statement implements IteratorAggregate, Statement * @param resource $dbh The connection handle. * @param string $query The SQL query. */ - public function __construct($dbh, string $query, OCI8Connection $conn) + public function __construct($dbh, string $query, ExecutionMode $executionMode) { [$query, $paramMap] = self::convertPositionalToNamedPlaceholders($query); $stmt = oci_parse($dbh, $query); assert(is_resource($stmt)); - $this->_sth = $stmt; - $this->_dbh = $dbh; - $this->_paramMap = $paramMap; - $this->_conn = $conn; + $this->_sth = $stmt; + $this->_dbh = $dbh; + $this->_paramMap = $paramMap; + $this->executionMode = $executionMode; } /** @@ -364,7 +366,13 @@ public function execute(?array $params = null) : void } } - $ret = @oci_execute($this->_sth, $this->_conn->getExecuteMode()); + if ($this->executionMode->isAutoCommitEnabled()) { + $mode = OCI_COMMIT_ON_SUCCESS; + } else { + $mode = OCI_NO_AUTO_COMMIT; + } + + $ret = @oci_execute($this->_sth, $mode); if (! $ret) { throw OCI8Exception::fromErrorInfo(oci_error($this->_sth)); } diff --git a/tests/Doctrine/Tests/DBAL/Driver/OCI8/ExecutionModeTest.php b/tests/Doctrine/Tests/DBAL/Driver/OCI8/ExecutionModeTest.php new file mode 100644 index 00000000000..4f7ad26d00c --- /dev/null +++ b/tests/Doctrine/Tests/DBAL/Driver/OCI8/ExecutionModeTest.php @@ -0,0 +1,33 @@ +mode = new ExecutionMode(); + } + + public function testDefaultAutoCommitStatus() : void + { + self::assertTrue($this->mode->isAutoCommitEnabled()); + } + + public function testChangeAutoCommitStatus() : void + { + $this->mode->disableAutoCommit(); + self::assertFalse($this->mode->isAutoCommitEnabled()); + + $this->mode->enableAutoCommit(); + self::assertTrue($this->mode->isAutoCommitEnabled()); + } +} diff --git a/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8StatementTest.php b/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8StatementTest.php index 846122b40db..619e2b41962 100644 --- a/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8StatementTest.php +++ b/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8StatementTest.php @@ -10,7 +10,6 @@ use Doctrine\Tests\DbalTestCase; use PHPUnit\Framework\MockObject\MockObject; use ReflectionProperty; -use const OCI_NO_AUTO_COMMIT; use function extension_loaded; use function fopen; @@ -55,17 +54,6 @@ public function testExecute(array $params) : void ); } - // can't pass to constructor since we don't have a real database handle, - // but execute must check the connection for the executeMode - $conn = $this->createMock(OCI8Connection::class); - $conn->expects($this->once()) - ->method('getExecuteMode') - ->willReturn(OCI_NO_AUTO_COMMIT); - - $connectionReflection = new ReflectionProperty($statement, '_conn'); - $connectionReflection->setAccessible(true); - $connectionReflection->setValue($statement, $conn); - $handleReflection = new ReflectionProperty($statement, '_sth'); $handleReflection->setAccessible(true); $handleReflection->setValue($statement, fopen('php://temp', 'r'));