diff --git a/htdocs/class/database/manager.php b/htdocs/class/database/manager.php index 452a01c0..6715113f 100644 --- a/htdocs/class/database/manager.php +++ b/htdocs/class/database/manager.php @@ -329,8 +329,7 @@ public function deleteTables($tables) $deleted = array(); // DBAL 4: connection is established lazily on first query foreach ($tables as $key => $val) { - //was: if (!$this->db->query("DROP TABLE " . $this->db->prefix($key))) { - if (!$this->db->query("DROP TABLE " . $this->db->prefix($val))) { + if ($this->db->query("DROP TABLE " . $this->db->prefix($val))) { $deleted[] = $val; } } diff --git a/htdocs/class/database/mysqldatabase.php b/htdocs/class/database/mysqldatabase.php index 4e52f1e1..63edb541 100644 --- a/htdocs/class/database/mysqldatabase.php +++ b/htdocs/class/database/mysqldatabase.php @@ -223,7 +223,12 @@ public function getRowsNum($result) public function getAffectedRows() { $this->deprecated(); - return $this->lastResult->rowCount(); + $result = $this->lastResult; + if ($result instanceof \Doctrine\DBAL\Result) { + return $result->rowCount(); + } + // Connection::query() returns the count for writes, or true when no rows changed + return is_numeric($result) ? (int) $result : 0; } /** diff --git a/tests/unit/class/database/managerTest.php b/tests/unit/class/database/managerTest.php index bec8b0d3..1ff4771c 100644 --- a/tests/unit/class/database/managerTest.php +++ b/tests/unit/class/database/managerTest.php @@ -25,4 +25,27 @@ public function test___publicProperties() $this->assertTrue($prop->isPublic()); } } + + public function testDeleteTablesReturnsOnlyDroppedTables() + { + $manager = (new ReflectionClass($this->myclass))->newInstanceWithoutConstructor(); + // DROP TABLE succeeds (true, no rows affected) for one table and fails (null) for the other + $manager->db = new class { + public $dropped = array(); + + public function prefix($table) + { + return 'x_' . $table; + } + + public function query($sql) + { + $this->dropped[] = $sql; + return (false !== strpos($sql, 'x_missing')) ? null : true; + } + }; + + $this->assertSame(array('items'), $manager->deleteTables(array('items', 'missing'))); + $this->assertSame(array('DROP TABLE x_items', 'DROP TABLE x_missing'), $manager->db->dropped); + } } diff --git a/tests/unit/class/database/mysqldatabaseTest.php b/tests/unit/class/database/mysqldatabaseTest.php index a13b3383..6d99e4ab 100644 --- a/tests/unit/class/database/mysqldatabaseTest.php +++ b/tests/unit/class/database/mysqldatabaseTest.php @@ -32,4 +32,26 @@ public function test_genId() $x = $instance->genId($sequence); $this->assertSame(0, $x); } + + #[\PHPUnit\Framework\Attributes\RequiresPhpExtension('pdo_sqlite')] + public function testGetAffectedRowsFollowsConnectionQuery() + { + $instance = new $this->myclass(); + $instance->conn = new \Xoops\Core\Database\Connection( + array('driver' => 'pdo_sqlite', 'memory' => true), + new \Doctrine\DBAL\Driver\PDO\SQLite\Driver(), + new Configuration() + ); + $instance->conn->setSafe(true); + + // DDL changes no rows: the query succeeds and reports 0 affected rows + $this->assertTrue($instance->queryF('CREATE TABLE items (id INTEGER)')); + $this->assertSame(0, $instance->getAffectedRows()); + + $this->assertSame(2, $instance->queryF('INSERT INTO items VALUES (1), (2)')); + $this->assertSame(2, $instance->getAffectedRows()); + + $this->assertFalse($instance->queryF('INSERT INTO missing VALUES (1)')); + $this->assertSame(0, $instance->getAffectedRows()); + } } diff --git a/tests/unit/xoopsLib/Xmf/Database/TablesSchemaTest.php b/tests/unit/xoopsLib/Xmf/Database/TablesSchemaTest.php index 35fc3dd0..322d6340 100644 --- a/tests/unit/xoopsLib/Xmf/Database/TablesSchemaTest.php +++ b/tests/unit/xoopsLib/Xmf/Database/TablesSchemaTest.php @@ -132,10 +132,6 @@ public function testExecuteQueueRendersCreateTableAndRunsItForced(): void public function testExecuteQueueAcceptsDdlThatAffectsNoRows(): void { - $this->markTestIncomplete( - 'Existing 2.6 bug: Connection::safeQuery() returns null when a statement affects 0 rows, ' - . 'which every CREATE TABLE does, so Tables::executeQueue() reports a successful CREATE as failed.' - ); $tables = $this->queueCreateTable(self::itemsSchema()); $this->assertTrue($tables->executeQueue(true)); @@ -188,12 +184,12 @@ public function query($sql) { $this->queries[] = $sql; // like Connection::safeQuery(): force lasts for one statement, and a - // non-SELECT returns its affected-row count, or null when that is 0 + // non-SELECT returns its affected-row count, or true when that is 0 $this->force = false; if ($this->fail) { $result = false; } elseif (stripos(ltrim($sql), 'select') !== 0) { - $result = $this->affectedRows ?: null; + $result = $this->affectedRows ?: true; } else { // only x_items exists; every other table comes back empty $rows = []; diff --git a/tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php b/tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php new file mode 100644 index 00000000..8e57187a --- /dev/null +++ b/tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php @@ -0,0 +1,102 @@ +db = new Connection(['driver' => 'pdo_sqlite', 'memory' => true], new Driver(), new Configuration()); + $this->db->setSafe(true); + } + + public function testDdlThatChangesNoRowsReportsSuccess(): void + { + $this->assertTrue($this->db->query('CREATE TABLE items (id INTEGER, title TEXT)')); + } + + public function testWriteReturnsAffectedRowCount(): void + { + $this->db->query('CREATE TABLE items (id INTEGER, title TEXT)'); + + $this->assertSame(2, $this->db->query("INSERT INTO items VALUES (1, 'a'), (2, 'b')")); + $this->assertSame(1, $this->db->query("UPDATE items SET title = 'c' WHERE id = 2")); + } + + public function testWriteMatchingNoRowsReportsSuccess(): void + { + $this->db->query('CREATE TABLE items (id INTEGER, title TEXT)'); + + $this->assertTrue($this->db->query("UPDATE items SET title = 'x' WHERE id = 99")); + $this->assertTrue($this->db->safeQuery('DELETE FROM items')); + } + + public function testSelectReturnsResult(): void + { + $this->db->query('CREATE TABLE items (id INTEGER)'); + + $result = $this->db->query('SELECT * FROM items'); + + $this->assertInstanceOf(Result::class, $result); + $this->assertFalse($result->fetchAssociative()); + } + + public function testFailedStatementReturnsNull(): void + { + $this->assertNull($this->db->query('CREATE TABLE broken (')); + $this->assertNull($this->db->query('SELECT * FROM missing_table')); + } + + public function testTablesExecuteQueueCreatesTableThroughConnection(): void + { + // SQLite has no INFORMATION_SCHEMA, so seed the definition addTable() would have built + $tables = (new \ReflectionClass(\Xmf\Database\Tables::class))->newInstanceWithoutConstructor(); + $state = [ + 'db' => $this->db, + 'tables' => ['fresh' => [ + 'name' => 'x_fresh', + 'options' => '', + 'columns' => [['name' => 'id', 'attributes' => 'INTEGER NOT NULL']], + 'keys' => ['PRIMARY' => ['columns' => '`id`', 'unique' => true]], + 'create' => true, + ]], + 'queue' => [['createtable' => 'fresh']], + ]; + foreach ($state as $name => $value) { + (new \ReflectionProperty(\Xmf\Database\Tables::class, $name))->setValue($tables, $value); + } + + $this->assertTrue($tables->executeQueue(true)); + $this->assertSame( + 'x_fresh', + $this->db->query("SELECT name FROM sqlite_master WHERE name = 'x_fresh'")->fetchOne() + ); + } + + public function testWriteIsRefusedWhenNotSafe(): void + { + $this->db->query('CREATE TABLE items (id INTEGER)'); + $this->db->setSafe(false); + + $this->assertNull($this->db->query('INSERT INTO items VALUES (1)')); + $this->assertInstanceOf(Result::class, $this->db->query('SELECT * FROM items')); + + $this->db->setForce(true); + $this->assertSame(1, $this->db->query('INSERT INTO items VALUES (1)')); + $this->assertFalse($this->db->getForce(), 'force applies to one statement'); + } +} diff --git a/xoops_lib/Xoops/Core/Database/Connection.php b/xoops_lib/Xoops/Core/Database/Connection.php index df090134..98033886 100644 --- a/xoops_lib/Xoops/Core/Database/Connection.php +++ b/xoops_lib/Xoops/Core/Database/Connection.php @@ -438,7 +438,9 @@ public function rollBack(): void * @param array $params The query parameters * @param array $types The parameter types * - * @return Result|int|string|null Result for SELECT, affected rows for DML, null on failure + * @return Result|int|string|true|null Result for SELECT; the affected row count for other + * statements, or true when one succeeded without changing + * rows (CREATE TABLE, an UPDATE matching nothing); null on failure */ public function query(string $sql, array $params = [], array $types = []) { @@ -455,7 +457,9 @@ public function query(string $sql, array $params = [], array $types = []) * @param array $params The query parameters * @param array $types The parameter types * - * @return Result|int|string|null Result for SELECT, affected rows for DML, null on failure + * @return Result|int|string|true|null Result for SELECT; the affected row count for other + * statements, or true when one succeeded without changing + * rows (CREATE TABLE, an UPDATE matching nothing); null on failure */ public function safeQuery(string $sql, array $params = [], array $types = []) { @@ -481,7 +485,11 @@ public function safeQuery(string $sql, array $params = [], array $types = []) $result = null; } $events->triggerEvent('core.database.query.end'); - return $result ?: null; + // 0 affected rows is still a success; null must mean the statement failed + if ($result === 0 || $result === '0') { + return true; + } + return $result; } /**