Fix #69181: READ_CSV|DROP_NEW_LINE drops newlines within fields #7618
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
| @@ -2045,7 +2045,7 @@ static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) / | |||
| intern->u.file.current_line = estrdup(""); | |||
| 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 |
| @@ -1887,7 +1887,7 @@ static zend_result spl_filesystem_object_cast(zend_object *readobj, zval *writeo | |||
| } | |||
| /* }}} */ | |||
|
|
|||
| 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.
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.