Skip to content

Fix GH-8563 #8651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix GH-8563 #8651

wants to merge 2 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented May 29, 2022

With memory streams if we get a NULL buffer we must not instantiate an empty line.

Splitting this from #8644 as this one seems to be fixed properly

@Girgias Girgias requested a review from cmb69 May 29, 2022 16:15
@Girgias
Copy link
Member Author

Girgias commented May 29, 2022

Ah great, this hits the issue 188b1d4 which was fixed in 8.1

@Girgias
Copy link
Member Author

Girgias commented May 29, 2022

Actually... I'm wondering if this is going to break non standard CSV files which have an empty line somewhere in the file (which is obviously untested).

@cmb69
Copy link
Member

cmb69 commented May 31, 2022

I'm wondering if this is going to break non standard CSV files which have an empty line somewhere in the file (which is obviously untested).

I think we should add tests for that. The current behavior is somewhat suprising: https://3v4l.org/PS0El; only if SplFileObject::DROP_NEW_LINE is set, empty lines will be skipped. This is consistent with ~SplFileObject::READ_CSV, but still suprising, because the line breaks are stripped from normal CSV reading.

Girgias added 2 commits June 8, 2022 11:50
With memory streams if we get a NULL buffer we must not instantiate an empty line
@Girgias
Copy link
Member Author

Girgias commented Jun 8, 2022

Seems like my fix might indeed be correct as it does not break it (however while merging up the NULL will need to be changed to false)

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is good to be merged.

I'm not sure, though, about renaming the existing tests (i.e. moving them to a subdirectory); maybe postpone that to "master".

@Girgias
Copy link
Member Author

Girgias commented Jun 20, 2022

Merged as 6f87a5c

@Girgias Girgias closed this Jun 20, 2022
@Girgias Girgias deleted the fix-gh8563 branch June 20, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants