Fix #69181: READ_CSV|DROP_NEW_LINE drops newlines within fields#7618
Fix #69181: READ_CSV|DROP_NEW_LINE drops newlines within fields#7618cmb69 wants to merge 2 commits intophp:masterfrom
Conversation
|
Sounds reasonable to me. |
|
Great, it works perfectly as far as I can tell. BTW, I tried fixing this via a contribution to php-src 6 years ago, but I didn't know what I was doing and ended up having a lot of breaking changes/other side effects. |
|
That makes sense. Thanks for the explanation and great work. |
ext/spl/spl_directory.c
Outdated
| intern->u.file.current_line_len = 0; | ||
| } else { | ||
| if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { | ||
| if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE) && !SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV)) { |
There was a problem hiding this comment.
Wouldn't this also affect the behavior of the fgets() method?
There was a problem hiding this comment.
Right! (in case ::READ_CSV is set)
There was a problem hiding this comment.
And the !SplFileObject::READ_CSV case is an issue, since ::fgetcsv() exhibits the same broken behavior (namely, that it drops the first linebreak in a quoted field): https://3v4l.org/OEiFq. This should now be fixed.
|
@cmb69 sorry for the late reply. From League CSV stand point I removed the use of So TL;DR:
Hope that comment will help |
|
@cmb69 is this bug still present? |
|
Yes, the bug is still present, but this PR needs more work, and I'm reluctant to fix this in any of the stable branches. I'll have a closer look soonish. |
One may argue that `DROP_NEW_LINE` does not make sense in combination with `READ_CSV`, but without `DROP_NEW_LINE`, `SKIP_EMPTY` does not skip empty lines at all. We could fix that, but do not for BC reasons. Instead we no longer drop newlines in `spl_filesystem_file_read_ex()` when reading CSV, but handle that in `spl_filesystem_file_read_csv()` by treating lines with only (CR)LF as being empty as well.
|
I've rebased onto "master" (after all, this is SPL), and fixed the issue for |
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
The output of the test looks correct to me. Just a couple of comment to clarify stuff
| /* }}} */ | ||
|
|
||
| static zend_result spl_filesystem_file_read_ex(spl_filesystem_object *intern, bool silent, zend_long line_add) /* {{{ */ | ||
| static zend_result spl_filesystem_file_read_ex(spl_filesystem_object *intern, bool silent, zend_long line_add, bool csv) /* {{{ */ |
There was a problem hiding this comment.
One thing I was thinking was instead of a csv bool arg, to pass the SPL flags as an argument and spl_filesystem_file_read_csv() to add the SPL_FILE_OBJECT_READ_CSV flag when calling this function. Not sure if this is cleaner tho
There was a problem hiding this comment.
Hmm, I guess we could even temporarily add the SPL_FILE_OBJECT_READ_CSV flag. But I'm afraid it's somewhat messy either way.
5 participants
One may argue that
DROP_NEW_LINEdoes not make sense in combinationwith
READ_CSV, but withoutDROP_NEW_LINE,SKIP_EMPTYdoes notskip empty lines at all. We could fix that, but do not for BC reasons.
Instead we no longer drop newlines in
spl_filesystem_file_read()whenreading CSV, but handle that in
spl_filesystem_file_read_csv()bytreating lines with only (CR)LF as being empty as well.
@nyamsprod, would that cause any BC issues? Also, @Girgias may want to have a look at this.