diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index 5c07565344f9..4e5f1f07bf26 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -2385,6 +2385,8 @@ public function countAllResults(bool $reset = true) if ($reset) { $this->resetSelect(); + $this->binds = []; + $this->bindsKeyCount = []; } elseif (! isset($this->QBOrderBy)) { $this->QBOrderBy = $orderBy; } @@ -3485,7 +3487,18 @@ public function delete($where = '', ?int $limit = null, bool $resetData = true) $this->resetWrite(); } - return $this->testMode ? $sql : $this->db->query($sql, $this->binds, false); + if ($this->testMode) { + return $sql; + } + + $result = $this->db->query($sql, $this->binds, false); + + if ($resetData) { + $this->binds = []; + $this->bindsKeyCount = []; + } + + return $result; } /** diff --git a/system/Database/SQLSRV/Builder.php b/system/Database/SQLSRV/Builder.php index f51f0d266326..4f61f5abdcf6 100644 --- a/system/Database/SQLSRV/Builder.php +++ b/system/Database/SQLSRV/Builder.php @@ -582,7 +582,18 @@ public function delete($where = '', ?int $limit = null, bool $resetData = true) $this->resetWrite(); } - return $this->testMode ? $sql : $this->db->query($sql, $this->binds, false); + if ($this->testMode) { + return $sql; + } + + $result = $this->db->query($sql, $this->binds, false); + + if ($resetData) { + $this->binds = []; + $this->bindsKeyCount = []; + } + + return $result; } /** diff --git a/tests/system/Database/Builder/ExecutionBindingsTest.php b/tests/system/Database/Builder/ExecutionBindingsTest.php new file mode 100644 index 000000000000..42ad7810c735 --- /dev/null +++ b/tests/system/Database/Builder/ExecutionBindingsTest.php @@ -0,0 +1,186 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Database\Builder; + +use CodeIgniter\Database\BaseBuilder; +use CodeIgniter\Database\Query; +use CodeIgniter\Database\SQLSRV\Builder as SQLSRVBuilder; +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\Mock\MockConnection; +use CodeIgniter\Test\Mock\MockResult; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\MockObject\MockObject; + +/** + * @internal + */ +#[Group('Others')] +final class ExecutionBindingsTest extends CIUnitTestCase +{ + #[DataProvider('provideExecution')] + public function testExecutionClearsBindingsOnReset(string $method): void + { + $db = $this->connection(); + $builder = new BaseBuilder('jobs', $db); + + for ($id = 1; $id <= 3; $id++) { + $result = $builder->where('id >', $id)->where('id <', $id + 10)->{$method}(); + + $this->assertSame($method === 'delete' ? true : 0, $result); + $this->assertStringContainsString('"id" > ' . $id, $db->lastQuery->getQuery()); + $this->assertSame([], $builder->getBinds()); + $this->assertSame([], $this->getPrivateProperty($builder, 'bindsKeyCount')); + } + } + + #[DataProvider('provideExecution')] + public function testExecutionPreservesBindingsWithoutReset(string $method): void + { + $db = $this->connection(); + $builder = new BaseBuilder('jobs', $db); + $builder->where('id', 3); + + for ($i = 0; $i < 2; $i++) { + if ($method === 'delete') { + $builder->delete('', null, false); + } else { + $builder->countAllResults(false); + } + + $this->assertSame(['id' => [3, true]], $builder->getBinds()); + $this->assertStringContainsString('"id" = 3', $db->lastQuery->getQuery()); + } + + $builder->{$method}(); + $this->assertSame([], $builder->getBinds()); + } + + #[DataProvider('provideExecution')] + public function testFailedExecutionClearsBindingsOnReset(string $method): void + { + $db = $this->connection(false); + $builder = new BaseBuilder('jobs', $db); + $builder->where('id', 3); + + $this->assertSame($method === 'delete' ? false : 0, $builder->{$method}()); + $this->assertSame([], $builder->getBinds()); + $this->assertSameSql('SELECT * FROM "jobs"', $builder->getCompiledSelect(false)); + } + + #[DataProvider('provideExecution')] + public function testTestModeRetainsBindingsForReturnedPlaceholders(string $method): void + { + $builder = new BaseBuilder('jobs', $this->connection()); + $builder->where('id', 3)->testMode(); + + $sql = $builder->{$method}(); + + $this->assertIsString($sql); + $this->assertStringContainsString(':id:', $sql); + $this->assertSame(['id' => [3, true]], $builder->getBinds()); + } + + #[DataProvider('provideExecution')] + public function testExecutionWithSubqueryPreservesInnerBindings(string $method): void + { + $db = $this->connection(); + $subquery = (new BaseBuilder('user_jobs', $db))->select('job_id')->where('id', 7); + $builder = (new BaseBuilder('jobs', $db))->whereIn('id', $subquery); + + if ($method === 'countAllResults') { + $builder->groupBy('id'); + } + + $builder->{$method}(); + + $this->assertStringContainsString('IN (SELECT "job_id" FROM "user_jobs" WHERE "id" = 7)', str_replace("\n", ' ', $db->lastQuery->getQuery())); + $this->assertSame([], $builder->getBinds()); + $this->assertSame(['id' => [7, true]], $subquery->getBinds()); + + if ($method === 'countAllResults') { + $this->assertStringContainsString('CI_count_all_results', $db->lastQuery->getQuery()); + } + } + + /** + * @return iterable + */ + public static function provideExecution(): iterable + { + yield 'delete' => ['delete']; + + yield 'count' => ['countAllResults']; + } + + public function testCompiledDeleteKeepsBindingsUntilFinalCompilation(): void + { + $builder = new BaseBuilder('jobs', $this->connection()); + $builder->where('id', 3); + + $this->assertSameSql('DELETE FROM "jobs" WHERE "id" = 3', $builder->getCompiledDelete(false)); + $this->assertSame(['id' => [3, true]], $builder->getBinds()); + $this->assertSameSql('DELETE FROM "jobs" WHERE "id" = 3', $builder->getCompiledDelete()); + $this->assertSame([], $builder->getBinds()); + } + + public function testSQLSRVDeleteClearsBindingsAfterExecution(): void + { + foreach ([true, false] as $success) { + $db = $this->connection($success); + $builder = new SQLSRVBuilder('jobs', $db); + + for ($id = 1; $id <= 3; $id++) { + $this->assertSame($success, $builder->where('id', $id)->delete()); + $this->assertSame([], $builder->getBinds()); + $this->assertStringContainsString('"id" = ' . $id, $db->lastQuery->getQuery()); + } + } + } + + public function testSQLSRVDeletePreservesBindingsWhenNeeded(): void + { + $builder = new SQLSRVBuilder('jobs', $this->connection()); + $builder->where('id', 3)->delete('', null, false); + + $this->assertSame(['id' => [3, true]], $builder->getBinds()); + $sql = $builder->getCompiledDelete(false); + $this->assertStringContainsString('"id" = 3', $sql); + $this->assertSame(['id' => [3, true]], $builder->getBinds()); + $this->assertSame($sql, $builder->getCompiledDelete()); + $this->assertSame([], $builder->getBinds()); + } + + private function connection(bool $success = true): MockConnection + { + $result = $this->createMock(MockResult::class); + $result->method('getRow')->willReturn((object) ['numrows' => 0]); + + $db = $this->getMockBuilder(MockConnection::class) + ->setConstructorArgs([[]]) + ->onlyMethods(['query']) + ->getMock(); + + $db->method('query')->willReturnCallback(static function (string $sql, array $binds, bool $setEscapeFlags) use ($db, $result, $success): bool|MockObject { + $query = new Query($db); + $query->setQuery($sql, $binds, $setEscapeFlags); + $query->getQuery(); + $db->lastQuery = $query; + + return $success ? ($query->isWriteType() ? true : $result) : false; + }); + + return $db; + } +} diff --git a/user_guide_src/source/changelogs/v4.8.0.rst b/user_guide_src/source/changelogs/v4.8.0.rst index 47810301e332..3ed4ee70f8ca 100644 --- a/user_guide_src/source/changelogs/v4.8.0.rst +++ b/user_guide_src/source/changelogs/v4.8.0.rst @@ -73,7 +73,7 @@ Behavior Changes - **Config:** ``BaseConfig`` now scans the environment once instead of checking every property against it, which makes building config classes several times faster without config caching and in Worker Mode. As a result, a bare ``putenv()`` call (adding, changing, or removing a variable) made after the first config class was built may not be seen until ``BaseConfig::reset()`` runs, including in later requests in Worker Mode. See :ref:`upgrade-480-config-env-lookups`. -- **Database:** ``BaseBuilder::getCompiledSelect()``, ``getCompiledInsert()``, ``getCompiledUpdate()`` and ``getCompiledDelete()`` now clear bindings and binding counters after compilation when ``$reset = true`` (the default), preventing binding accumulation when a builder is reused. ``getBinds()`` now returns an empty array after these calls. This aligns ``getCompiledSelect()`` with its documented reset behavior, which should match ``get()``. Pass ``false`` to retain the query and its bindings. See :ref:`upgrade-480-query-builder-bindings`. +- **Database:** ``BaseBuilder::getCompiledSelect()``, ``getCompiledInsert()``, ``getCompiledUpdate()``, ``getCompiledDelete()``, ``delete()`` and ``countAllResults()`` now clear bindings and binding counters after compilation or execution when their reset parameter is enabled (the default). This prevents accumulation when a builder is reused, but ``getBinds()`` now returns an empty array after these calls. This also aligns ``getCompiledSelect()`` with its documented reset behavior, which should match ``get()``. Pass ``false`` for the reset parameter to retain the query and its bindings. Test mode retains bindings needed by its returned raw SQL. See :ref:`upgrade-480-query-builder-bindings`. - **Database:** The Postgre driver's ``$db->error()['code']`` previously always returned ``''``. It now returns the 5-character SQLSTATE string for query and transaction failures (e.g., ``'42P01'``), or ``'08006'`` for connection-level failures. Code that relied on ``$db->error()['code'] === ''`` will need updating. - **Debug:** ``CodeIgniter\Debug\Toolbar\Collectors\BaseCollector`` is now ``abstract``. Custom toolbar collectors must extend it; code that instantiated it directly with ``new BaseCollector()`` will now throw an ``Error``. - **Filters:** HTTP method matching for method-based filters is now case-sensitive. The keys in ``Config\Filters::$methods`` must exactly match the request method diff --git a/user_guide_src/source/database/query_builder.rst b/user_guide_src/source/database/query_builder.rst index 265241f5ff5e..903670506513 100644 --- a/user_guide_src/source/database/query_builder.rst +++ b/user_guide_src/source/database/query_builder.rst @@ -913,6 +913,12 @@ first parameter. .. literalinclude:: query_builder/073.php +.. note:: + **Changed in version 4.8.0.** With ``$reset = true`` (the default), bindings and + binding counters are cleared after execution, so ``getBinds()`` returns an empty + array. Pass ``false`` to retain the query conditions and bindings. Test mode + retains bindings for the placeholders in its returned SQL. + .. _query-builder-exists: $builder->exists() @@ -1654,6 +1660,12 @@ the data to the first parameter of the method: If you want to delete all data from a table, you can use the ``truncate()`` method, or ``emptyTable()``. +.. note:: + **Changed in version 4.8.0.** With ``$resetData = true`` (the default), bindings + and binding counters are cleared after execution, so ``getBinds()`` returns an + empty array. Pass ``false`` as the third argument to retain the query conditions + and bindings. Test mode retains bindings for the placeholders in its returned SQL. + $builder->getCompiledDelete() ----------------------------- diff --git a/user_guide_src/source/installation/upgrade_480.rst b/user_guide_src/source/installation/upgrade_480.rst index d71be11cfa04..35435a3308a9 100644 --- a/user_guide_src/source/installation/upgrade_480.rst +++ b/user_guide_src/source/installation/upgrade_480.rst @@ -22,33 +22,33 @@ Breaking Changes .. _upgrade-480-query-builder-bindings: -Compiled Query Builder Bindings -=============================== +Query Builder Bindings +====================== -``getCompiledSelect()``, ``getCompiledInsert()``, ``getCompiledUpdate()`` and -``getCompiledDelete()`` now clear their bindings and binding counters after -compilation when ``$reset = true`` (the default). Previously, these methods -reset the query clauses but retained the bindings, which accumulated when the -same builder was reused and made subsequent query compilation increasingly -expensive. +``getCompiledSelect()``, ``getCompiledInsert()``, ``getCompiledUpdate()``, +``getCompiledDelete()``, ``delete()`` and ``countAllResults()`` now clear their +bindings and binding counters after compilation or execution when their reset +parameter is enabled (the default). This prevents accumulation when a builder +is reused, but ``getBinds()`` now returns an empty array after these calls. +Bindings are also cleared when the database query returns ``false``. The :ref:`getCompiledSelect() documentation ` already states that the default reset should work just like ``get()``. -Clearing the bindings corrects that inconsistency, but changes the result of -``getBinds()`` after compilation: it now returns an empty array. - -If your application reads the bindings after compilation or reuses the query, -pass ``false`` to preserve them: +Clearing the bindings corrects that inconsistency while changing the observable +result of ``getBinds()``. -.. code-block:: php +To retain the query and its bindings, pass ``false`` as the first argument to +any of the compiled methods or ``countAllResults()``, or as the third argument +to ``delete()``: ``$builder->delete('', null, false)``. This also retains query +clauses such as WHERE and LIMIT. - $sql = $builder->getCompiledSelect(false); - $binds = $builder->getBinds(); +If you only need a snapshot of the bindings, read them before compilation or +execution instead. When passing conditions directly to ``delete()``, move them +into ``where()`` first so their bindings are available before execution. -Passing ``false`` also retains the query clauses, including WHERE and LIMIT. -If you only need a snapshot of the bindings while retaining the default reset -behavior, call ``getBinds()`` before compiling the query instead. -The same migration applies to the compiled INSERT, UPDATE and DELETE methods. +Test mode retains bindings needed by the placeholders in its returned raw SQL. +In particular, ``getCompiledDelete()`` needs these bindings to finish compilation +and clears them afterwards according to its own reset parameter. Query Builder JOIN Overrides ============================