Skip to content

move valid check to beginning of loop#3

Closed
bendavies wants to merge 1 commit into
portphp:masterfrom
bendavies:move-valid-check
Closed

move valid check to beginning of loop#3
bendavies wants to merge 1 commit into
portphp:masterfrom
bendavies:move-valid-check

Conversation

@bendavies

@bendavies bendavies commented Feb 4, 2019

Copy link
Copy Markdown

I'm seeing an issue with a CSV I have where this line
https://github.com/portphp/csv/blob/master/src/CsvReader.php#L116
is coming back as false on the last line.
Switching the do while to a while loop fixes this, but I can't work out why it's happening or replicate it in a test.

Anyway, it doesn't break any existing tests.

@rodnaph

rodnaph commented Jan 29, 2021

Copy link
Copy Markdown
Contributor

This fixes an issue I'm having trying to use CsvReader with the OneToManyReader 👍

@slashrsm

Copy link
Copy Markdown
Contributor

Superseded by #18 — same do-whilewhile ($this->valid()) change, with a reproducible test.

Root cause

SplFileObject::current() returns false at EOF. With headers set, the old do { $line = $this->file->current(); … count($line) … } while ($this->valid()) still entered the body once when the iterator was already invalid → count(false) TypeError on PHP 8+.

Why a plain last-line CSV never failed in tests

Foreach only calls current() when valid() is true, so normal “last data line” fixtures never hit the bad path.

What does hit it

Port\Reader\OneToManyReader (as @rodnaph noted) calls rightReader->current() after next() past the last detail row. Also: current() / getRow() after the reader is exhausted.

Please close this PR in favor of #18 once that lands. Thanks for the original diagnosis — the loop change was correct.

slashrsm added a commit that referenced this pull request Jul 23, 2026
Supersedes #3 with regression tests.
@slashrsm slashrsm closed this in #18 Jul 23, 2026
@slashrsm

Copy link
Copy Markdown
Contributor

Closed as superseded by #18 (merged).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants