From aa4f462b2eaa8279a3fb89ed315d30a2955b5678 Mon Sep 17 00:00:00 2001 From: dualfroz Date: Fri, 4 Sep 2026 09:31:11 +0200 Subject: [PATCH] fix: make Buffer::nth() positional so it stays correct after deletions Buffer::nth() resolved the requested position by adding it to the first row key and indexing the rows array by that computed key. That assumes contiguous keys, but Buffer::delete() removes rows with unset() without reindexing, leaving gaps. After a deletion nth() therefore returned the wrong record (or an empty array for a valid position, or a record for an out-of-range position). Resolve the position against the record order with array_values() so nth() matches getRecords() and ResultSet::nth(). --- PR.md | 94 ++++++++++++++++++++++++++++++++++++++++++++++ src/Buffer.php | 13 ++----- src/BufferTest.php | 18 +++++++++ 3 files changed, 116 insertions(+), 9 deletions(-) create mode 100644 PR.md diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..1ce81e3 --- /dev/null +++ b/PR.md @@ -0,0 +1,94 @@ +# fix: make Buffer::nth() positional so it stays correct after deletions + +## Problem + +`Buffer::nth()` (and `Buffer::nthAsObject()`) return the wrong record once a +record has been deleted from the buffer. `nth()` is a positional accessor - the +same contract implemented by `ResultSet::nth()` via +`new LimitIterator($iterator, $nth, 1)` - so `nth($n)` must return the `$n`-th +record in iteration order, consistent with `getRecords()`. + +```php +$buffer = new League\Csv\Buffer(['id', 'name']); +$buffer->insert(['1', 'a'], ['2', 'b'], ['3', 'c'], ['4', 'd'], ['5', 'e']); +$buffer->delete(fn (array $row) => $row['id'] === '2'); + +// getRecords() correctly yields a, c, d, e +// but: +$buffer->nth(1); // [] (expected ['id' => '3', 'name' => 'c']) +$buffer->nth(4); // ['5', 'e'] (expected [] - only 4 records remain) +``` + +## Root cause + +`src/Buffer.php`, `nthRow()`: + +```php +$offset = $first + $nth; +if (!array_key_exists($offset, $this->rows)) { + return []; +} + +return $this->rows[$nth + $first]; +``` + +The lookup adds `$nth` to the first row key and indexes `$this->rows` by that +computed key. That only works while the row keys are contiguous. +`Buffer::delete()` removes rows with `unset($this->rows[$offset])` without +reindexing, so after any deletion the keys contain gaps and the positional index +`$nth` no longer maps to key `$first + $nth`. As a result `nth()` skips the +deleted offset, and can also return a record for an index that is out of range. + +## The fix + +Resolve `$nth` against the record order instead of the row keys: + +```php +private function nthRow(int $nth, string $method): array +{ + -1 < $nth || throw InvalidArgument::dueToInvalidRecordOffset($nth, $method); + + return array_values($this->rows)[$nth] ?? []; +} +``` + +`array_values()` collapses the gaps, so `$nth` addresses the record by its +position in iteration order, matching `getRecords()` and `ResultSet::nth()`. +`array_values` is already imported by the class. + +## Test + +Added `BufferTest::it_returns_records_by_positional_offset_after_a_middle_record_is_deleted`, +which inserts five records, deletes a middle one, and asserts that +`nth(0..3)` return the four remaining records in order and that `nth(4)` returns +`[]`. + +## Gate + +``` +composer install --ignore-platform-req=ext-xdebug +vendor/bin/phpunit --no-coverage +``` + +The full run reports 6 pre-existing errors, all +`PDOException: could not find driver` from the SQLite3/PDO-backed tests +(`RdbmsResultTest` and the `sqlite3`/`pdo` cases in `BufferTest`); these are +environmental (the `sqlite3` / `pdo_sqlite` extensions are not installed) and are +present unchanged on a clean checkout. Excluding those environment-dependent +cases the suite is green: + +``` +vendor/bin/phpunit --no-coverage --filter '/^(?!.*(sqlite3|pdo)).*$/i' +``` + +Result: exit code `0` - `OK, but some tests were skipped! Tests: 1006, +Assertions: 1990, Skipped: 3` (the 3 skips are pre-existing). + +Counterfactual: reverting `nthRow()` to the key-arithmetic version makes the new +test fail - `nth(1)` returns `[]` instead of `['id' => '3', 'name' => 'c']`. + +## Flags for the account owner + +The SQLite3/PDO tests are not annotated with a required-extension attribute, so +they error (rather than skip) when the drivers are missing. That is unrelated to +this change and was left untouched. diff --git a/src/Buffer.php b/src/Buffer.php index d4083c2..843c219 100644 --- a/src/Buffer.php +++ b/src/Buffer.php @@ -335,16 +335,11 @@ final class Buffer implements TabularData private function nthRow(int $nth, string $method): array { -1 < $nth || throw InvalidArgument::dueToInvalidRecordOffset($nth, $method); - if (null === ($first = $this->firstOffset())) { - return []; - } - $offset = $first + $nth; - if (!array_key_exists($offset, $this->rows)) { - return []; - } - - return $this->rows[$nth + $first]; + // $nth is a positional index (like ResultSet::nth), so it must be + // resolved against the record order rather than the row keys, which + // may contain gaps after records have been deleted. + return array_values($this->rows)[$nth] ?? []; } public function fetchColumn(int|string $index = 0): Iterator diff --git a/src/BufferTest.php b/src/BufferTest.php index e373f7e..8c19af1 100644 --- a/src/BufferTest.php +++ b/src/BufferTest.php @@ -285,6 +285,24 @@ final class BufferTest extends TestCase self::assertSame(['date' => '2025-02-08', 'temperature' => '3', 'place' => 'Berkeley'], $buffer->nth(0)); } + #[Test] + public function it_returns_records_by_positional_offset_after_a_middle_record_is_deleted(): void + { + $buffer = new Buffer(['id', 'name']); + $buffer->insert(['1', 'a'], ['2', 'b'], ['3', 'c'], ['4', 'd'], ['5', 'e']); + + // Delete the second record (id = 2), which leaves a gap in the row keys. + self::assertSame(1, $buffer->delete(fn (array $row): bool => $row['id'] === '2')); + self::assertSame(4, $buffer->recordCount()); + + // nth() is positional and must stay consistent with getRecords(). + self::assertSame(['id' => '1', 'name' => 'a'], $buffer->nth(0)); + self::assertSame(['id' => '3', 'name' => 'c'], $buffer->nth(1)); + self::assertSame(['id' => '4', 'name' => 'd'], $buffer->nth(2)); + self::assertSame(['id' => '5', 'name' => 'e'], $buffer->nth(3)); + self::assertSame([], $buffer->nth(4)); + } + #[Test] public function it_can_not_insert_invalid_records(): void {