From 99d5e595b8ed537a37c8108bf38533fb4d345cb2 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 19 Feb 2024 00:08:57 -0500 Subject: [PATCH 01/10] Start refactoring adapters to use Connection Instead of drilling into PDO, Migrations adapters should use the Database\Connection API instead. This creates a few behavioral differences from before. I hope that this level of abstraction wasn't commonly used by userland code. --- src/Db/Adapter/PdoAdapter.php | 108 ++++------ src/Db/Adapter/SqliteAdapter.php | 80 +------- src/Migration/Environment.php | 4 +- .../TestCase/Db/Adapter/SqliteAdapterTest.php | 189 +++++++++--------- 4 files changed, 150 insertions(+), 231 deletions(-) diff --git a/src/Db/Adapter/PdoAdapter.php b/src/Db/Adapter/PdoAdapter.php index 539eb720..17cf2a91 100644 --- a/src/Db/Adapter/PdoAdapter.php +++ b/src/Db/Adapter/PdoAdapter.php @@ -39,7 +39,7 @@ use PDOException; use Phinx\Config\Config; use Phinx\Migration\MigrationInterface; -use ReflectionProperty; +use ReflectionMethod; use RuntimeException; use Symfony\Component\Console\Output\OutputInterface; use UnexpectedValueException; @@ -49,15 +49,10 @@ */ abstract class PdoAdapter extends AbstractAdapter implements DirectActionInterface { - /** - * @var \PDO|null - */ - protected ?PDO $connection = null; - /** * @var \Cake\Database\Connection|null */ - protected ?Connection $decoratedConnection = null; + protected ?Connection $connection = null; /** * Writes a message to stdout if verbose output is on @@ -121,18 +116,9 @@ public function setOptions(array $options): AdapterInterface { parent::setOptions($options); - if (isset($options['connection'])) { - // TODO Change migrations adapters to use the Cake - // Database API instead of PDO. - $driver = $options['connection']->getDriver(); - - // TODO this is gross and needs to be replaced - $reflect = new ReflectionProperty($driver, 'pdo'); - $reflect->setAccessible(true); - $pdo = $reflect->getValue($driver); - - assert($pdo instanceof PDO, 'Need a PDO connection'); - $this->setConnection($pdo); + // TODO: Consider renaming this class to ConnectionAdapter + if (isset($options['connection']) && $options['connection'] instanceof Connection) { + $this->setConnection($options['connection']); } return $this; @@ -141,11 +127,13 @@ public function setOptions(array $options): AdapterInterface /** * Sets the database connection. * - * @param \PDO $connection Connection + * @param \Cake\Database\Connection $connection Connection * @return \Migrations\Db\Adapter\AdapterInterface */ - public function setConnection(PDO $connection): AdapterInterface + public function setConnection(Connection $connection): AdapterInterface { + // TODO how do PDO connection flags get set? Phinx used to + // turn on exception error mode, and I don't think Cake does that by default. $this->connection = $connection; // Create the schema table if it doesn't already exist @@ -175,15 +163,16 @@ public function setConnection(PDO $connection): AdapterInterface /** * Gets the database connection * - * @return \PDO + * @return \Cake\Database\Connection */ - public function getConnection(): PDO + public function getConnection(): Connection { if ($this->connection === null) { + $this->connection = $this->getOption('connection'); $this->connect(); } - /** @var \PDO $this->connection */ + /** @var \Cake\Database\Connection $this->connection */ return $this->connection; } @@ -209,26 +198,17 @@ public function execute(string $sql, array $params = []): int return 0; } + $connection = $this->getConnection(); if (empty($params)) { - $result = $this->getConnection()->exec($sql); + $result = $connection->execute($sql); - return is_int($result) ? $result : 0; + return $result->rowCount(); } + $stmt = $connection->execute($sql, $params); - $stmt = $this->getConnection()->prepare($sql); - $result = $stmt->execute($params); - - return $result ? $stmt->rowCount() : 0; + return $stmt->rowCount(); } - /** - * Returns the Cake\Database connection object using the same underlying - * PDO object as this connection. - * - * @return \Cake\Database\Connection - */ - abstract public function getDecoratedConnection(): Connection; - /** * Build connection instance. * @@ -251,10 +231,10 @@ protected function buildConnection(string $driverClass, array $options): Connect public function getQueryBuilder(string $type): Query { return match ($type) { - Query::TYPE_SELECT => $this->getDecoratedConnection()->selectQuery(), - Query::TYPE_INSERT => $this->getDecoratedConnection()->insertQuery(), - Query::TYPE_UPDATE => $this->getDecoratedConnection()->updateQuery(), - Query::TYPE_DELETE => $this->getDecoratedConnection()->deleteQuery(), + Query::TYPE_SELECT => $this->getConnection()->selectQuery(), + Query::TYPE_INSERT => $this->getConnection()->insertQuery(), + Query::TYPE_UPDATE => $this->getConnection()->updateQuery(), + Query::TYPE_DELETE => $this->getConnection()->deleteQuery(), default => throw new InvalidArgumentException( 'Query type must be one of: `select`, `insert`, `update`, `delete`.' ) @@ -266,7 +246,7 @@ public function getQueryBuilder(string $type): Query */ public function getSelectBuilder(): SelectQuery { - return $this->getDecoratedConnection()->selectQuery(); + return $this->getConnection()->selectQuery(); } /** @@ -274,7 +254,7 @@ public function getSelectBuilder(): SelectQuery */ public function getInsertBuilder(): InsertQuery { - return $this->getDecoratedConnection()->insertQuery(); + return $this->getConnection()->insertQuery(); } /** @@ -282,7 +262,7 @@ public function getInsertBuilder(): InsertQuery */ public function getUpdateBuilder(): UpdateQuery { - return $this->getDecoratedConnection()->updateQuery(); + return $this->getConnection()->updateQuery(); } /** @@ -290,24 +270,18 @@ public function getUpdateBuilder(): UpdateQuery */ public function getDeleteBuilder(): DeleteQuery { - return $this->getDecoratedConnection()->deleteQuery(); + return $this->getConnection()->deleteQuery(); } /** * Executes a query and returns PDOStatement. * * @param string $sql SQL - * @return mixed + * @return \Cake\Database\StatementInterface */ public function query(string $sql, array $params = []): mixed { - if (empty($params)) { - return $this->getConnection()->query($sql); - } - $stmt = $this->getConnection()->prepare($sql); - $result = $stmt->execute($params); - - return $result ? $stmt : false; + return $this->getConnection()->execute($sql, $params); } /** @@ -315,7 +289,7 @@ public function query(string $sql, array $params = []): mixed */ public function fetchRow(string $sql): array|false { - return $this->query($sql)->fetch(); + return $this->getConnection()->execute($sql)->fetch('assoc'); } /** @@ -323,7 +297,7 @@ public function fetchRow(string $sql): array|false */ public function fetchAll(string $sql): array { - return $this->query($sql)->fetchAll(); + return $this->getConnection()->execute($sql)->fetchAll('assoc'); } /** @@ -349,8 +323,7 @@ public function insert(Table $table, array $row): void $this->output->writeln($sql); } else { $sql .= ' VALUES (' . implode(', ', array_fill(0, count($columns), '?')) . ')'; - $stmt = $this->getConnection()->prepare($sql); - $stmt->execute(array_values($row)); + $this->getConnection()->execute($sql, array_values($row)); } } @@ -369,8 +342,12 @@ protected function quoteValue(mixed $value): mixed if ($value === null) { return 'null'; } + // TODO remove hacks like this by using cake's database layer better. + $driver = $this->getConnection()->getDriver(); + $method = new ReflectionMethod($driver, 'getPdo'); + $method->setAccessible(true); - return $this->getConnection()->quote($value); + return $method->invoke($driver)->quote($value); } /** @@ -381,7 +358,12 @@ protected function quoteValue(mixed $value): mixed */ protected function quoteString(string $value): string { - return $this->getConnection()->quote($value); + // TODO remove hacks like this by using cake's database layer better. + $driver = $this->getConnection()->getDriver(); + $method = new ReflectionMethod($driver, 'getPdo'); + $method->setAccessible(true); + + return $method->invoke($driver)->quote($value); } /** @@ -411,7 +393,6 @@ public function bulkinsert(Table $table, array $rows): void $count_vars = count($rows); $queries = array_fill(0, $count_vars, $query); $sql .= implode(',', $queries); - $stmt = $this->getConnection()->prepare($sql); $vals = []; foreach ($rows as $row) { @@ -423,8 +404,7 @@ public function bulkinsert(Table $table, array $rows): void } } } - - $stmt->execute($vals); + $this->getConnection()->execute($sql, $vals); } } @@ -684,7 +664,7 @@ protected function getDefaultValueDefinition(mixed $default, ?string $columnType $default = (string)$default; } elseif (is_string($default) && stripos($default, 'CURRENT_TIMESTAMP') !== 0) { // Ensure a defaults of CURRENT_TIMESTAMP(3) is not quoted. - $default = $this->getConnection()->quote($default); + $default = $this->quoteString($default); } elseif (is_bool($default)) { $default = $this->castToBool($default); } elseif ($default !== null && $columnType === static::PHINX_TYPE_BOOLEAN) { diff --git a/src/Db/Adapter/SqliteAdapter.php b/src/Db/Adapter/SqliteAdapter.php index 212eb962..a0576b0c 100644 --- a/src/Db/Adapter/SqliteAdapter.php +++ b/src/Db/Adapter/SqliteAdapter.php @@ -131,7 +131,7 @@ class SqliteAdapter extends PdoAdapter */ public function databaseVersionAtLeast(string $ver): bool { - $actual = $this->query('SELECT sqlite_version()')->fetchColumn(); + $actual = $this->query('SELECT sqlite_version()')->fetchColumn(0); return version_compare($actual, $ver, '>='); } @@ -145,53 +145,8 @@ public function databaseVersionAtLeast(string $ver): bool */ public function connect(): void { - if ($this->connection === null) { - if (!class_exists('PDO') || !in_array('sqlite', PDO::getAvailableDrivers(), true)) { - // @codeCoverageIgnoreStart - throw new RuntimeException('You need to enable the PDO_SQLITE extension for Migrations to run properly.'); - // @codeCoverageIgnoreEnd - } - - $options = $this->getOptions(); - - if (PHP_VERSION_ID < 80100 && (!empty($options['mode']) || !empty($options['cache']))) { - throw new RuntimeException('SQLite URI support requires PHP 8.1.'); - } elseif ((!empty($options['mode']) || !empty($options['cache'])) && !empty($options['memory'])) { - throw new RuntimeException('Memory must not be set when cache or mode are.'); - } elseif (PHP_VERSION_ID >= 80100 && (!empty($options['mode']) || !empty($options['cache']))) { - $params = []; - if (!empty($options['cache'])) { - $params[] = 'cache=' . $options['cache']; - } - if (!empty($options['mode'])) { - $params[] = 'mode=' . $options['mode']; - } - $dsn = 'sqlite:file:' . ($options['name'] ?? '') . '?' . implode('&', $params); - } else { - // use a memory database if the option was specified - if (!empty($options['memory']) || $options['name'] === static::MEMORY) { - $dsn = 'sqlite:' . static::MEMORY; - } else { - $dsn = 'sqlite:' . $options['name'] . $this->suffix; - } - } - - $driverOptions = []; - - // use custom data fetch mode - if (!empty($options['fetch_mode'])) { - $driverOptions[PDO::ATTR_DEFAULT_FETCH_MODE] = constant('\PDO::FETCH_' . strtoupper($options['fetch_mode'])); - } - - // pass \PDO::ATTR_PERSISTENT to driver options instead of useless setting it after instantiation - if (isset($options['attr_persistent'])) { - $driverOptions[PDO::ATTR_PERSISTENT] = $options['attr_persistent']; - } - - $db = $this->createPdoConnection($dsn, null, null, $driverOptions); - - $this->setConnection($db); - } + $this->getConnection()->getDriver()->connect(); + $this->setConnection($this->getConnection()); } /** @@ -218,7 +173,7 @@ public function setOptions(array $options): AdapterInterface */ public function disconnect(): void { - $this->connection = null; + $this->getConnection()->getDriver()->disconnect(); } /** @@ -234,7 +189,7 @@ public function hasTransactions(): bool */ public function beginTransaction(): void { - $this->getConnection()->beginTransaction(); + $this->getConnection()->begin(); } /** @@ -1018,7 +973,7 @@ protected function validateForeignKeys(AlterInstructions $instructions, string $ "SELECT name FROM sqlite_master WHERE type = 'table' AND name != ?", [$tableName] ) - ->fetchAll(); + ->fetchAll('assoc'); foreach ($otherTables as $otherTable) { $foreignKeyList = $this->getTableInfo($otherTable['name'], 'foreign_key_list'); @@ -1968,27 +1923,4 @@ protected function getForeignKeySqlDefinition(ForeignKey $foreignKey): string return $def; } - - /** - * @inheritDoc - */ - public function getDecoratedConnection(): Connection - { - if (isset($this->decoratedConnection)) { - return $this->decoratedConnection; - } - - $options = $this->getOptions(); - $options['quoteIdentifiers'] = true; - - if (!empty($options['name'])) { - $options['database'] = $options['name']; - - if (file_exists($options['name'] . $this->suffix)) { - $options['database'] = $options['name'] . $this->suffix; - } - } - - return $this->decoratedConnection = $this->buildConnection(SqliteDriver::class, $options); - } } diff --git a/src/Migration/Environment.php b/src/Migration/Environment.php index 31a8fbb9..85e89f60 100644 --- a/src/Migration/Environment.php +++ b/src/Migration/Environment.php @@ -347,13 +347,13 @@ public function getAdapter(): AdapterInterface // AdapterFactory to choose the adapter. // Adapters should use Connection API instead of PDO. $connection = ConnectionManager::get($options['connection']); + $options['connection'] = $connection; // Get the driver classname as those are aligned with adapter names. $driver = $connection->getDriver(); $driverClass = get_class($driver); $driverName = strtolower(substr($driverClass, (int)strrpos($driverClass, '\\') + 1)); - - $options['connection'] = $connection; + $options['adapter'] = $driverName; $factory = AdapterFactory::instance(); $adapter = $factory diff --git a/tests/TestCase/Db/Adapter/SqliteAdapterTest.php b/tests/TestCase/Db/Adapter/SqliteAdapterTest.php index 705f26ef..724cb55b 100644 --- a/tests/TestCase/Db/Adapter/SqliteAdapterTest.php +++ b/tests/TestCase/Db/Adapter/SqliteAdapterTest.php @@ -4,6 +4,7 @@ namespace Migrations\Test\Db\Adapter; use BadMethodCallException; +use Cake\Database\Connection; use Cake\Database\Query; use Cake\Datasource\ConnectionManager; use Exception; @@ -29,34 +30,32 @@ class SqliteAdapterTest extends TestCase { + private array $config; + /** * @var \Migrations\Db\Adapter\SqliteAdapter */ private $adapter; - /** - * @var array - */ - private $config; - protected function setUp(): void { + /** @var array $config */ $config = ConnectionManager::getConfig('test'); - // Emulate the results of Util::parseDsn() - $this->config = [ - 'adapter' => $config['scheme'], - 'host' => $config['host'], - 'name' => $config['database'], - ]; - if ($this->config['adapter'] !== 'sqlite') { + if ($config['scheme'] !== 'sqlite') { $this->markTestSkipped('SQLite tests disabled.'); } + $this->config = [ + 'adapter' => 'sqlite', + 'suffix' => '', + 'connection' => ConnectionManager::get('test'), + 'database' => $config['database'], + ]; $this->adapter = new SqliteAdapter($this->config, new ArrayInput([]), new NullOutput()); - if ($this->config['name'] !== ':memory:') { + if ($config['database'] !== ':memory:') { // ensure the database is empty for each test - $this->adapter->dropDatabase($this->config['name']); - $this->adapter->createDatabase($this->config['name']); + $this->adapter->dropDatabase($config['database']); + $this->adapter->createDatabase($config['database']); } // leave the adapter in a disconnected state for each test @@ -70,17 +69,7 @@ protected function tearDown(): void public function testConnection() { - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - $this->assertSame(PDO::ERRMODE_EXCEPTION, $this->adapter->getConnection()->getAttribute(PDO::ATTR_ERRMODE)); - } - - public function testConnectionWithFetchMode() - { - $options = $this->adapter->getOptions(); - $options['fetch_mode'] = 'assoc'; - $this->adapter->setOptions($options); - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - $this->assertSame(PDO::FETCH_ASSOC, $this->adapter->getConnection()->getAttribute(PDO::ATTR_DEFAULT_FETCH_MODE)); + $this->assertInstanceOf(Connection::class, $this->adapter->getConnection()); } public function testBeginTransaction() @@ -95,8 +84,6 @@ public function testBeginTransaction() public function testRollbackTransaction() { - $this->adapter->getConnection() - ->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); $this->adapter->beginTransaction(); $this->adapter->rollbackTransaction(); @@ -108,8 +95,6 @@ public function testRollbackTransaction() public function testCommitTransactionTransaction() { - $this->adapter->getConnection() - ->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); $this->adapter->beginTransaction(); $this->adapter->commitTransaction(); @@ -286,11 +271,6 @@ public function testCreateTableWithNamedIndexes() $this->assertTrue($this->adapter->hasIndexByName('table1', 'myemailindex')); } - public function testCreateTableWithMultiplePKsAndUniqueIndexes() - { - $this->markTestIncomplete(); - } - public function testCreateTableWithForeignKey() { $refTable = new Table('ref_table', [], $this->adapter); @@ -1583,11 +1563,11 @@ public function testDropForeignKeyByName() public function testHasDatabase() { - if ($this->config['name'] === ':memory:') { + if ($this->config['database'] === ':memory:') { $this->markTestSkipped('Skipping hasDatabase() when testing in-memory db.'); } $this->assertFalse($this->adapter->hasDatabase('fake_database_name')); - $this->assertTrue($this->adapter->hasDatabase($this->config['name'])); + $this->assertTrue($this->adapter->hasDatabase($this->config['database'])); } public function testDropDatabase() @@ -1759,8 +1739,6 @@ public function testBulkInsertDataEnum() public function testNullWithoutDefaultValue() { - $this->markTestSkipped('Skipping for now. See Github Issue #265.'); - // construct table with default/null combinations $table = new Table('table1', [], $this->adapter); $table->addColumn('aa', 'string', ['null' => true]) // no default value @@ -1860,7 +1838,7 @@ public function testDumpInsert() $countQuery = $this->adapter->query('SELECT COUNT(*) FROM table1'); $this->assertTrue($countQuery->execute()); $res = $countQuery->fetchAll(); - $this->assertEquals(0, $res[0]['COUNT(*)']); + $this->assertEquals(0, $res[0][0]); } /** @@ -1901,7 +1879,7 @@ public function testDumpBulkinsert() $countQuery = $this->adapter->query('SELECT COUNT(*) FROM table1'); $this->assertTrue($countQuery->execute()); $res = $countQuery->fetchAll(); - $this->assertEquals(0, $res[0]['COUNT(*)']); + $this->assertEquals(0, $res[0][0]); } public function testDumpCreateTableAndThenInsert() @@ -1998,13 +1976,13 @@ public function testQueryWithParams() ]); $countQuery = $this->adapter->query('SELECT COUNT(*) AS c FROM table1 WHERE int_col > ?', [5]); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(2, $res[0]['c']); $this->adapter->execute('UPDATE table1 SET int_col = ? WHERE int_col IS NULL', [12]); $countQuery->execute([1]); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(3, $res[0]['c']); } @@ -2306,11 +2284,11 @@ public function testHasTable($createName, $tableName, $exp) { // Test case for issue #1535 $conn = $this->adapter->getConnection(); - $conn->exec('ATTACH DATABASE \':memory:\' as etc'); - $conn->exec('ATTACH DATABASE \':memory:\' as "main.db"'); - $conn->exec(sprintf('DROP TABLE IF EXISTS %s', $createName)); + $conn->execute('ATTACH DATABASE \':memory:\' as etc'); + $conn->execute('ATTACH DATABASE \':memory:\' as "main.db"'); + $conn->execute(sprintf('DROP TABLE IF EXISTS %s', $createName)); $this->assertFalse($this->adapter->hasTable($tableName), sprintf('Adapter claims table %s exists when it does not', $tableName)); - $conn->exec(sprintf('CREATE TABLE %s (a text)', $createName)); + $conn->execute(sprintf('CREATE TABLE %s (a text)', $createName)); if ($exp == true) { $this->assertTrue($this->adapter->hasTable($tableName), sprintf('Adapter claims table %s does not exist when it does', $tableName)); } else { @@ -2358,7 +2336,17 @@ public static function provideTableNamesForPresenceCheck() public function testHasIndex($tableDef, $cols, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec($tableDef); + if (strpos($tableDef, ';') !== false) { + $queries = explode(';', $tableDef); + foreach ($queries as $query) { + $stmt = $conn->execute($query); + $stmt->closeCursor(); + } + } else { + $stmt = $conn->execute($tableDef); + $stmt->closeCursor(); + } + $this->assertEquals($exp, $this->adapter->hasIndex('t', $cols)); } @@ -2366,7 +2354,7 @@ public static function provideIndexColumnsToCheck() { return [ ['create table t(a text)', 'a', false], - ['create table t(a text); create index test on t(a);', 'a', true], + ['create table t(a text); create index test on t(a)', 'a', true], ['create table t(a text unique)', 'a', true], ['create table t(a text primary key)', 'a', true], ['create table t(a text unique, b text unique)', ['a', 'b'], false], @@ -2393,7 +2381,16 @@ public static function provideIndexColumnsToCheck() public function testHasIndexByName($tableDef, $index, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec($tableDef); + if (strpos($tableDef, ';') !== false) { + $queries = explode(';', $tableDef); + foreach ($queries as $query) { + $stmt = $conn->execute($query); + $stmt->closeCursor(); + } + } else { + $stmt = $conn->execute($tableDef); + $stmt->closeCursor(); + } $this->assertEquals($exp, $this->adapter->hasIndexByName('t', $index)); } @@ -2401,12 +2398,12 @@ public static function provideIndexNamesToCheck() { return [ ['create table t(a text)', 'test', false], - ['create table t(a text); create index test on t(a);', 'test', true], - ['create table t(a text); create index test on t(a);', 'TEST', true], - ['create table t(a text); create index "TEST" on t(a);', 'test', true], + ['create table t(a text); create index test on t(a)', 'test', true], + ['create table t(a text); create index test on t(a)', 'TEST', true], + ['create table t(a text); create index "TEST" on t(a)', 'test', true], ['create table t(a text unique)', 'sqlite_autoindex_t_1', true], ['create table t(a text primary key)', 'sqlite_autoindex_t_1', true], - ['create table not_t(a text); create index test on not_t(a);', 'test', false], // test checks table t which does not exist + ['create table not_t(a text); create index test on not_t(a)', 'test', false], // test checks table t which does not exist ['create table t(a text unique); create temp table t(a text)', 'sqlite_autoindex_t_1', false], ]; } @@ -2422,7 +2419,16 @@ public function testHasPrimaryKey($tableDef, $key, $exp) { $this->assertFalse($this->adapter->hasTable('t'), 'Dirty test fixture'); $conn = $this->adapter->getConnection(); - $conn->exec($tableDef); + if (strpos($tableDef, ';') !== false) { + $queries = explode(';', $tableDef); + foreach ($queries as $query) { + $stmt = $conn->execute($query); + $stmt->closeCursor(); + } + } else { + $stmt = $conn->execute($tableDef); + $stmt->closeCursor(); + } $this->assertSame($exp, $this->adapter->hasPrimaryKey('t', $key)); } @@ -2488,8 +2494,18 @@ public function testHasNamedPrimaryKey() public function testHasForeignKey($tableDef, $key, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec('CREATE TABLE other(a integer, b integer, c integer)'); - $conn->exec($tableDef); + $conn->execute('CREATE TABLE other(a integer, b integer, c integer)'); + if (strpos($tableDef, ';') !== false) { + $queries = explode(';', $tableDef); + foreach ($queries as $query) { + $stmt = $conn->execute($query); + $stmt->closeCursor(); + } + } else { + $stmt = $conn->execute($tableDef); + $stmt->closeCursor(); + } + $this->assertSame($exp, $this->adapter->hasForeignKey('t', $key)); } @@ -2929,7 +2945,17 @@ public static function provideDatabaseVersionStrings() public function testHasColumn($tableDef, $col, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec($tableDef); + if (strpos($tableDef, ';') !== false) { + $queries = explode(';', $tableDef); + foreach ($queries as $query) { + $stmt = $conn->execute($query); + $stmt->closeCursor(); + } + } else { + $stmt = $conn->execute($tableDef); + $stmt->closeCursor(); + } + $this->assertEquals($exp, $this->adapter->hasColumn('t', $col)); } @@ -2960,7 +2986,7 @@ public static function provideColumnNamesToCheck() public function testGetColumns() { $conn = $this->adapter->getConnection(); - $conn->exec('create table t(a integer, b text, c char(5), d integer(12,6), e integer not null, f integer null)'); + $conn->execute('create table t(a integer, b text, c char(5), d integer(12,6), e integer not null, f integer null)'); $exp = [ ['name' => 'a', 'type' => 'integer', 'null' => true, 'limit' => null, 'precision' => null, 'scale' => null], ['name' => 'b', 'type' => 'text', 'null' => true, 'limit' => null, 'precision' => null, 'scale' => null], @@ -2987,7 +3013,7 @@ public function testGetColumns() public function testGetColumnsForIdentity($tableDef, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec($tableDef); + $conn->execute($tableDef); $cols = $this->adapter->getColumns('t'); $act = []; foreach ($cols as $col) { @@ -3019,7 +3045,7 @@ public static function provideIdentityCandidates() public function testGetColumnsForDefaults($tableDef, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec($tableDef); + $conn->execute($tableDef); $act = $this->adapter->getColumns('t')[0]->getDefault(); if (is_object($exp)) { $this->assertEquals($exp, $act); @@ -3096,7 +3122,7 @@ public function testGetColumnsForBooleanDefaults($tableDef, $exp) $this->markTestSkipped('SQLite 3.24.0 or later is required for this test.'); } $conn = $this->adapter->getConnection(); - $conn->exec($tableDef); + $conn->execute($tableDef); $act = $this->adapter->getColumns('t')[0]->getDefault(); if (is_object($exp)) { $this->assertEquals($exp, $act); @@ -3124,16 +3150,17 @@ public static function provideBooleanDefaultValues() public function testTruncateTable($tableDef, $tableName, $tableId) { $conn = $this->adapter->getConnection(); - $conn->exec($tableDef); - $conn->exec("INSERT INTO $tableId default values"); - $conn->exec("INSERT INTO $tableId default values"); - $conn->exec("INSERT INTO $tableId default values"); - $this->assertEquals(3, $conn->query("select count(*) from $tableId")->fetchColumn(), 'Broken fixture: data were not inserted properly'); - $this->assertEquals(3, $conn->query("select max(id) from $tableId")->fetchColumn(), 'Broken fixture: data were not inserted properly'); + $conn->execute($tableDef); + $conn->execute("INSERT INTO $tableId default values"); + $conn->execute("INSERT INTO $tableId default values"); + $conn->execute("INSERT INTO $tableId default values"); + $this->assertEquals(3, $conn->execute("select count(*) from $tableId")->fetchColumn(0), 'Broken fixture: data were not inserted properly'); + $this->assertEquals(3, $conn->execute("select max(id) from $tableId")->fetchColumn(0), 'Broken fixture: data were not inserted properly'); $this->adapter->truncateTable($tableName); - $this->assertEquals(0, $conn->query("select count(*) from $tableId")->fetchColumn(), 'Table was not truncated'); - $conn->exec("INSERT INTO $tableId default values"); - $this->assertEquals(1, $conn->query("select max(id) from $tableId")->fetchColumn(), 'Autoincrement was not reset'); + $this->assertEquals(0, $conn->execute("select count(*) from $tableId")->fetchColumn(0), 'Table was not truncated'); + $conn->execute("INSERT INTO $tableId default values"); + $this->assertEquals(1, $conn->execute("select max(id) from $tableId")->fetchColumn(0), 'Autoincrement was not reset'); + $conn->execute("DROP TABLE $tableId"); } /** @@ -3305,30 +3332,10 @@ public function testForeignKeyReferenceCorrectAfterDropForeignKey() $this->assertStringContainsString("REFERENCES `{$refTable->getName()}` (`id`)", $sql); } - public function testInvalidPdoAttribute() - { - $adapter = new SqliteAdapter($this->config + ['attr_invalid' => true]); - $this->expectException(UnexpectedValueException::class); - $this->expectExceptionMessage('Invalid PDO attribute: attr_invalid (\PDO::ATTR_INVALID)'); - $adapter->connect(); - } - public function testPdoExceptionUpdateNonExistingTable() { $this->expectException(PDOException::class); $table = new Table('non_existing_table', [], $this->adapter); $table->addColumn('column', 'string')->update(); } - - public function testPdoPersistentConnection() - { - $adapter = new SqliteAdapter($this->config + ['attr_persistent' => true]); - $this->assertTrue($adapter->getConnection()->getAttribute(PDO::ATTR_PERSISTENT)); - } - - public function testPdoNotPersistentConnection() - { - $adapter = new SqliteAdapter($this->config); - $this->assertFalse($adapter->getConnection()->getAttribute(PDO::ATTR_PERSISTENT)); - } } From 125cdd0c164c65bedeb791a8cf8de4d5515a06c0 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 19 Feb 2024 18:53:09 -0500 Subject: [PATCH 02/10] Update MysqlAdapter to use Connection - Update implementation to use `Connection` methods - Remove redundant tests --- src/Db/Adapter/MysqlAdapter.php | 113 +++------- .../TestCase/Db/Adapter/MysqlAdapterTest.php | 193 ++++-------------- 2 files changed, 66 insertions(+), 240 deletions(-) diff --git a/src/Db/Adapter/MysqlAdapter.php b/src/Db/Adapter/MysqlAdapter.php index 580f4da1..c8912e35 100644 --- a/src/Db/Adapter/MysqlAdapter.php +++ b/src/Db/Adapter/MysqlAdapter.php @@ -102,63 +102,15 @@ class MysqlAdapter extends PdoAdapter */ public function connect(): void { - if ($this->connection === null) { - if (!class_exists('PDO') || !in_array('mysql', PDO::getAvailableDrivers(), true)) { - // @codeCoverageIgnoreStart - throw new RuntimeException('You need to enable the PDO_Mysql extension for Phinx to run properly.'); - // @codeCoverageIgnoreEnd - } - - $options = $this->getOptions(); - - $dsn = 'mysql:'; - - if (!empty($options['unix_socket'])) { - // use socket connection - $dsn .= 'unix_socket=' . $options['unix_socket']; - } else { - // use network connection - $dsn .= 'host=' . $options['host']; - if (!empty($options['port'])) { - $dsn .= ';port=' . $options['port']; - } - } - - $dsn .= ';dbname=' . $options['name']; - - // charset support - if (!empty($options['charset'])) { - $dsn .= ';charset=' . $options['charset']; - } - - $driverOptions = []; - - // use custom data fetch mode - if (!empty($options['fetch_mode'])) { - $driverOptions[PDO::ATTR_DEFAULT_FETCH_MODE] = constant('\PDO::FETCH_' . strtoupper($options['fetch_mode'])); - } - - // pass \PDO::ATTR_PERSISTENT to driver options instead of useless setting it after instantiation - if (isset($options['attr_persistent'])) { - $driverOptions[PDO::ATTR_PERSISTENT] = $options['attr_persistent']; - } - - // support arbitrary \PDO::MYSQL_ATTR_* driver options and pass them to PDO - // https://php.net/manual/en/ref.pdo-mysql.php#pdo-mysql.constants - foreach ($options as $key => $option) { - if (strpos($key, 'mysql_attr_') === 0) { - $pdoConstant = '\PDO::' . strtoupper($key); - if (!defined($pdoConstant)) { - throw new UnexpectedValueException('Invalid PDO attribute: ' . $key . ' (' . $pdoConstant . ')'); - } - $driverOptions[constant($pdoConstant)] = $option; - } - } + $this->getConnection()->getDriver()->connect(); + $this->setConnection($this->getConnection()); + } - $db = $this->createPdoConnection($dsn, $options['user'] ?? null, $options['pass'] ?? null, $driverOptions); + public function setConnection(Connection $connection): AdapterInterface + { + $connection->execute(sprintf("USE %s", $this->getOption('database'))); - $this->setConnection($db); - } + return parent::setConnection($connection); } /** @@ -166,7 +118,7 @@ public function connect(): void */ public function disconnect(): void { - $this->connection = null; + $this->getConnection()->getDriver()->disconnect(); } /** @@ -182,7 +134,7 @@ public function hasTransactions(): bool */ public function beginTransaction(): void { - $this->execute('START TRANSACTION'); + $this->getConnection()->begin(); } /** @@ -190,7 +142,7 @@ public function beginTransaction(): void */ public function commitTransaction(): void { - $this->execute('COMMIT'); + $this->getConnection()->commit(); } /** @@ -198,7 +150,7 @@ public function commitTransaction(): void */ public function rollbackTransaction(): void { - $this->execute('ROLLBACK'); + $this->getConnection()->rollback(); } /** @@ -237,7 +189,7 @@ public function hasTable(string $tableName): bool $options = $this->getOptions(); - return $this->hasTableWithSchema($options['name'], $tableName); + return $this->hasTableWithSchema($options['database'], $tableName); } /** @@ -318,7 +270,7 @@ public function createTable(Table $table, array $columns = [], array $indexes = // set the table comment if (isset($options['comment'])) { - $optionsStr .= sprintf(' COMMENT=%s ', $this->getConnection()->quote($options['comment'])); + $optionsStr .= sprintf(' COMMENT=%s ', $this->quoteString($options['comment'])); } // set the table row format @@ -406,7 +358,7 @@ protected function getChangeCommentInstructions(Table $table, ?string $newCommen // passing 'null' is to remove table comment $newComment = $newComment ?? ''; - $sql = sprintf(' COMMENT=%s ', $this->getConnection()->quote($newComment)); + $sql = sprintf(' COMMENT=%s ', $this->quoteString($newComment)); $instructions->addAlter($sql); return $instructions; @@ -797,7 +749,7 @@ public function getPrimaryKey(string $tableName): array WHERE t.CONSTRAINT_TYPE='PRIMARY KEY' AND t.TABLE_SCHEMA='%s' AND t.TABLE_NAME='%s'", - $options['name'], + $options['database'], $tableName )); @@ -1298,6 +1250,7 @@ public function createDatabase(string $name, array $options = []): void } else { $this->execute(sprintf('CREATE DATABASE `%s` DEFAULT CHARACTER SET `%s`', $name, $charset)); } + $this->execute(sprintf("USE %s", $name)); } /** @@ -1356,7 +1309,7 @@ protected function getColumnSqlDefinition(Column $column): string // we special case NULL as it's not actually allowed an enum value, // and we want MySQL to issue an error on the create statement, but // quote coerces it to an empty string, which will not error - return $value === null ? 'NULL' : $this->getConnection()->quote($value); + return $value === null ? 'NULL' : $this->quoteString($value); }, $values)) . ')'; } @@ -1365,8 +1318,10 @@ protected function getColumnSqlDefinition(Column $column): string $def .= !$column->isSigned() && isset($this->signedColumnTypes[(string)$column->getType()]) ? ' unsigned' : ''; $def .= $column->isNull() ? ' NULL' : ' NOT NULL'; + $connection = $this->getConnection(); + $version = $connection->getDriver()->version(); if ( - version_compare($this->getAttribute(PDO::ATTR_SERVER_VERSION), '8', '>=') + version_compare($version, '8', '>=') && in_array($column->getType(), static::PHINX_TYPES_GEOSPATIAL) && !is_null($column->getSrid()) ) { @@ -1378,7 +1333,7 @@ protected function getColumnSqlDefinition(Column $column): string $default = $column->getDefault(); // MySQL 8 supports setting default for the following tested types, but only if they are "cast as expressions" if ( - version_compare($this->getAttribute(PDO::ATTR_SERVER_VERSION), '8', '>=') && + version_compare($version, '8', '>=') && is_string($default) && in_array( $column->getType(), @@ -1388,12 +1343,12 @@ protected function getColumnSqlDefinition(Column $column): string ) ) ) { - $default = Literal::from('(' . $this->getConnection()->quote($column->getDefault()) . ')'); + $default = Literal::from('(' . $this->quoteString($column->getDefault()) . ')'); } $def .= $this->getDefaultValueDefinition($default, (string)$column->getType()); if ($column->getComment()) { - $def .= ' COMMENT ' . $this->getConnection()->quote((string)$column->getComment()); + $def .= ' COMMENT ' . $this->quoteString((string)$column->getComment()); } if ($column->getUpdate()) { @@ -1508,7 +1463,7 @@ public function describeTable(string $tableName): array FROM information_schema.tables WHERE table_schema = '%s' AND table_name = '%s'", - $options['name'], + $options['database'], $tableName ); @@ -1526,24 +1481,4 @@ public function getColumnTypes(): array { return array_merge(parent::getColumnTypes(), static::$specificColumnTypes); } - - /** - * @inheritDoc - */ - public function getDecoratedConnection(): Connection - { - if (isset($this->decoratedConnection)) { - return $this->decoratedConnection; - } - - $options = $this->getOptions(); - $options = [ - 'username' => $options['user'] ?? null, - 'password' => $options['pass'] ?? null, - 'database' => $options['name'], - 'quoteIdentifiers' => true, - ] + $options; - - return $this->decoratedConnection = $this->buildConnection(MysqlDriver::class, $options); - } } diff --git a/tests/TestCase/Db/Adapter/MysqlAdapterTest.php b/tests/TestCase/Db/Adapter/MysqlAdapterTest.php index ec2a76eb..469c036f 100644 --- a/tests/TestCase/Db/Adapter/MysqlAdapterTest.php +++ b/tests/TestCase/Db/Adapter/MysqlAdapterTest.php @@ -3,6 +3,7 @@ namespace Migrations\Test\Db\Adapter; +use Cake\Database\Connection; use Cake\Database\Query; use Cake\Datasource\ConnectionManager; use InvalidArgumentException; @@ -43,18 +44,16 @@ protected function setUp(): void } // Emulate the results of Util::parseDsn() $this->config = [ - 'adapter' => $config['scheme'], - 'user' => $config['username'], - 'pass' => $config['password'], - 'host' => $config['host'], - 'name' => $config['database'], + 'adapter' => 'mysql', + 'connection' => ConnectionManager::get('test'), + 'database' => $config['database'], ]; $this->adapter = new MysqlAdapter($this->config, new ArrayInput([]), new NullOutput()); // ensure the database is empty for each test - $this->adapter->dropDatabase($this->config['name']); - $this->adapter->createDatabase($this->config['name'], ['charset' => 'utf8mb4']); + $this->adapter->dropDatabase($this->config['database']); + $this->adapter->createDatabase($this->config['database'], ['charset' => 'utf8mb4']); // leave the adapter in a disconnected state for each test $this->adapter->disconnect(); @@ -67,61 +66,14 @@ protected function tearDown(): void private function usingMysql8(): bool { - return version_compare($this->adapter->getAttribute(PDO::ATTR_SERVER_VERSION), '8.0.0', '>='); - } - - public function testConnection() - { - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - $this->assertSame(PDO::ERRMODE_EXCEPTION, $this->adapter->getConnection()->getAttribute(PDO::ATTR_ERRMODE)); - } + $version = $this->adapter->getConnection()->getDriver()->version(); - public function testConnectionWithFetchMode() - { - $options = $this->adapter->getOptions(); - $options['fetch_mode'] = 'assoc'; - $this->adapter->setOptions($options); - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - $this->assertSame(PDO::FETCH_ASSOC, $this->adapter->getConnection()->getAttribute(PDO::ATTR_DEFAULT_FETCH_MODE)); - } - - public function testConnectionWithoutPort() - { - $options = $this->adapter->getOptions(); - unset($options['port']); - $this->adapter->setOptions($options); - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); + return version_compare($version, '8.0.0', '>='); } - public function testConnectionWithInvalidCredentials() - { - $options = ['user' => 'invalid', 'pass' => 'invalid'] + $this->config; - - try { - $adapter = new MysqlAdapter($options, new ArrayInput([]), new NullOutput()); - $adapter->connect(); - $this->fail('Expected the adapter to throw an exception'); - } catch (InvalidArgumentException $e) { - $this->assertInstanceOf( - 'InvalidArgumentException', - $e, - 'Expected exception of type InvalidArgumentException, got ' . get_class($e) - ); - $this->assertStringContainsString('There was a problem connecting to the database', $e->getMessage()); - } - } - - public function testConnectionWithSocketConnection() + public function testConnection() { - if (!getenv('MYSQL_UNIX_SOCKET')) { - $this->markTestSkipped('MySQL socket connection skipped.'); - } - - $options = ['unix_socket' => getenv('MYSQL_UNIX_SOCKET')] + $this->config; - $adapter = new MysqlAdapter($options, new ArrayInput([]), new NullOutput()); - $adapter->connect(); - - $this->assertInstanceOf('\PDO', $this->adapter->getConnection()); + $this->assertInstanceOf(Connection::class, $this->adapter->getConnection()); } public function testCreatingTheSchemaTableOnConnect() @@ -199,7 +151,7 @@ public function testCreateTableWithComment() $rows = $this->adapter->fetchAll(sprintf( "SELECT TABLE_COMMENT FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA='%s' AND TABLE_NAME='ntable'", - $this->config['name'] + $this->config['database'] )); $comment = $rows[0]; @@ -227,7 +179,7 @@ public function testCreateTableWithForeignKeys() "SELECT TABLE_NAME, COLUMN_NAME, REFERENCED_TABLE_NAME, REFERENCED_COLUMN_NAME FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE WHERE TABLE_SCHEMA='%s' AND REFERENCED_TABLE_NAME='ntable_tag'", - $this->config['name'] + $this->config['database'] )); $foreignKey = $rows[0]; @@ -403,11 +355,6 @@ public function testCreateTableWithNamedIndex() $this->assertTrue($this->adapter->hasIndexByName('table1', 'myemailindex')); } - public function testCreateTableWithMultiplePKsAndUniqueIndexes() - { - $this->markTestIncomplete(); - } - public function testCreateTableWithMyISAMEngine() { $table = new Table('ntable', ['engine' => 'MyISAM'], $this->adapter); @@ -426,11 +373,6 @@ public function testCreateTableAndInheritDefaultCollation() ]; $adapter = new MysqlAdapter($options, new ArrayInput([]), new NullOutput()); - // Ensure the database is empty and the adapter is in a disconnected state - $adapter->dropDatabase($options['name']); - $adapter->createDatabase($options['name']); - $adapter->disconnect(); - $table = new Table('table_with_default_collation', [], $adapter); $table->addColumn('name', 'string') ->save(); @@ -536,7 +478,7 @@ public function testCreateTableWithLimitPK() public function testCreateTableWithSchema() { - $table = new Table($this->config['name'] . '.ntable', [], $this->adapter); + $table = new Table($this->config['database'] . '.ntable', [], $this->adapter); $table->addColumn('realname', 'string') ->addColumn('email', 'integer') ->save(); @@ -603,7 +545,7 @@ public function testAddComment() FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA='%s' AND TABLE_NAME='%s'", - $this->config['name'], + $this->config['database'], 'table1' ) ); @@ -625,7 +567,7 @@ public function testChangeComment() FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA='%s' AND TABLE_NAME='%s'", - $this->config['name'], + $this->config['database'], 'table1' ) ); @@ -647,7 +589,7 @@ public function testDropComment() FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA='%s' AND TABLE_NAME='%s'", - $this->config['name'], + $this->config['database'], 'table1' ) ); @@ -1173,7 +1115,8 @@ public function testIntegerColumnLimit() public function testDatetimeColumn() { $this->adapter->connect(); - if (version_compare($this->adapter->getAttribute(PDO::ATTR_SERVER_VERSION), '5.6.4') === -1) { + $version = $this->adapter->getConnection()->getDriver()->version(); + if (version_compare($version, '5.6.4') === -1) { $this->markTestSkipped('Cannot test datetime limit on versions less than 5.6.4'); } $table = new Table('t', [], $this->adapter); @@ -1186,7 +1129,8 @@ public function testDatetimeColumn() public function testDatetimeColumnLimit() { $this->adapter->connect(); - if (version_compare($this->adapter->getAttribute(PDO::ATTR_SERVER_VERSION), '5.6.4') === -1) { + $version = $this->adapter->getConnection()->getDriver()->version(); + if (version_compare($version, '5.6.4') === -1) { $this->markTestSkipped('Cannot test datetime limit on versions less than 5.6.4'); } $limit = 6; @@ -1200,7 +1144,8 @@ public function testDatetimeColumnLimit() public function testTimeColumnLimit() { $this->adapter->connect(); - if (version_compare($this->adapter->getAttribute(PDO::ATTR_SERVER_VERSION), '5.6.4') === -1) { + $version = $this->adapter->getConnection()->getDriver()->version(); + if (version_compare($version, '5.6.4') === -1) { $this->markTestSkipped('Cannot test datetime limit on versions less than 5.6.4'); } $limit = 3; @@ -1214,7 +1159,8 @@ public function testTimeColumnLimit() public function testTimestampColumnLimit() { $this->adapter->connect(); - if (version_compare($this->adapter->getAttribute(PDO::ATTR_SERVER_VERSION), '5.6.4') === -1) { + $version = $this->adapter->getConnection()->getDriver()->version(); + if (version_compare($version, '5.6.4') === -1) { $this->markTestSkipped('Cannot test datetime limit on versions less than 5.6.4'); } $limit = 1; @@ -1228,7 +1174,8 @@ public function testTimestampColumnLimit() public function testTimestampInvalidLimit() { $this->adapter->connect(); - if (version_compare($this->adapter->getAttribute(PDO::ATTR_SERVER_VERSION), '5.6.4') === -1) { + $version = $this->adapter->getConnection()->getDriver()->version(); + if (version_compare($version, '5.6.4') === -1) { $this->markTestSkipped('Cannot test datetime limit on versions less than 5.6.4'); } $table = new Table('t', [], $this->adapter); @@ -1343,7 +1290,7 @@ public function testDescribeTable() $this->assertContains($described['TABLE_TYPE'], ['VIEW', 'BASE TABLE']); $this->assertEquals($described['TABLE_NAME'], 't'); - $this->assertEquals($described['TABLE_SCHEMA'], $this->config['name']); + $this->assertEquals($described['TABLE_SCHEMA'], $this->config['database']); $this->assertEquals($described['TABLE_ROWS'], 0); } @@ -1421,7 +1368,7 @@ public function testAddIndexWithLimit() $this->assertTrue($table->hasIndex('email')); $index_data = $this->adapter->query(sprintf( 'SELECT SUB_PART FROM information_schema.STATISTICS WHERE TABLE_SCHEMA = "%s" AND TABLE_NAME = "table1" AND INDEX_NAME = "email"', - $this->config['name'] + $this->config['database'] ))->fetch(PDO::FETCH_ASSOC); $expected_limit = $index_data['SUB_PART']; $this->assertEquals($expected_limit, 50); @@ -1439,13 +1386,13 @@ public function testAddMultiIndexesWithLimitSpecifier() $this->assertTrue($table->hasIndex(['email', 'username'])); $index_data = $this->adapter->query(sprintf( 'SELECT SUB_PART FROM information_schema.STATISTICS WHERE TABLE_SCHEMA = "%s" AND TABLE_NAME = "table1" AND INDEX_NAME = "email" AND COLUMN_NAME = "email"', - $this->config['name'] + $this->config['database'] ))->fetch(PDO::FETCH_ASSOC); $expected_limit = $index_data['SUB_PART']; $this->assertEquals($expected_limit, 3); $index_data = $this->adapter->query(sprintf( 'SELECT SUB_PART FROM information_schema.STATISTICS WHERE TABLE_SCHEMA = "%s" AND TABLE_NAME = "table1" AND INDEX_NAME = "email" AND COLUMN_NAME = "username"', - $this->config['name'] + $this->config['database'] ))->fetch(PDO::FETCH_ASSOC); $expected_limit = $index_data['SUB_PART']; $this->assertEquals($expected_limit, 2); @@ -1463,7 +1410,7 @@ public function testAddSingleIndexesWithLimitSpecifier() $this->assertTrue($table->hasIndex('email')); $index_data = $this->adapter->query(sprintf( 'SELECT SUB_PART FROM information_schema.STATISTICS WHERE TABLE_SCHEMA = "%s" AND TABLE_NAME = "table1" AND INDEX_NAME = "email" AND COLUMN_NAME = "email"', - $this->config['name'] + $this->config['database'] ))->fetch(PDO::FETCH_ASSOC); $expected_limit = $index_data['SUB_PART']; $this->assertEquals($expected_limit, 3); @@ -1800,8 +1747,8 @@ public function testDropForeignKeyAsString() public function testHasForeignKey($tableDef, $key, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec('CREATE TABLE other(a int, b int, c int, key(a), key(b), key(a,b), key(a,b,c));'); - $conn->exec($tableDef); + $conn->execute('CREATE TABLE other(a int, b int, c int, key(a), key(b), key(a,b), key(a,b,c));'); + $conn->execute($tableDef); $this->assertSame($exp, $this->adapter->hasForeignKey('t', $key)); } @@ -1890,14 +1837,14 @@ public function testsHasForeignKeyWithSchemaDotTableName() ->addForeignKey(['ref_table_id'], 'ref_table', ['id']) ->save(); - $this->assertTrue($this->adapter->hasForeignKey($this->config['name'] . '.' . $table->getName(), ['ref_table_id'])); - $this->assertFalse($this->adapter->hasForeignKey($this->config['name'] . '.' . $table->getName(), ['ref_table_id2'])); + $this->assertTrue($this->adapter->hasForeignKey($this->config['database'] . '.' . $table->getName(), ['ref_table_id'])); + $this->assertFalse($this->adapter->hasForeignKey($this->config['database'] . '.' . $table->getName(), ['ref_table_id2'])); } public function testHasDatabase() { $this->assertFalse($this->adapter->hasDatabase('fake_database_name')); - $this->assertTrue($this->adapter->hasDatabase($this->config['name'])); + $this->assertTrue($this->adapter->hasDatabase($this->config['database'])); } public function testDropDatabase() @@ -1919,7 +1866,7 @@ public function testAddColumnWithComment() FROM information_schema.columns WHERE TABLE_SCHEMA='%s' AND TABLE_NAME='table1' ORDER BY ORDINAL_POSITION", - $this->config['name'] + $this->config['database'] )); $columnWithComment = $rows[1]; @@ -2140,7 +2087,7 @@ public function testDumpInsert() $countQuery = $this->adapter->query('SELECT COUNT(*) FROM table1'); $this->assertTrue($countQuery->execute()); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(0, $res[0]['COUNT(*)']); } @@ -2181,7 +2128,7 @@ public function testDumpBulkinsert() $countQuery = $this->adapter->query('SELECT COUNT(*) FROM table1'); $this->assertTrue($countQuery->execute()); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(0, $res[0]['COUNT(*)']); } @@ -2216,31 +2163,6 @@ public function testDumpCreateTableAndThenInsert() $this->assertStringContainsString($expectedOutput, $actualOutput, 'Passing the --dry-run option does not dump create and then insert table queries to the output'); } - public function testDumpTransaction() - { - $inputDefinition = new InputDefinition([new InputOption('dry-run')]); - $this->adapter->setInput(new ArrayInput(['--dry-run' => true], $inputDefinition)); - - $consoleOutput = new BufferedOutput(); - $this->adapter->setOutput($consoleOutput); - - $this->adapter->beginTransaction(); - $table = new Table('table1', [], $this->adapter); - - $table->addColumn('column1', 'string') - ->addColumn('column2', 'integer') - ->addColumn('column3', 'string', ['default' => 'test']) - ->save(); - $this->adapter->commitTransaction(); - $this->adapter->rollbackTransaction(); - - $actualOutput = $consoleOutput->fetch(); - // Add this to be LF - CR/LF systems independent - $actualOutput = preg_replace('~\R~u', '', $actualOutput); - $this->assertStringStartsWith('START TRANSACTION;', $actualOutput, 'Passing the --dry-run doesn\'t dump the transaction to the output'); - $this->assertStringEndsWith('COMMIT;ROLLBACK;', $actualOutput, 'Passing the --dry-run doesn\'t dump the transaction to the output'); - } - /** * Tests interaction with the query builder */ @@ -2304,13 +2226,13 @@ public function testQueryWithParams() ]); $countQuery = $this->adapter->query('SELECT COUNT(*) AS c FROM table1 WHERE int_col > ?', [5]); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(2, $res[0]['c']); $this->adapter->execute('UPDATE table1 SET int_col = ? WHERE int_col IS NULL', [12]); $countQuery->execute([1]); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(3, $res[0]['c']); } @@ -2453,31 +2375,12 @@ public function testCreateTableWithPrecisionCurrentTimestamp() $rows = $this->adapter->fetchAll(sprintf( "SELECT COLUMN_DEFAULT FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_SCHEMA='%s' AND TABLE_NAME='exampleCurrentTimestamp3'", - $this->config['name'] + $this->config['database'] )); $colDef = $rows[0]; $this->assertEqualsIgnoringCase('CURRENT_TIMESTAMP(3)', $colDef['COLUMN_DEFAULT']); } - public static function pdoAttributeProvider() - { - return [ - ['mysql_attr_invalid'], - ['attr_invalid'], - ]; - } - - /** - * @dataProvider pdoAttributeProvider - */ - public function testInvalidPdoAttribute($attribute) - { - $adapter = new MysqlAdapter($this->config + [$attribute => true]); - $this->expectException(UnexpectedValueException::class); - $this->expectExceptionMessage('Invalid PDO attribute: ' . $attribute . ' (\PDO::' . strtoupper($attribute) . ')'); - $adapter->connect(); - } - public static function integerDataTypesSQLProvider() { return [ @@ -2516,16 +2419,4 @@ public function testGetPhinxTypeFromSQLDefinition(string $sqlDefinition, array $ $this->assertSame($expectedResponse['name'], $result['name'], "Type mismatch - got '{$result['name']}' when expecting '{$expectedResponse['name']}'"); $this->assertSame($expectedResponse['limit'], $result['limit'], "Field upper boundary mismatch - got '{$result['limit']}' when expecting '{$expectedResponse['limit']}'"); } - - public function testPdoPersistentConnection() - { - $adapter = new MysqlAdapter($this->config + ['attr_persistent' => true]); - $this->assertTrue($adapter->getConnection()->getAttribute(PDO::ATTR_PERSISTENT)); - } - - public function testPdoNotPersistentConnection() - { - $adapter = new MysqlAdapter($this->config); - $this->assertFalse($adapter->getConnection()->getAttribute(PDO::ATTR_PERSISTENT)); - } } From f9e89a2836f7afb126dbadf5044b0c1f0dcb730a Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 21 Feb 2024 23:54:01 -0500 Subject: [PATCH 03/10] Update postgres to use Connection instead of Pdo --- src/Db/Adapter/PostgresAdapter.php | 163 +++++------------- .../Db/Adapter/PostgresAdapterTest.php | 138 +++------------ 2 files changed, 67 insertions(+), 234 deletions(-) diff --git a/src/Db/Adapter/PostgresAdapter.php b/src/Db/Adapter/PostgresAdapter.php index a61cc5ff..3a1cf844 100644 --- a/src/Db/Adapter/PostgresAdapter.php +++ b/src/Db/Adapter/PostgresAdapter.php @@ -63,10 +63,11 @@ class PostgresAdapter extends PdoAdapter /** * {@inheritDoc} */ - public function setConnection(PDO $connection): AdapterInterface + public function setConnection(Connection $connection): AdapterInterface { // always set here since connect() isn't always called - $this->useIdentity = (float)$connection->getAttribute(PDO::ATTR_SERVER_VERSION) >= 10; + $version = $connection->getDriver()->version(); + $this->useIdentity = (float)$version >= 10; return parent::setConnection($connection); } @@ -80,55 +81,8 @@ public function setConnection(PDO $connection): AdapterInterface */ public function connect(): void { - if ($this->connection === null) { - if (!class_exists('PDO') || !in_array('pgsql', PDO::getAvailableDrivers(), true)) { - // @codeCoverageIgnoreStart - throw new RuntimeException('You need to enable the PDO_Pgsql extension for Phinx to run properly.'); - // @codeCoverageIgnoreEnd - } - - $options = $this->getOptions(); - $dsn = 'pgsql:dbname=' . $options['name']; - - if (isset($options['host'])) { - $dsn .= ';host=' . $options['host']; - } - - // if custom port is specified use it - if (isset($options['port'])) { - $dsn .= ';port=' . $options['port']; - } - - $driverOptions = []; - - // use custom data fetch mode - if (!empty($options['fetch_mode'])) { - $driverOptions[PDO::ATTR_DEFAULT_FETCH_MODE] = - constant('\PDO::FETCH_' . strtoupper($options['fetch_mode'])); - } - - // pass \PDO::ATTR_PERSISTENT to driver options instead of useless setting it after instantiation - if (isset($options['attr_persistent'])) { - $driverOptions[PDO::ATTR_PERSISTENT] = $options['attr_persistent']; - } - - $db = $this->createPdoConnection($dsn, $options['user'] ?? null, $options['pass'] ?? null, $driverOptions); - - $schema = $options['schema'] ?? null; - if ($schema) { - try { - $db->exec('SET search_path TO ' . $this->quoteSchemaName($schema)); - } catch (PDOException $exception) { - throw new InvalidArgumentException( - sprintf('Schema does not exists: %s', $schema), - 0, - $exception - ); - } - } - - $this->setConnection($db); - } + $this->getConnection()->getDriver()->connect(); + $this->setConnection($this->getConnection()); } /** @@ -136,7 +90,7 @@ public function connect(): void */ public function disconnect(): void { - $this->connection = null; + $this->getConnection()->getDriver()->disconnect(); } /** @@ -152,7 +106,7 @@ public function hasTransactions(): bool */ public function beginTransaction(): void { - $this->execute('BEGIN'); + $this->getConnection()->begin(); } /** @@ -160,7 +114,7 @@ public function beginTransaction(): void */ public function commitTransaction(): void { - $this->execute('COMMIT'); + $this->getConnection()->commit(); } /** @@ -168,7 +122,7 @@ public function commitTransaction(): void */ public function rollbackTransaction(): void { - $this->execute('ROLLBACK'); + $this->getConnection()->rollback(); } /** @@ -210,21 +164,18 @@ public function hasTable(string $tableName): bool } $parts = $this->getSchemaName($tableName); - $result = $this->getConnection()->query( - sprintf( - 'SELECT * - FROM information_schema.tables - WHERE table_schema = %s - AND table_name = %s', - $this->getConnection()->quote($parts['schema']), - $this->getConnection()->quote($parts['table']) - ) + $connection = $this->getConnection(); + $stmt = $connection->execute( + 'SELECT * + FROM information_schema.tables + WHERE table_schema = ? + AND table_name = ?', + [$parts['schema'], $parts['table']] ); - if (!$result) { - return false; - } + $count = $stmt->rowCount(); + $stmt->closeCursor(); - return $result->rowCount() === 1; + return $count === 1; } /** @@ -310,7 +261,7 @@ public function createTable(Table $table, array $columns = [], array $indexes = $queries[] = sprintf( 'COMMENT ON TABLE %s IS %s', $this->quoteTableName($table->getName()), - $this->getConnection()->quote($options['comment']) + $this->quoteString($options['comment']) ); } @@ -369,7 +320,7 @@ protected function getChangeCommentInstructions(Table $table, ?string $newCommen // passing 'null' is to remove table comment $newComment = $newComment !== null - ? $this->getConnection()->quote($newComment) + ? $this->quoteString($newComment) : 'NULL'; $sql = sprintf( 'COMMENT ON TABLE %s IS %s', @@ -436,8 +387,8 @@ public function getColumns(string $tableName): array WHERE table_schema = %s AND table_name = %s ORDER BY ordinal_position', $this->useIdentity ? ', identity_generation' : '', - $this->getConnection()->quote($parts['schema']), - $this->getConnection()->quote($parts['table']) + $this->quoteString($parts['schema']), + $this->quoteString($parts['table']) ); $columnsInfo = $this->fetchAll($sql); foreach ($columnsInfo as $columnInfo) { @@ -504,21 +455,16 @@ public function getColumns(string $tableName): array public function hasColumn(string $tableName, string $columnName): bool { $parts = $this->getSchemaName($tableName); - $sql = sprintf( - 'SELECT count(*) + $connection = $this->getConnection(); + $sql = 'SELECT count(*) FROM information_schema.columns - WHERE table_schema = %s AND table_name = %s AND column_name = %s', - $this->getConnection()->quote($parts['schema']), - $this->getConnection()->quote($parts['table']), - $this->getConnection()->quote($columnName) - ); + WHERE table_schema = ? AND table_name = ? AND column_name = ?'; - $result = $this->fetchRow($sql); - if (!$result) { - return false; - } + $result = $connection->execute($sql, [$parts['schema'], $parts['table'], $columnName]); + $row = $result->fetch('assoc'); + $result->closeCursor(); - return $result['count'] > 0; + return $row['count'] > 0; } /** @@ -557,9 +503,9 @@ protected function getRenameColumnInstructions( 'SELECT CASE WHEN COUNT(*) > 0 THEN 1 ELSE 0 END AS column_exists FROM information_schema.columns WHERE table_schema = %s AND table_name = %s AND column_name = %s', - $this->getConnection()->quote($parts['schema']), - $this->getConnection()->quote($parts['table']), - $this->getConnection()->quote($columnName) + $this->quoteString($parts['schema']), + $this->quoteString($parts['table']), + $this->quoteString($columnName) ); $result = $this->fetchRow($sql); @@ -755,8 +701,8 @@ protected function getIndexes(string $tableName): array ORDER BY t.relname, i.relname", - $this->getConnection()->quote($parts['schema']), - $this->getConnection()->quote($parts['table']) + $this->quoteString($parts['schema']), + $this->quoteString($parts['table']) ); $rows = $this->fetchAll($sql); foreach ($rows as $row) { @@ -901,8 +847,8 @@ public function getPrimaryKey(string $tableName): array AND tc.table_schema = %s AND tc.table_name = %s ORDER BY kcu.position_in_unique_constraint", - $this->getConnection()->quote($parts['schema']), - $this->getConnection()->quote($parts['table']) + $this->quoteString($parts['schema']), + $this->quoteString($parts['table']) )); $primaryKey = [ @@ -965,8 +911,8 @@ protected function getForeignKeys(string $tableName): array JOIN information_schema.constraint_column_usage AS ccu ON ccu.constraint_name = tc.constraint_name WHERE constraint_type = 'FOREIGN KEY' AND tc.table_schema = %s AND tc.table_name = %s ORDER BY kcu.ordinal_position", - $this->getConnection()->quote($parts['schema']), - $this->getConnection()->quote($parts['table']) + $this->quoteString($parts['schema']), + $this->quoteString($parts['table']) )); foreach ($rows as $row) { $foreignKeys[$row['constraint_name']]['table'] = $row['table_name']; @@ -1298,7 +1244,7 @@ protected function getColumnCommentSqlDefinition(Column $column, string $tableNa $comment = (string)$column->getComment(); // passing 'null' is to remove column comment $comment = strcasecmp($comment, 'NULL') !== 0 - ? $this->getConnection()->quote($comment) + ? $this->quoteString($comment) : 'NULL'; return sprintf( @@ -1445,7 +1391,7 @@ public function hasSchema(string $schemaName): bool 'SELECT count(*) FROM pg_namespace WHERE nspname = %s', - $this->getConnection()->quote($schemaName) + $this->quoteString($schemaName) ); $result = $this->fetchRow($sql); if (!$result) { @@ -1576,26 +1522,6 @@ public function castToBool($value): mixed return (bool)$value ? 'TRUE' : 'FALSE'; } - /** - * @inheritDoc - */ - public function getDecoratedConnection(): Connection - { - if (isset($this->decoratedConnection)) { - return $this->decoratedConnection; - } - - $options = $this->getOptions(); - $options = [ - 'username' => $options['user'] ?? null, - 'password' => $options['pass'] ?? null, - 'database' => $options['name'], - 'quoteIdentifiers' => true, - ] + $options; - - return $this->decoratedConnection = $this->buildConnection(PostgresDriver::class, $options); - } - /** * Sets search path of schemas to look through for a table * @@ -1639,8 +1565,7 @@ public function insert(Table $table, array $row): void $this->output->writeln($sql); } else { $sql .= ' ' . $override . 'VALUES (' . implode(', ', array_fill(0, count($columns), '?')) . ')'; - $stmt = $this->getConnection()->prepare($sql); - $stmt->execute(array_values($row)); + $this->getConnection()->execute($sql, array_values($row)); } } @@ -1671,12 +1596,12 @@ public function bulkinsert(Table $table, array $rows): void $sql .= implode(', ', $values) . ';'; $this->output->writeln($sql); } else { + $connection = $this->getConnection(); $count_keys = count($keys); $query = '(' . implode(', ', array_fill(0, $count_keys, '?')) . ')'; $count_vars = count($rows); $queries = array_fill(0, $count_vars, $query); $sql .= implode(',', $queries); - $stmt = $this->getConnection()->prepare($sql); $vals = []; foreach ($rows as $row) { @@ -1689,7 +1614,7 @@ public function bulkinsert(Table $table, array $rows): void } } - $stmt->execute($vals); + $connection->execute($sql, $vals); } } } diff --git a/tests/TestCase/Db/Adapter/PostgresAdapterTest.php b/tests/TestCase/Db/Adapter/PostgresAdapterTest.php index e52d85b4..5138d295 100644 --- a/tests/TestCase/Db/Adapter/PostgresAdapterTest.php +++ b/tests/TestCase/Db/Adapter/PostgresAdapterTest.php @@ -3,6 +3,7 @@ namespace Migrations\Test\Db\Adapter; +use Cake\Database\Connection; use Cake\Database\Query; use Cake\Datasource\ConnectionManager; use InvalidArgumentException; @@ -59,11 +60,9 @@ protected function setUp(): void // Emulate the results of Util::parseDsn() $this->config = [ - 'adapter' => $config['scheme'], - 'user' => $config['username'], - 'pass' => $config['password'], - 'host' => $config['host'], - 'name' => $config['database'], + 'adapter' => 'postgres', + 'connection' => ConnectionManager::get('test'), + 'database' => $config['database'], ]; if (!self::isPostgresAvailable()) { @@ -94,62 +93,14 @@ protected function tearDown(): void private function usingPostgres10(): bool { - return version_compare($this->adapter->getConnection()->getAttribute(PDO::ATTR_SERVER_VERSION), '10.0.0', '>='); - } - - public function testConnection() - { - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - $this->assertSame(PDO::ERRMODE_EXCEPTION, $this->adapter->getConnection()->getAttribute(PDO::ATTR_ERRMODE)); - } - - public function testConnectionWithFetchMode() - { - $options = $this->adapter->getOptions(); - $options['fetch_mode'] = 'assoc'; - $this->adapter->setOptions($options); - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - $this->assertSame(PDO::FETCH_ASSOC, $this->adapter->getConnection()->getAttribute(PDO::ATTR_DEFAULT_FETCH_MODE)); - } - - public function testConnectionWithoutPort() - { - $options = $this->adapter->getOptions(); - unset($options['port']); - $this->adapter->setOptions($options); - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - } - - public function testConnectionWithInvalidCredentials() - { - $options = ['user' => 'invalidu', 'pass' => 'invalid'] + $this->config; + $version = $this->adapter->getConnection()->getDriver()->version(); - try { - $adapter = new PostgresAdapter($options, new ArrayInput([]), new NullOutput()); - $adapter->connect(); - $this->fail('Expected the adapter to throw an exception'); - } catch (InvalidArgumentException $e) { - $this->assertInstanceOf( - 'InvalidArgumentException', - $e, - 'Expected exception of type InvalidArgumentException, got ' . get_class($e) - ); - $this->assertStringContainsString('There was a problem connecting to the database', $e->getMessage()); - } + return version_compare($version, '10.0.0', '>='); } - public function testConnectionWithSocketConnection() + public function testConnection() { - if (!getenv('POSTGRES_TEST_SOCKETS')) { - $this->markTestSkipped('Postgres socket connection skipped.'); - } - - $options = $this->config; - unset($options['host']); - $adapter = new PostgresAdapter($options, new ArrayInput([]), new NullOutput()); - $adapter->connect(); - - $this->assertInstanceOf('\PDO', $this->adapter->getConnection()); + $this->assertInstanceOf(Connection::class, $this->adapter->getConnection()); } public function testCreatingTheSchemaTableOnConnect() @@ -770,7 +721,7 @@ public function testAddColumnWithComment() from pg_catalog.pg_class c where c.relname=cols.table_name ) as column_comment FROM information_schema.columns cols - WHERE cols.table_catalog=\'' . $this->config['name'] . '\' + WHERE cols.table_catalog=\'' . $this->config['database'] . '\' AND cols.table_name=\'table1\' AND cols.column_name = \'email\'' ); @@ -1733,8 +1684,8 @@ public function testDropForeignKeyByName() public function testHasForeignKey($tableDef, $key, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec('CREATE TABLE other(a int, b int, c int, unique(a), unique(b), unique(a,b), unique(a,b,c));'); - $conn->exec($tableDef); + $conn->execute('CREATE TABLE other(a int, b int, c int, unique(a), unique(b), unique(a,b), unique(a,b,c));'); + $conn->execute($tableDef); $this->assertSame($exp, $this->adapter->hasForeignKey('t', $key)); } @@ -1824,7 +1775,7 @@ public function testDropForeignKeyNotDroppingPrimaryKey() public function testHasDatabase() { $this->assertFalse($this->adapter->hasDatabase('fake_database_name')); - $this->assertTrue($this->adapter->hasDatabase($this->config['name'])); + $this->assertTrue($this->adapter->hasDatabase($this->config['database'])); } public function testDropDatabase() @@ -1946,7 +1897,7 @@ public function testCanAddColumnComment() from pg_catalog.pg_class c where c.relname=cols.table_name ) as column_comment FROM information_schema.columns cols - WHERE cols.table_catalog=\'' . $this->config['name'] . '\' + WHERE cols.table_catalog=\'' . $this->config['database'] . '\' AND cols.table_name=\'table1\' AND cols.column_name = \'field1\'' ); @@ -1966,7 +1917,7 @@ public function testCanAddCommentForColumnWithReservedName() from pg_catalog.pg_class c where c.relname=cols.table_name ) as column_comment FROM information_schema.columns cols - WHERE cols.table_catalog=\'' . $this->config['name'] . '\' + WHERE cols.table_catalog=\'' . $this->config['database'] . '\' AND cols.table_name=\'user\' AND cols.column_name = \'index\'' ); @@ -1999,7 +1950,7 @@ public function testCanChangeColumnComment() from pg_catalog.pg_class c where c.relname=cols.table_name ) as column_comment FROM information_schema.columns cols - WHERE cols.table_catalog=\'' . $this->config['name'] . '\' + WHERE cols.table_catalog=\'' . $this->config['database'] . '\' AND cols.table_name=\'table1\' AND cols.column_name = \'field1\'' ); @@ -2025,7 +1976,7 @@ public function testCanRemoveColumnComment() from pg_catalog.pg_class c where c.relname=cols.table_name ) as column_comment FROM information_schema.columns cols - WHERE cols.table_catalog=\'' . $this->config['name'] . '\' + WHERE cols.table_catalog=\'' . $this->config['database'] . '\' AND cols.table_name=\'table1\' AND cols.column_name = \'field1\'' ); @@ -2053,7 +2004,7 @@ public function testCanAddMultipleCommentsToOneTable() from pg_catalog.pg_class c where c.relname=cols.table_name ) as column_comment FROM information_schema.columns cols - WHERE cols.table_catalog=\'' . $this->config['name'] . '\' + WHERE cols.table_catalog=\'' . $this->config['database'] . '\' AND cols.table_name=\'table1\' AND cols.column_name = \'comment1\'' ); @@ -2066,7 +2017,7 @@ public function testCanAddMultipleCommentsToOneTable() from pg_catalog.pg_class c where c.relname=cols.table_name ) as column_comment FROM information_schema.columns cols - WHERE cols.table_catalog=\'' . $this->config['name'] . '\' + WHERE cols.table_catalog=\'' . $this->config['database'] . '\' AND cols.table_name=\'table1\' AND cols.column_name = \'comment2\'' ); @@ -2095,7 +2046,7 @@ public function testColumnsAreResetBetweenTables() from pg_catalog.pg_class c where c.relname=cols.table_name ) as column_comment FROM information_schema.columns cols - WHERE cols.table_catalog=\'' . $this->config['name'] . '\' + WHERE cols.table_catalog=\'' . $this->config['database'] . '\' AND cols.table_name=\'widgets\' AND cols.column_name = \'transport\'' ); @@ -2546,7 +2497,7 @@ public function testDumpInsert() $countQuery = $this->adapter->query('SELECT COUNT(*) FROM table1'); $this->assertTrue($countQuery->execute()); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(0, $res[0]['count']); } @@ -2598,7 +2549,7 @@ public function testDumpBulkinsert() $countQuery = $this->adapter->query('SELECT COUNT(*) FROM table1'); $this->assertTrue($countQuery->execute()); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(0, $res[0]['count']); } @@ -2637,29 +2588,6 @@ public function testDumpCreateTableAndThenInsert() $this->assertStringContainsString($expectedOutput, $actualOutput, 'Passing the --dry-run option does not dump create and then insert table queries to the output'); } - public function testDumpTransaction() - { - $inputDefinition = new InputDefinition([new InputOption('dry-run')]); - $this->adapter->setInput(new ArrayInput(['--dry-run' => true], $inputDefinition)); - - $consoleOutput = new BufferedOutput(); - $this->adapter->setOutput($consoleOutput); - - $this->adapter->beginTransaction(); - $table = new Table('schema1.table1', [], $this->adapter); - - $table->addColumn('column1', 'string') - ->addColumn('column2', 'integer') - ->addColumn('column3', 'string', ['default' => 'test']) - ->save(); - $this->adapter->commitTransaction(); - $this->adapter->rollbackTransaction(); - - $actualOutput = $consoleOutput->fetch(); - $this->assertStringStartsWith("BEGIN;\n", $actualOutput, 'Passing the --dry-run doesn\'t dump the transaction to the output'); - $this->assertStringEndsWith("COMMIT;\nROLLBACK;\n", $actualOutput, 'Passing the --dry-run doesn\'t dump the transaction to the output'); - } - /** * Tests interaction with the query builder */ @@ -2723,13 +2651,13 @@ public function testQueryWithParams() ]); $countQuery = $this->adapter->query('SELECT COUNT(*) AS c FROM table1 WHERE int_col > ?', [5]); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(2, $res[0]['c']); $this->adapter->execute('UPDATE table1 SET int_col = ? WHERE int_col IS NULL', [12]); $countQuery->execute([1]); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(3, $res[0]['c']); } @@ -2778,24 +2706,4 @@ public function testSerialAliases(string $columnType): void $this->assertSame($columnType, $column->getType()); $this->assertSame("nextval('test_id_seq'::regclass)", (string)$column->getDefault()); } - - public function testInvalidPdoAttribute() - { - $adapter = new PostgresAdapter($this->config + ['attr_invalid' => true]); - $this->expectException(UnexpectedValueException::class); - $this->expectExceptionMessage('Invalid PDO attribute: attr_invalid (\PDO::ATTR_INVALID)'); - $adapter->connect(); - } - - public function testPdoPersistentConnection() - { - $adapter = new PostgresAdapter($this->config + ['attr_persistent' => true]); - $this->assertTrue($adapter->getConnection()->getAttribute(PDO::ATTR_PERSISTENT)); - } - - public function testPdoNotPersistentConnection() - { - $adapter = new PostgresAdapter($this->config); - $this->assertFalse($adapter->getConnection()->getAttribute(PDO::ATTR_PERSISTENT)); - } } From 7218a00c32e67435d74b5152fe38e37d5e32a483 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 21 Feb 2024 23:55:33 -0500 Subject: [PATCH 04/10] Use prepared statements more --- src/Db/Adapter/MysqlAdapter.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Db/Adapter/MysqlAdapter.php b/src/Db/Adapter/MysqlAdapter.php index c8912e35..975de9f5 100644 --- a/src/Db/Adapter/MysqlAdapter.php +++ b/src/Db/Adapter/MysqlAdapter.php @@ -199,15 +199,15 @@ public function hasTable(string $tableName): bool */ protected function hasTableWithSchema(string $schema, string $tableName): bool { - $result = $this->fetchRow(sprintf( + $connection = $this->getConnection(); + $result = $connection->execute( "SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES - WHERE TABLE_SCHEMA = '%s' AND TABLE_NAME = '%s'", - $schema, - $tableName - )); + WHERE TABLE_SCHEMA = ? AND TABLE_NAME = ?", + [$schema, $tableName] + ); - return !empty($result); + return $result->rowCount() === 1; } /** From a135df945b7acf1a911b576b8f3761c0d5da43f0 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Thu, 22 Feb 2024 22:10:08 -0500 Subject: [PATCH 05/10] Update SQLServer Adapter to use Connection --- src/Db/Adapter/SqlserverAdapter.php | 108 ++-------------- .../Db/Adapter/SqlserverAdapterTest.php | 116 ++---------------- 2 files changed, 19 insertions(+), 205 deletions(-) diff --git a/src/Db/Adapter/SqlserverAdapter.php b/src/Db/Adapter/SqlserverAdapter.php index 4985738d..316d7a3c 100644 --- a/src/Db/Adapter/SqlserverAdapter.php +++ b/src/Db/Adapter/SqlserverAdapter.php @@ -60,102 +60,8 @@ class SqlserverAdapter extends PdoAdapter */ public function connect(): void { - if ($this->connection === null) { - if (!class_exists('PDO') || !in_array('sqlsrv', PDO::getAvailableDrivers(), true)) { - // try our connection via freetds (Mac/Linux) - $this->connectDblib(); - - return; - } - - $options = $this->getOptions(); - - $dsn = 'sqlsrv:server=' . $options['host']; - // if port is specified use it, otherwise use the SqlServer default - if (!empty($options['port'])) { - $dsn .= ',' . $options['port']; - } - $dsn .= ';database=' . $options['name'] . ';MultipleActiveResultSets=false'; - - // option to add additional connection options - // https://docs.microsoft.com/en-us/sql/connect/php/connection-options?view=sql-server-ver15 - if (isset($options['dsn_options'])) { - foreach ($options['dsn_options'] as $key => $option) { - $dsn .= ';' . $key . '=' . $option; - } - } - - $driverOptions = []; - - // charset support - if (isset($options['charset'])) { - $driverOptions[PDO::SQLSRV_ATTR_ENCODING] = $options['charset']; - } - - // use custom data fetch mode - if (!empty($options['fetch_mode'])) { - $driverOptions[PDO::ATTR_DEFAULT_FETCH_MODE] = constant('\PDO::FETCH_' . strtoupper($options['fetch_mode'])); - } - - // Note, the PDO::ATTR_PERSISTENT attribute is not supported for sqlsrv and will throw an error when used - // See https://github.com/Microsoft/msphpsql/issues/65 - - // support arbitrary \PDO::SQLSRV_ATTR_* driver options and pass them to PDO - // https://php.net/manual/en/ref.pdo-sqlsrv.php#pdo-sqlsrv.constants - foreach ($options as $key => $option) { - if (strpos($key, 'sqlsrv_attr_') === 0) { - $pdoConstant = '\PDO::' . strtoupper($key); - if (!defined($pdoConstant)) { - throw new UnexpectedValueException('Invalid PDO attribute: ' . $key . ' (' . $pdoConstant . ')'); - } - $driverOptions[constant($pdoConstant)] = $option; - } - } - - $db = $this->createPdoConnection($dsn, $options['user'] ?? null, $options['pass'] ?? null, $driverOptions); - - $this->setConnection($db); - } - } - - /** - * Connect to MSSQL using dblib/freetds. - * - * The "sqlsrv" driver is not available on Unix machines. - * - * @throws \InvalidArgumentException - * @throws \RuntimeException - * @return void - */ - protected function connectDblib(): void - { - if (!class_exists('PDO') || !in_array('dblib', PDO::getAvailableDrivers(), true)) { - // @codeCoverageIgnoreStart - throw new RuntimeException('You need to enable the PDO_Dblib extension for Migrations to run properly.'); - // @codeCoverageIgnoreEnd - } - - $options = $this->getOptions(); - - // if port is specified use it, otherwise use the SqlServer default - if (empty($options['port'])) { - $dsn = 'dblib:host=' . $options['host'] . ';dbname=' . $options['name']; - } else { - $dsn = 'dblib:host=' . $options['host'] . ':' . $options['port'] . ';dbname=' . $options['name']; - } - - $driverOptions = [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]; - - try { - $db = new PDO($dsn, $options['user'], $options['pass'], $driverOptions); - } catch (PDOException $exception) { - throw new InvalidArgumentException(sprintf( - 'There was a problem connecting to the database: %s', - $exception->getMessage() - ), 0, $exception); - } - - $this->setConnection($db); + $this->getConnection()->getDriver()->connect(); + $this->setConnection($this->getConnection()); } /** @@ -163,7 +69,7 @@ protected function connectDblib(): void */ public function disconnect(): void { - $this->connection = null; + $this->getConnection()->getDriver()->disconnect(); } /** @@ -179,7 +85,7 @@ public function hasTransactions(): bool */ public function beginTransaction(): void { - $this->execute('BEGIN TRANSACTION'); + $this->getConnection()->begin(); } /** @@ -187,7 +93,7 @@ public function beginTransaction(): void */ public function commitTransaction(): void { - $this->execute('COMMIT TRANSACTION'); + $this->getConnection()->commit(); } /** @@ -195,7 +101,7 @@ public function commitTransaction(): void */ public function rollbackTransaction(): void { - $this->execute('ROLLBACK TRANSACTION'); + $this->getConnection()->rollback(); } /** @@ -363,7 +269,7 @@ protected function getColumnCommentSqlDefinition(Column $column, ?string $tableN // passing 'null' is to remove column comment $currentComment = $this->getColumnComment((string)$tableName, $column->getName()); - $comment = strcasecmp((string)$column->getComment(), 'NULL') !== 0 ? $this->getConnection()->quote((string)$column->getComment()) : '\'\''; + $comment = strcasecmp((string)$column->getComment(), 'NULL') !== 0 ? $this->quoteString((string)$column->getComment()) : '\'\''; $command = $currentComment === null ? 'sp_addextendedproperty' : 'sp_updateextendedproperty'; return sprintf( diff --git a/tests/TestCase/Db/Adapter/SqlserverAdapterTest.php b/tests/TestCase/Db/Adapter/SqlserverAdapterTest.php index 13e46485..8a932665 100644 --- a/tests/TestCase/Db/Adapter/SqlserverAdapterTest.php +++ b/tests/TestCase/Db/Adapter/SqlserverAdapterTest.php @@ -4,6 +4,7 @@ namespace Migrations\Test\Db\Adapter; use BadMethodCallException; +use Cake\Database\Connection; use Cake\Database\Query; use Cake\Datasource\ConnectionManager; use InvalidArgumentException; @@ -42,18 +43,16 @@ protected function setUp(): void } // Emulate the results of Util::parseDsn() $this->config = [ - 'adapter' => $config['scheme'], - 'user' => $config['username'], - 'pass' => $config['password'], - 'host' => $config['host'], - 'name' => $config['database'], + 'adapter' => 'sqlserver', + 'connection' => ConnectionManager::get('test'), + 'database' => $config['database'], ]; $this->adapter = new SqlserverAdapter($this->config, new ArrayInput([]), new NullOutput()); // ensure the database is empty for each test - $this->adapter->dropDatabase($this->config['name']); - $this->adapter->createDatabase($this->config['name']); + $this->adapter->dropDatabase($this->config['database']); + $this->adapter->createDatabase($this->config['database']); // leave the adapter in a disconnected state for each test $this->adapter->disconnect(); @@ -69,56 +68,7 @@ protected function tearDown(): void public function testConnection() { - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - $this->assertSame(PDO::ERRMODE_EXCEPTION, $this->adapter->getConnection()->getAttribute(PDO::ATTR_ERRMODE)); - } - - public function testConnectionWithDsnOptions() - { - $options = $this->adapter->getOptions(); - $options['dsn_options'] = ['TrustServerCertificate' => 'true']; - $this->adapter->setOptions($options); - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - } - - public function testConnectionWithFetchMode() - { - $options = $this->adapter->getOptions(); - $options['fetch_mode'] = 'assoc'; - $this->adapter->setOptions($options); - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - $this->assertSame(PDO::FETCH_ASSOC, $this->adapter->getConnection()->getAttribute(PDO::ATTR_DEFAULT_FETCH_MODE)); - } - - public function testConnectionWithoutPort() - { - $options = $this->adapter->getOptions(); - unset($options['port']); - $this->adapter->setOptions($options); - $this->assertInstanceOf('PDO', $this->adapter->getConnection()); - } - - public function testConnectionWithInvalidCredentials() - { - $options = ['user' => 'invalid', 'pass' => 'invalid'] + $this->config; - - $adapter = null; - try { - $adapter = new SqlServerAdapter($options, new ArrayInput([]), new NullOutput()); - $adapter->connect(); - $this->fail('Expected the adapter to throw an exception'); - } catch (InvalidArgumentException $e) { - $this->assertInstanceOf( - 'InvalidArgumentException', - $e, - 'Expected exception of type InvalidArgumentException, got ' . get_class($e) - ); - $this->assertStringContainsString('There was a problem connecting to the database', $e->getMessage()); - } finally { - if (!empty($adapter)) { - $adapter->disconnect(); - } - } + $this->assertInstanceOf(Connection::class, $this->adapter->getConnection()); } public function testCreatingTheSchemaTableOnConnect() @@ -1047,8 +997,8 @@ public function testDropForeignKeyByName() public function testHasForeignKey($tableDef, $key, $exp) { $conn = $this->adapter->getConnection(); - $conn->exec('CREATE TABLE other(a int, b int, c int, unique(a), unique(b), unique(a,b), unique(a,b,c));'); - $conn->exec($tableDef); + $conn->execute('CREATE TABLE other(a int, b int, c int, unique(a), unique(b), unique(a,b), unique(a,b,c));'); + $conn->execute($tableDef); $this->assertSame($exp, $this->adapter->hasForeignKey('t', $key)); } @@ -1099,7 +1049,7 @@ public function testHasNamedForeignKey() public function testHasDatabase() { $this->assertFalse($this->adapter->hasDatabase('fake_database_name')); - $this->assertTrue($this->adapter->hasDatabase($this->config['name'])); + $this->assertTrue($this->adapter->hasDatabase($this->config['database'])); } public function testDropDatabase() @@ -1334,29 +1284,6 @@ public function testDumpCreateTableAndThenInsert() $this->assertStringContainsString($expectedOutput, $actualOutput, 'Passing the --dry-run option does not dump create and then insert table queries to the output'); } - public function testDumpTransaction() - { - $inputDefinition = new InputDefinition([new InputOption('dry-run')]); - $this->adapter->setInput(new ArrayInput(['--dry-run' => true], $inputDefinition)); - - $consoleOutput = new BufferedOutput(); - $this->adapter->setOutput($consoleOutput); - - $this->adapter->beginTransaction(); - $table = new Table('schema1.table1', [], $this->adapter); - - $table->addColumn('column1', 'string') - ->addColumn('column2', 'integer') - ->addColumn('column3', 'string', ['default' => 'test']) - ->save(); - $this->adapter->commitTransaction(); - $this->adapter->rollbackTransaction(); - - $actualOutput = str_replace("\r\n", "\n", $consoleOutput->fetch()); - $this->assertStringStartsWith("BEGIN TRANSACTION;\n", $actualOutput, 'Passing the --dry-run doesn\'t dump the transaction to the output'); - $this->assertStringEndsWith("COMMIT TRANSACTION;\nROLLBACK TRANSACTION;\n", $actualOutput, 'Passing the --dry-run doesn\'t dump the transaction to the output'); - } - /** * Tests interaction with the query builder */ @@ -1422,13 +1349,13 @@ public function testQueryWithParams() ]); $countQuery = $this->adapter->query('SELECT COUNT(*) AS c FROM table1 WHERE int_col > ?', [5]); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(2, $res[0]['c']); $this->adapter->execute('UPDATE table1 SET int_col = ? WHERE int_col IS NULL', [12]); $countQuery->execute([1]); - $res = $countQuery->fetchAll(); + $res = $countQuery->fetchAll('assoc'); $this->assertEquals(3, $res[0]['c']); } @@ -1443,23 +1370,4 @@ public function testLiteralSupport() $this->assertCount(1, $columns); $this->assertEquals(Literal::from('smallmoney'), array_pop($columns)->getType()); } - - public static function pdoAttributeProvider() - { - return [ - ['sqlsrv_attr_invalid'], - ['attr_invalid'], - ]; - } - - /** - * @dataProvider pdoAttributeProvider - */ - public function testInvalidPdoAttribute($attribute) - { - $adapter = new SqlServerAdapter($this->config + [$attribute => true]); - $this->expectException(UnexpectedValueException::class); - $this->expectExceptionMessage('Invalid PDO attribute: ' . $attribute . ' (\PDO::' . strtoupper($attribute) . ')'); - $adapter->connect(); - } } From e0646bbe5cc3c9496f8267757177e91ae7d8a117 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 23 Feb 2024 00:01:48 -0500 Subject: [PATCH 06/10] Fix failing tests --- src/Command/StatusCommand.php | 7 +-- tests/TestCase/Db/Adapter/PdoAdapterTest.php | 49 -------------------- tests/TestCase/Migration/EnvironmentTest.php | 2 +- tests/TestCase/Migration/ManagerTest.php | 8 +--- 4 files changed, 5 insertions(+), 61 deletions(-) diff --git a/src/Command/StatusCommand.php b/src/Command/StatusCommand.php index 95924ed6..b0ac0de7 100644 --- a/src/Command/StatusCommand.php +++ b/src/Command/StatusCommand.php @@ -132,12 +132,9 @@ protected function getConfig(Arguments $args): Config $adapter = $connectionConfig['scheme'] ?? null; $adapterConfig = [ 'adapter' => $adapter, - 'user' => $connectionConfig['username'], - 'pass' => $connectionConfig['password'], - 'host' => $connectionConfig['host'], - 'name' => $connectionConfig['database'], - 'migration_table' => $table, 'connection' => $connectionName, + 'database' => $connectionConfig['database'], + 'migration_table' => $table, ]; $configData = [ diff --git a/tests/TestCase/Db/Adapter/PdoAdapterTest.php b/tests/TestCase/Db/Adapter/PdoAdapterTest.php index ad8113e2..74d6c616 100644 --- a/tests/TestCase/Db/Adapter/PdoAdapterTest.php +++ b/tests/TestCase/Db/Adapter/PdoAdapterTest.php @@ -40,17 +40,6 @@ public function testOptionsSetSchemaTableName() $this->assertEquals('schema_table_test', $this->adapter->getSchemaTableName()); } - public function testOptionsSetDefaultMigrationTableThrowsDeprecation() - { - $this->markTestIncomplete('Deprecation assertions are not supported in PHPUnit anymore. We need to adopt the cakephp TestSuite class instead'); - $this->assertEquals('phinxlog', $this->adapter->getSchemaTableName()); - - $this->expectDeprecation(); - $this->expectExceptionMessage('The default_migration_table setting for adapter has been deprecated since 0.13.0. Use `migration_table` instead.'); - $this->adapter->setOptions(['default_migration_table' => 'schema_table_test']); - $this->assertEquals('schema_table_test', $this->adapter->getSchemaTableName()); - } - public function testSchemaTableName() { $this->assertEquals('phinxlog', $this->adapter->getSchemaTableName()); @@ -169,42 +158,4 @@ public function testGetVersionLongDryRun() $this->assertEquals([], $adapter->getVersionLog()); } - - /** - * Tests that execute() can be called on the adapter, and that the SQL is passed through to the PDO. - */ - public function testExecuteCanBeCalled() - { - /** @var \PDO&\PHPUnit\Framework\MockObject\MockObject $pdo */ - $pdo = $this->getMockBuilder(PDO::class)->disableOriginalConstructor()->onlyMethods(['exec'])->getMock(); - $pdo->expects($this->once())->method('exec')->with('SELECT 1;')->will($this->returnValue(1)); - - $this->adapter->setConnection($pdo); - $this->adapter->execute('SELECT 1'); - } - - public function testExecuteRightTrimsSemiColons() - { - /** @var \PDO&\PHPUnit\Framework\MockObject\MockObject $pdo */ - $pdo = $this->getMockBuilder(PDO::class)->disableOriginalConstructor()->onlyMethods(['exec'])->getMock(); - $pdo->expects($this->once())->method('exec')->with('SELECT 1;')->will($this->returnValue(1)); - - $this->adapter->setConnection($pdo); - $this->adapter->execute('SELECT 1;;'); - } - - public function testQueryBuilderMethods() - { - $result = $this->adapter->getSelectBuilder(); - $this->assertNotEmpty($result); - - $result = $this->adapter->getUpdateBuilder(); - $this->assertNotEmpty($result); - - $result = $this->adapter->getDeleteBuilder(); - $this->assertNotEmpty($result); - - $result = $this->adapter->getInsertBuilder(); - $this->assertNotEmpty($result); - } } diff --git a/tests/TestCase/Migration/EnvironmentTest.php b/tests/TestCase/Migration/EnvironmentTest.php index d9678226..27af0f87 100644 --- a/tests/TestCase/Migration/EnvironmentTest.php +++ b/tests/TestCase/Migration/EnvironmentTest.php @@ -77,7 +77,7 @@ public function testGetAdapter() $config = ConnectionManager::getConfig('test'); $environment = new Environment('default', [ 'connection' => 'test', - 'name' => $config['database'], + 'database' => $config['database'], 'migration_table' => 'phinxlog', ]); $adapter = $environment->getAdapter(); diff --git a/tests/TestCase/Migration/ManagerTest.php b/tests/TestCase/Migration/ManagerTest.php index 0ba2c991..6bfba0d5 100644 --- a/tests/TestCase/Migration/ManagerTest.php +++ b/tests/TestCase/Migration/ManagerTest.php @@ -78,13 +78,8 @@ public static function getConfigArray(): array $dbConfig = ConnectionManager::getConfig('test'); $config = [ 'connection' => 'test', + 'database' => $dbConfig['database'], 'migration_table' => 'phinxlog', - // TODO all of these should go away. - 'adapter' => $dbConfig['scheme'], - 'user' => $dbConfig['username'] ?? null, - 'pass' => $dbConfig['password'] ?? null, - 'host' => $dbConfig['host'], - 'name' => $dbConfig['database'], ]; return [ @@ -2639,6 +2634,7 @@ public function testMigrationWithDropColumnAndForeignKeyAndIndex(): void $adapter->dropDatabase($dbName); $adapter->createDatabase($dbName); $adapter->disconnect(); + $adapter->connect(); $this->manager->setConfig($config); $this->manager->migrate(20190928205056); From 18df5ff9e26488d36a0931ee6ad2a8915d4aed69 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 23 Feb 2024 00:15:01 -0500 Subject: [PATCH 07/10] Fix warnings --- src/View/Helper/MigrationHelper.php | 1 + tests/TestCase/TestSuite/MigratorTest.php | 11 ++++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/View/Helper/MigrationHelper.php b/src/View/Helper/MigrationHelper.php index e20830a6..9ba144b5 100644 --- a/src/View/Helper/MigrationHelper.php +++ b/src/View/Helper/MigrationHelper.php @@ -417,6 +417,7 @@ public function getColumnOption(array $options): array unset($columnOptions['collate']); } + // TODO this can be cleaned up when we stop using phinx data structures for column definitions if ($columnOptions['precision'] === null) { unset($columnOptions['precision']); } else { diff --git a/tests/TestCase/TestSuite/MigratorTest.php b/tests/TestCase/TestSuite/MigratorTest.php index 3f11eb22..631f7723 100644 --- a/tests/TestCase/TestSuite/MigratorTest.php +++ b/tests/TestCase/TestSuite/MigratorTest.php @@ -32,8 +32,10 @@ public function setUp(): void { parent::setUp(); - $this->restore = $GLOBALS['__PHPUNIT_BOOTSTRAP']; - unset($GLOBALS['__PHPUNIT_BOOTSTRAP']); + if (isset($GLOBALS['__PHPUNIT_BOOTSTRAP'])) { + $this->restore = $GLOBALS['__PHPUNIT_BOOTSTRAP']; + unset($GLOBALS['__PHPUNIT_BOOTSTRAP']); + } (new ConnectionHelper())->dropTables('test'); } @@ -41,7 +43,10 @@ public function setUp(): void public function tearDown(): void { parent::tearDown(); - $GLOBALS['__PHPUNIT_BOOTSTRAP'] = $this->restore; + if ($this->restore) { + $GLOBALS['__PHPUNIT_BOOTSTRAP'] = $this->restore; + unset($this->restore); + } (new ConnectionHelper())->dropTables('test'); } From 517856e7c58d0b711d8d98bb43e97af7a56f7739 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 23 Feb 2024 01:00:59 -0500 Subject: [PATCH 08/10] Try fixing postgres build --- tests/TestCase/Migration/ManagerTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/TestCase/Migration/ManagerTest.php b/tests/TestCase/Migration/ManagerTest.php index 6bfba0d5..6a7fb45a 100644 --- a/tests/TestCase/Migration/ManagerTest.php +++ b/tests/TestCase/Migration/ManagerTest.php @@ -2592,6 +2592,7 @@ public function testPostgresFullMigration(): void $adapter = $this->prepareEnvironment([ 'migrations' => ROOT . '/config/Postgres', ]); + $adapter->connect(); // migrate to the latest version $this->manager->migrate(); From 5f77fcec9985f406aa86e82f43e10cba0bd3dfc9 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 23 Feb 2024 01:01:48 -0500 Subject: [PATCH 09/10] fix phpcs, phpstan and psalm --- phpstan-baseline.neon | 12 +------- src/Db/Adapter/MysqlAdapter.php | 11 ++++---- src/Db/Adapter/PdoAdapter.php | 28 ------------------- src/Db/Adapter/PostgresAdapter.php | 4 --- src/Db/Adapter/SqliteAdapter.php | 3 -- src/Db/Adapter/SqlserverAdapter.php | 26 ----------------- .../TestCase/Db/Adapter/MysqlAdapterTest.php | 1 - tests/TestCase/Db/Adapter/PdoAdapterTest.php | 1 - .../Db/Adapter/PostgresAdapterTest.php | 1 - .../TestCase/Db/Adapter/SqliteAdapterTest.php | 2 -- .../Db/Adapter/SqlserverAdapterTest.php | 2 -- 11 files changed, 6 insertions(+), 85 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index eebf9dfe..fb6c23e7 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -25,18 +25,13 @@ parameters: count: 2 path: src/Db/Adapter/MysqlAdapter.php - - - message: "#^Parameter \\#4 \\$options of method Migrations\\\\Db\\\\Adapter\\\\PdoAdapter\\:\\:createPdoConnection\\(\\) expects array\\, array\\ given\\.$#" - count: 1 - path: src/Db/Adapter/MysqlAdapter.php - - message: "#^Right side of && is always true\\.$#" count: 1 path: src/Db/Adapter/MysqlAdapter.php - - message: "#^Access to an undefined property PDO\\:\\:\\$connection\\.$#" + message: "#^Access to an undefined property Cake\\\\Database\\\\Connection\\:\\:\\$connection\\.$#" count: 1 path: src/Db/Adapter/PdoAdapter.php @@ -60,11 +55,6 @@ parameters: count: 1 path: src/Db/Adapter/SqlserverAdapter.php - - - message: "#^Parameter \\#4 \\$options of method Migrations\\\\Db\\\\Adapter\\\\PdoAdapter\\:\\:createPdoConnection\\(\\) expects array\\, array\\ given\\.$#" - count: 1 - path: src/Db/Adapter/SqlserverAdapter.php - - message: "#^Ternary operator condition is always true\\.$#" count: 2 diff --git a/src/Db/Adapter/MysqlAdapter.php b/src/Db/Adapter/MysqlAdapter.php index 975de9f5..6f69b365 100644 --- a/src/Db/Adapter/MysqlAdapter.php +++ b/src/Db/Adapter/MysqlAdapter.php @@ -9,7 +9,6 @@ namespace Migrations\Db\Adapter; use Cake\Database\Connection; -use Cake\Database\Driver\Mysql as MysqlDriver; use InvalidArgumentException; use Migrations\Db\AlterInstructions; use Migrations\Db\Literal; @@ -17,10 +16,7 @@ use Migrations\Db\Table\ForeignKey; use Migrations\Db\Table\Index; use Migrations\Db\Table\Table; -use PDO; use Phinx\Config\FeatureFlags; -use RuntimeException; -use UnexpectedValueException; /** * Phinx MySQL Adapter. @@ -106,9 +102,12 @@ public function connect(): void $this->setConnection($this->getConnection()); } + /** + * @inheritDoc + */ public function setConnection(Connection $connection): AdapterInterface { - $connection->execute(sprintf("USE %s", $this->getOption('database'))); + $connection->execute(sprintf('USE %s', $this->getOption('database'))); return parent::setConnection($connection); } @@ -1250,7 +1249,7 @@ public function createDatabase(string $name, array $options = []): void } else { $this->execute(sprintf('CREATE DATABASE `%s` DEFAULT CHARACTER SET `%s`', $name, $charset)); } - $this->execute(sprintf("USE %s", $name)); + $this->execute(sprintf('USE %s', $name)); } /** diff --git a/src/Db/Adapter/PdoAdapter.php b/src/Db/Adapter/PdoAdapter.php index 17cf2a91..52058936 100644 --- a/src/Db/Adapter/PdoAdapter.php +++ b/src/Db/Adapter/PdoAdapter.php @@ -209,22 +209,6 @@ public function execute(string $sql, array $params = []): int return $stmt->rowCount(); } - /** - * Build connection instance. - * - * @param class-string<\Cake\Database\Driver> $driverClass Driver class name. - * @param array $options Options. - * @return \Cake\Database\Connection - */ - protected function buildConnection(string $driverClass, array $options): Connection - { - $driver = new $driverClass($options); - $prop = new ReflectionProperty($driver, 'pdo'); - $prop->setValue($driver, $this->connection); - - return new Connection(['driver' => $driver] + $options); - } - /** * @inheritDoc */ @@ -639,18 +623,6 @@ public function castToBool($value): mixed return (bool)$value ? 1 : 0; } - /** - * Retrieve a database connection attribute - * - * @see https://php.net/manual/en/pdo.getattribute.php - * @param int $attribute One of the PDO::ATTR_* constants - * @return mixed - */ - public function getAttribute(int $attribute): mixed - { - return $this->getConnection()->getAttribute($attribute); - } - /** * Get the definition for a `DEFAULT` statement. * diff --git a/src/Db/Adapter/PostgresAdapter.php b/src/Db/Adapter/PostgresAdapter.php index 3a1cf844..1ffc0016 100644 --- a/src/Db/Adapter/PostgresAdapter.php +++ b/src/Db/Adapter/PostgresAdapter.php @@ -9,7 +9,6 @@ namespace Migrations\Db\Adapter; use Cake\Database\Connection; -use Cake\Database\Driver\Postgres as PostgresDriver; use InvalidArgumentException; use Migrations\Db\AlterInstructions; use Migrations\Db\Literal; @@ -17,9 +16,6 @@ use Migrations\Db\Table\ForeignKey; use Migrations\Db\Table\Index; use Migrations\Db\Table\Table; -use PDO; -use PDOException; -use RuntimeException; class PostgresAdapter extends PdoAdapter { diff --git a/src/Db/Adapter/SqliteAdapter.php b/src/Db/Adapter/SqliteAdapter.php index a0576b0c..acc581f8 100644 --- a/src/Db/Adapter/SqliteAdapter.php +++ b/src/Db/Adapter/SqliteAdapter.php @@ -9,8 +9,6 @@ namespace Migrations\Db\Adapter; use BadMethodCallException; -use Cake\Database\Connection; -use Cake\Database\Driver\Sqlite as SqliteDriver; use InvalidArgumentException; use Migrations\Db\AlterInstructions; use Migrations\Db\Expression; @@ -19,7 +17,6 @@ use Migrations\Db\Table\ForeignKey; use Migrations\Db\Table\Index; use Migrations\Db\Table\Table; -use PDO; use PDOException; use RuntimeException; use const FILTER_VALIDATE_BOOLEAN; diff --git a/src/Db/Adapter/SqlserverAdapter.php b/src/Db/Adapter/SqlserverAdapter.php index 316d7a3c..f3667c3e 100644 --- a/src/Db/Adapter/SqlserverAdapter.php +++ b/src/Db/Adapter/SqlserverAdapter.php @@ -9,8 +9,6 @@ namespace Migrations\Db\Adapter; use BadMethodCallException; -use Cake\Database\Connection; -use Cake\Database\Driver\Sqlserver as SqlServerDriver; use InvalidArgumentException; use Migrations\Db\AlterInstructions; use Migrations\Db\Literal; @@ -18,11 +16,7 @@ use Migrations\Db\Table\ForeignKey; use Migrations\Db\Table\Index; use Migrations\Db\Table\Table; -use PDO; -use PDOException; use Phinx\Migration\MigrationInterface; -use RuntimeException; -use UnexpectedValueException; /** * Migrations SqlServer Adapter. @@ -1257,24 +1251,4 @@ public function migrated(MigrationInterface $migration, string $direction, strin return parent::migrated($migration, $direction, $startTime, $endTime); } - - /** - * @inheritDoc - */ - public function getDecoratedConnection(): Connection - { - if (isset($this->decoratedConnection)) { - return $this->decoratedConnection; - } - - $options = $this->getOptions(); - $options = [ - 'username' => $options['user'] ?? null, - 'password' => $options['pass'] ?? null, - 'database' => $options['name'], - 'quoteIdentifiers' => true, - ] + $options; - - return $this->decoratedConnection = $this->buildConnection(SqlServerDriver::class, $options); - } } diff --git a/tests/TestCase/Db/Adapter/MysqlAdapterTest.php b/tests/TestCase/Db/Adapter/MysqlAdapterTest.php index 469c036f..9252a65e 100644 --- a/tests/TestCase/Db/Adapter/MysqlAdapterTest.php +++ b/tests/TestCase/Db/Adapter/MysqlAdapterTest.php @@ -22,7 +22,6 @@ use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\BufferedOutput; use Symfony\Component\Console\Output\NullOutput; -use UnexpectedValueException; class MysqlAdapterTest extends TestCase { diff --git a/tests/TestCase/Db/Adapter/PdoAdapterTest.php b/tests/TestCase/Db/Adapter/PdoAdapterTest.php index 74d6c616..cb3bfdd5 100644 --- a/tests/TestCase/Db/Adapter/PdoAdapterTest.php +++ b/tests/TestCase/Db/Adapter/PdoAdapterTest.php @@ -3,7 +3,6 @@ namespace Migrations\Test\Db\Adapter; -use PDO; use PDOException; use Phinx\Config\Config; use PHPUnit\Framework\TestCase; diff --git a/tests/TestCase/Db/Adapter/PostgresAdapterTest.php b/tests/TestCase/Db/Adapter/PostgresAdapterTest.php index 5138d295..3121377e 100644 --- a/tests/TestCase/Db/Adapter/PostgresAdapterTest.php +++ b/tests/TestCase/Db/Adapter/PostgresAdapterTest.php @@ -21,7 +21,6 @@ use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\BufferedOutput; use Symfony\Component\Console\Output\NullOutput; -use UnexpectedValueException; class PostgresAdapterTest extends TestCase { diff --git a/tests/TestCase/Db/Adapter/SqliteAdapterTest.php b/tests/TestCase/Db/Adapter/SqliteAdapterTest.php index 724cb55b..65b25076 100644 --- a/tests/TestCase/Db/Adapter/SqliteAdapterTest.php +++ b/tests/TestCase/Db/Adapter/SqliteAdapterTest.php @@ -16,7 +16,6 @@ use Migrations\Db\Table; use Migrations\Db\Table\Column; use Migrations\Db\Table\ForeignKey; -use PDO; use PDOException; use PHPUnit\Framework\TestCase; use ReflectionObject; @@ -26,7 +25,6 @@ use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\BufferedOutput; use Symfony\Component\Console\Output\NullOutput; -use UnexpectedValueException; class SqliteAdapterTest extends TestCase { diff --git a/tests/TestCase/Db/Adapter/SqlserverAdapterTest.php b/tests/TestCase/Db/Adapter/SqlserverAdapterTest.php index 8a932665..792db596 100644 --- a/tests/TestCase/Db/Adapter/SqlserverAdapterTest.php +++ b/tests/TestCase/Db/Adapter/SqlserverAdapterTest.php @@ -13,7 +13,6 @@ use Migrations\Db\Table; use Migrations\Db\Table\Column; use Migrations\Db\Table\ForeignKey; -use PDO; use PHPUnit\Framework\TestCase; use RuntimeException; use Symfony\Component\Console\Input\ArrayInput; @@ -21,7 +20,6 @@ use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\BufferedOutput; use Symfony\Component\Console\Output\NullOutput; -use UnexpectedValueException; class SqlserverAdapterTest extends TestCase { From 17c87a9568ace26c5c7ec10f665f8162e9041ead Mon Sep 17 00:00:00 2001 From: Mark Story Date: Sun, 25 Feb 2024 01:34:51 -0500 Subject: [PATCH 10/10] Fix tests with sqlite --- .../TestCase/Db/Adapter/PhinxAdapterTest.php | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/tests/TestCase/Db/Adapter/PhinxAdapterTest.php b/tests/TestCase/Db/Adapter/PhinxAdapterTest.php index 3cea1efd..c648bb96 100644 --- a/tests/TestCase/Db/Adapter/PhinxAdapterTest.php +++ b/tests/TestCase/Db/Adapter/PhinxAdapterTest.php @@ -40,15 +40,15 @@ class PhinxAdapterTest extends TestCase protected function setUp(): void { $config = ConnectionManager::getConfig('test'); + if ($config['adapter'] !== 'sqlite') { + $this->markTestSkipped('phinx adapter tests require sqlite'); + } // Emulate the results of Util::parseDsn() $this->config = [ - 'adapter' => $config['scheme'], - 'host' => $config['host'], - 'name' => $config['database'], + 'adapter' => 'sqlite', + 'connection' => ConnectionManager::get('test'), + 'database' => $config['database'], ]; - if ($this->config['adapter'] !== 'sqlite') { - $this->markTestSkipped('phinx adapter tests require sqlite'); - } $this->adapter = new PhinxAdapter( new SqliteAdapter( $this->config, @@ -57,10 +57,10 @@ protected function setUp(): void ) ); - if ($this->config['name'] !== ':memory:') { + if ($this->config['database'] !== ':memory:') { // ensure the database is empty for each test - $this->adapter->dropDatabase($this->config['name']); - $this->adapter->createDatabase($this->config['name']); + $this->adapter->dropDatabase($this->config['database']); + $this->adapter->createDatabase($this->config['database']); } // leave the adapter in a disconnected state for each test @@ -80,12 +80,11 @@ public function testBeginTransaction() $this->adapter->getConnection()->inTransaction(), 'Underlying PDO instance did not detect new transaction' ); + $this->adapter->rollbackTransaction(); } public function testRollbackTransaction() { - $this->adapter->getConnection() - ->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); $this->adapter->beginTransaction(); $this->adapter->rollbackTransaction();