From 8d960d4409753e42ba5758e327f77c03c934f0c1 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Fri, 9 Oct 2026 10:10:19 -0400 Subject: [PATCH 1/2] Report zero-row statements as successful in Connection::safeQuery() safeQuery() ended with `return $result ?: null;`, so a statement that succeeded without changing rows (every CREATE TABLE, DROP TABLE, an UPDATE or DELETE matching nothing) returned null, the failure value. Callers that test the result treated those statements as failed: module install SQL, profile field ALTER TABLE, Xmf\Database\Tables. It now returns true in that case. Affected-row counts above zero and SELECT results are returned unchanged, and null still means failure. - XoopsDatabaseManager::deleteTables() listed a table as dropped when its DROP failed; the old null result hid the inverted condition - XoopsMySQLDatabase::getAffectedRows() no longer calls rowCount() on a non-Result value - Tests run the statements against in-memory SQLite --- htdocs/class/database/manager.php | 3 +- htdocs/class/database/mysqldatabase.php | 7 +- tests/unit/class/database/managerTest.php | 23 ++++++ .../unit/class/database/mysqldatabaseTest.php | 22 ++++++ .../Xmf/Database/TablesSchemaTest.php | 8 +- .../Core/Database/ConnectionSafeQueryTest.php | 76 +++++++++++++++++++ xoops_lib/Xoops/Core/Database/Connection.php | 14 +++- 7 files changed, 141 insertions(+), 12 deletions(-) create mode 100644 tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php diff --git a/htdocs/class/database/manager.php b/htdocs/class/database/manager.php index 452a01c08..6715113fb 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 4e52f1e10..63edb5414 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 bec8b0d36..6665fb567 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 test_deleteTablesReturnsOnlyDroppedTables() + { + $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 a13b33833..3fba2068e 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 test_getAffectedRowsFollowsConnectionQuery() + { + $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 35fc3dd0d..322d63405 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 000000000..da0ecd7b2 --- /dev/null +++ b/tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php @@ -0,0 +1,76 @@ +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 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 df090134a..980338863 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; } /** From e25c6fc8ab3254e9f0c5d9e7402d927f14befbbe Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Fri, 9 Oct 2026 10:52:01 -0400 Subject: [PATCH 2/2] Test Tables::executeQueue() against a real Connection The TablesSchemaTest fake returns true for zero-row statements itself, so it cannot catch a regression in Connection::safeQuery(). Run a CREATE TABLE through executeQueue() on an in-memory SQLite Connection instead, and use camelCase names for the new test methods. --- tests/unit/class/database/managerTest.php | 2 +- .../unit/class/database/mysqldatabaseTest.php | 2 +- .../Core/Database/ConnectionSafeQueryTest.php | 26 +++++++++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/tests/unit/class/database/managerTest.php b/tests/unit/class/database/managerTest.php index 6665fb567..1ff4771cc 100644 --- a/tests/unit/class/database/managerTest.php +++ b/tests/unit/class/database/managerTest.php @@ -26,7 +26,7 @@ public function test___publicProperties() } } - public function test_deleteTablesReturnsOnlyDroppedTables() + 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 diff --git a/tests/unit/class/database/mysqldatabaseTest.php b/tests/unit/class/database/mysqldatabaseTest.php index 3fba2068e..6d99e4ab0 100644 --- a/tests/unit/class/database/mysqldatabaseTest.php +++ b/tests/unit/class/database/mysqldatabaseTest.php @@ -34,7 +34,7 @@ public function test_genId() } #[\PHPUnit\Framework\Attributes\RequiresPhpExtension('pdo_sqlite')] - public function test_getAffectedRowsFollowsConnectionQuery() + public function testGetAffectedRowsFollowsConnectionQuery() { $instance = new $this->myclass(); $instance->conn = new \Xoops\Core\Database\Connection( diff --git a/tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php b/tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php index da0ecd7b2..8e57187a7 100644 --- a/tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php +++ b/tests/unit/xoopsLib/Xoops/Core/Database/ConnectionSafeQueryTest.php @@ -61,6 +61,32 @@ public function testFailedStatementReturnsNull(): void $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)');