[PR #5740] Fix onNotSuccessfulTest skipping the latest query #9711

New Issue

admin · 2026-01-22T16:05:12+01:00

admin commented

2026-01-22 16:05:12 +01:00

Original Pull Request: https://github.com/doctrine/orm/pull/5740

State: closed
Merged: No

... + fix SQL queries numbering in test failure output, as doctrine/dbal#2340

The bottom modification in commit 0fa136e369 (whose effect is still there in https://github.com/doctrine/doctrine2/blob/master/tests/Doctrine/Tests/OrmFunctionalTestCase.php) was supposedly intended to output only the 25 (max) latest queries from the most recent to the oldest (rather than all the queries from the first to the latest), but it introduced a regression:
let N be a shorthand for count($this->_sqlLoggerStack->queries),
given that we're inside the condition that N!=0, i.e. N>0, i.e. N>=1,
the loop for($i = N-1; $i > max(N-25, 0); $i--)
iterates from N-1 (included) down to max(N-25, 0) excluded, so e.g.:

if N is 1: it iterates “from 0 (included) down to 0 excluded”, i.e. it doesn't iterate at all!
if N is 10: it iterates from 9 (included) down to 0 excluded, i.e. from 9 down to 1;
if N is 35: it iterates from 34 (included) down to 10 excluded, i.e. from 34 down to 11;

so it should rather have been iterating from N-1 (included) down to max(N-25, 0) (included),
i.e. the > should have been >= in the loop condition.
... At least until the change 8cbb70c61a (diff-9997e1f6eb43e2dcdf6f284b1d49164e), before which the DebugStack was storing its queries at indices 0 to N-1, but since this change (more than 5 years ago) it has been storing them at indices 1 to N (and there are even tests that depend on that, for example: 0d76c6e952/tests/Doctrine/Tests/DBAL/Logging/DebugStackTest.php (L22) (and also line 33) and 97cc49033e/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC1595Test.php (L41)),
so now the loop should rather iterate from N (included) down to max(N-25, 0) excluded,
i.e. we should remove the -1 in the initialization of $i (and let the > in the condition), so as to have:

if N is 1: it should iterate from 1 (included) down to 0 excluded, i.e. 1 only;
if N is 10: it should iterate from 10 (included) down to 0 excluded, i.e. from 10 down to 1;
if N is 35: it should iterate from 35 (included) down to 10 excluded, i.e. from 35 down to 11;

and lastly, the loop body should output $i directly for numbering rather than $i+1 (same as PR doctrine/dbal#2340).

(PS: I don't know how to write a unit-test for this fix, as it concerns the debug output for test failures...)

**Original Pull Request:** https://github.com/doctrine/orm/pull/5740 **State:** closed **Merged:** No --- ... + fix SQL queries numbering in test failure output, as doctrine/dbal#2340 --- The bottom modification in commit 0fa136e3694202d73bdaef7a2e7fdc27fe3f83a4 (whose effect is still there in https://github.com/doctrine/doctrine2/blob/master/tests/Doctrine/Tests/OrmFunctionalTestCase.php) was supposedly intended to output only the 25 (max) latest queries from the most recent to the oldest (rather than all the queries from the first to the latest), but it introduced a regression: let `N` be a shorthand for `count($this->_sqlLoggerStack->queries)`, given that we're inside the condition that `N`!=0, i.e. `N`>0, i.e. `N`>=1, the loop `for($i = N-1; $i > max(N-25, 0); $i--)` iterates from `N-1` (included) down to `max(N-25, 0)` _excluded_, so e.g.: - if `N` is `1`: it iterates “from `0` (included) down to `0` _excluded_”, i.e. it doesn't iterate at all! - if `N` is `10`: it iterates from `9` (included) down to `0` _excluded_, i.e. from `9` down to **`1`**; - if `N` is `35`: it iterates from `34` (included) down to `10` _excluded_, i.e. from `34` down to **`11`**; so it should rather have been iterating from `N-1` (included) down to `max(N-25, 0)` **(included)**, i.e. the `>` should have been **`>=`** in the loop condition. ... At least until the change https://github.com/doctrine/dbal/commit/8cbb70c61a3931d1e60b52373c5787bab6711a99#diff-9997e1f6eb43e2dcdf6f284b1d49164e, before which the `DebugStack` was storing its queries at indices `0` to `N-1`, but since this change (more than 5 years ago) it has been storing them at indices **`1` to `N`** (and there are even tests that depend on that, for example: https://github.com/doctrine/dbal/blob/0d76c6e952bd97f2ccc9232b1e0e64cb79cf5652/tests/Doctrine/Tests/DBAL/Logging/DebugStackTest.php#L22 (and also line 33) and https://github.com/doctrine/doctrine2/blob/97cc49033ee18e2ecb5803f95732ecdc6875aec0/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC1595Test.php#L41), so now the loop should rather iterate from **`N`** (included) down to `max(N-25, 0)` _excluded_, i.e. we should remove the `-1` in the initialization of `$i` (and let the `>` in the condition), so as to have: - if `N` is `1`: it should iterate from **`1`** (included) down to `0` _excluded_, i.e. `1` only; - if `N` is `10`: it should iterate from **`10`** (included) down to `0` _excluded_, i.e. from `10` down to `1`; - if `N` is `35`: it should iterate from **`35`** (included) down to `10` _excluded_, i.e. from `35` down to `11`; and lastly, the loop body should output `$i` directly for numbering rather than `$i+1` (same as PR doctrine/dbal#2340). (PS: I don't know how to write a unit-test for this fix, as it concerns _the debug output for test failures_...)

admin added the pull-request label 2026-01-22 16:05:12 +01:00

admin closed this issue

2026-01-22 16:05:12 +01:00

admin referenced this issue

2026-01-22 16:12:01 +01:00

[PR #9711] Merge 2.13.x into 3.0.x #11820

Sign in to join this conversation.