Skip to content

Commit

Permalink
Removed the OCI8Connection::getExecuteMode() method
Browse files Browse the repository at this point in the history
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` (doctrine#3590).
  • Loading branch information
morozov committed Jan 3, 2020
1 parent f37a88e commit 8d7d18a
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 37 deletions.
29 changes: 29 additions & 0 deletions lib/Doctrine/DBAL/Driver/OCI8/ExecutionMode.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Driver\OCI8;

/**
* Encapsulates the execution mode that is shared between the connection and its statements.
*/
final class ExecutionMode
{
/** @var bool */
private $isAutoCommitEnabled = true;

public function enableAutoCommit() : void
{
$this->isAutoCommitEnabled = true;
}

public function disableAutoCommit() : void
{
$this->isAutoCommitEnabled = false;
}

public function isAutoCommitEnabled() : bool
{
return $this->isAutoCommitEnabled;
}
}
25 changes: 8 additions & 17 deletions lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -55,7 +53,8 @@ public function __construct(
throw OCI8Exception::fromErrorInfo(oci_error());
}

$this->dbh = $dbh;
$this->dbh = $dbh;
$this->executionMode = new ExecutionMode();
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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();
}

/**
Expand All @@ -176,7 +167,7 @@ public function commit() : void
throw OCI8Exception::fromErrorInfo(oci_error($this->dbh));
}

$this->executeMode = OCI_COMMIT_ON_SUCCESS;
$this->executionMode->enableAutoCommit();
}

/**
Expand All @@ -188,6 +179,6 @@ public function rollBack() : void
throw OCI8Exception::fromErrorInfo(oci_error($this->dbh));
}

$this->executeMode = OCI_COMMIT_ON_SUCCESS;
$this->executionMode->enableAutoCommit();
}
}
24 changes: 16 additions & 8 deletions lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 = [
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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));
}
Expand Down
33 changes: 33 additions & 0 deletions tests/Doctrine/Tests/DBAL/Driver/OCI8/ExecutionModeTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\DBAL\Driver\OCI8;

use Doctrine\DBAL\Driver\OCI8\ExecutionMode;
use PHPStan\Testing\TestCase;

final class ExecutionModeTest extends TestCase
{
/** @var ExecutionMode */
private $mode;

protected function setUp() : void
{
$this->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());
}
}
12 changes: 0 additions & 12 deletions tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8StatementTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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'));
Expand Down

0 comments on commit 8d7d18a

Please sign in to comment.