# [PATCH] Bug #46367 - fputcsv does not add the correct newline character on Windows - Thread: https://externals.io/message/41320 - Original archive: https://news-web.php.net/php.internals/41320 - Started by John Mertic on 2008-10-22T17:03:46+00:00 - Messages: 13 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #41320 — John Mertic — 2008-10-22T17:03:46+00:00 URL: https://externals.io/message/41320#41320 Hi All, My first patch so be gentle :-). Per the documentation for the `fputcsv()` function, it adds a newline to the end of the csv string it returns. However, it is hardcoded to be '\n' ( default for unix newline ), while Windows uses \r\n. PHP should do this as well. Below is a patch to fix this issue; it uses the constant `PHP_EOL` to get the correct newline to use on the current platform: Index: php-src/ext/standard/file.c =================================================================== RCS file: /repository/php-src/ext/standard/file.c,v retrieving revision 1.530 diff -r1.530 file.c 2107c2107 < smart_str_appendc(&csvline, '\n'); --- > smart_str_appendc(&csvline, PHP_EOL); John Mertic jmertic@gmail.com http://jmertic.wordpress.com "Explaining a joke is like dissecting a frog: you understand it better, but the frog dies in the process." --Mark Twain --- ## #41321 — Lars Strojny — 2008-10-22T17:36:14+00:00 URL: https://externals.io/message/41320#41321 · In reply to: #41320 Hi John, Am Mittwoch, den 22.10.2008, 10:03 -0700 schrieb John Mertic: [...] > Below is a patch to fix this issue; it uses the constant `PHP_EOL` to > get the correct newline to use on the current platform: Thanks for your patch. A few things to mention, as it is your first patch: please use "diff -ru" to create unified diffs. Also we try to always add tests for the things we fix or create. Would you mind creating a test for the fix you sent to make sure no regression happens in the next n years? Thanks, Lars -- Jabber: lars@strojny.net Weblog: http://usrportage.de --- ## #41322 — John Mertic — 2008-10-22T18:33:50+00:00 URL: https://externals.io/message/41320#41322 Hi Stefan, The context on how this function is being used is the case where you are writing out to a file, so in this case I think we should be using the EOL standards of the underlying platform. If we were just keeping the output as a string, I could see us sticking with the standard newline character. And also, the function previously took a character and still will take a character ( `PHP_EOL` is #defined to whatever character string should be used for the EOL character, be it "\r\n" or "\n" ), unless I'm missing something ( which could be since I haven't done much C in a while ). John Mertic jmertic@gmail.com http://jmertic.wordpress.com "Explaining a joke is like dissecting a frog: you understand it better, but the frog dies in the process." --Mark Twain > John Mertic wrote: > > […] > > This can't work. Either the function takes a character or a pointer to a > character, it can't take both. > > Also, a "newline" is \n. Everywhere. The "End of line"-marker is distinct > from that. > > Regards, > Stefan --- ## #41323 — John Mertic — 2008-10-22T18:35:49+00:00 URL: https://externals.io/message/41320#41323 · In reply to: #41321 Hi Lars, Thanks for the pointers, updated the patch and added a test. Index: file.c =================================================================== RCS file: /repository/php-src/ext/standard/file.c,v retrieving revision 1.530 diff -u -r1.530 file.c --- file.c 21 Oct 2008 22:06:48 -0000 1.530 +++ file.c 22 Oct 2008 18:21:10 -0000 @@ -2104,7 +2104,7 @@ } } - smart_str_appendc(&csvline, '\n'); + smart_str_appendc(&csvline, PHP_EOL); smart_str_0(&csvline); ret = php_stream_write(stream, csvline.c, csvline.len); John Mertic jmertic@gmail.com http://jmertic.wordpress.com "Explaining a joke is like dissecting a frog: you understand it better, but the frog dies in the process." --Mark Twain > Hi John, > > > Am Mittwoch, den 22.10.2008, 10:03 -0700 schrieb John Mertic: > [...] > > […] > > Thanks for your patch. A few things to mention, as it is your first > patch: please use "diff -ru" to create unified diffs. Also we try to > always add tests for the things we fix or create. Would you mind > creating a test for the fix you sent to make sure no regression happens > in the next n years? > > Thanks, Lars > -- > Jabber: lars@strojny.net > Weblog: http://usrportage.de --- ## #41324 — Ilia Alshanetsky — 2008-10-22T20:16:56+00:00 URL: https://externals.io/message/41320#41324 · In reply to: #41323 You cannot use smart_str_appendc() in this case, since the EOL could be a 2 byte string "\r\n". smart_str_appendl(&csvline, PHP_EOL, sizeof(PHP_EOL)-1); should be used here. > Hi Lars, > > Thanks for the pointers, updated the patch and added a test. > > Index: file.c > =================================================================== > RCS file: /repository/php-src/ext/standard/file.c,v > retrieving revision 1.530 > diff -u -r1.530 file.c > --- file.c 21 Oct 2008 22:06:48 -0000 1.530 > +++ file.c 22 Oct 2008 18:21:10 -0000 > @@ -2104,7 +2104,7 @@ > } > } > > - smart_str_appendc(&csvline, '\n'); > + smart_str_appendc(&csvline, PHP_EOL); > smart_str_0(&csvline); > > ret = php_stream_write(stream, csvline.c, csvline.len); > > > John Mertic > jmertic@gmail.com > http://jmertic.wordpress.com > > "Explaining a joke is like dissecting a frog: you understand it > better, but the frog dies > in the process." --Mark Twain > > > > On Wed, Oct 22, 2008 at 10:36 AM, Lars Strojny > wrote: > > […] > > -- > > Ilia Alshanetsky --- ## #41325 — John Mertic — 2008-10-22T20:26:30+00:00 URL: https://externals.io/message/41320#41325 · In reply to: #41324 Hi Ilia, Sorry for my C confusion, like I said it's been awhile. Anyways, would smart_str_appends() be the correct function to use then? John Mertic jmertic@gmail.com http://jmertic.wordpress.com "Explaining a joke is like dissecting a frog: you understand it better, but the frog dies in the process." --Mark Twain > You cannot use smart_str_appendc() in this case, since the EOL could be a 2 > byte string "\r\n". > > smart_str_appendl(&csvline, PHP_EOL, sizeof(PHP_EOL)-1); should be used > here. > > > > > […] > > Ilia Alshanetsky --- ## #41326 — Ilia Alshanetsky — 2008-10-22T20:33:47+00:00 URL: https://externals.io/message/41320#41326 · In reply to: #41325 smart_str_appendl() would be faster since it'll avoid having to calculate the string length, and `sizeof()` can be used to determine the length of the constant, resulting in faster code. On a general note, I am not sure its a good idea to change the EOL for CSV file, since many scanners expect "\n" as a line separator and the change may introduce regressions. > Hi Ilia, > > Sorry for my C confusion, like I said it's been awhile. > > Anyways, would smart_str_appends() be the correct function to use > then? > > John Mertic > jmertic@gmail.com > http://jmertic.wordpress.com > > "Explaining a joke is like dissecting a frog: you understand it > better, but the frog dies > in the process." --Mark Twain > > > > On Wed, Oct 22, 2008 at 4:16 PM, Ilia Alshanetsky > wrote: > > […] Ilia Alshanetsky --- ## #41328 — John Mertic — 2008-10-22T21:28:11+00:00 URL: https://externals.io/message/41320#41328 · In reply to: #41324 > You cannot use smart_str_appendc() in this case, since the EOL could be a 2 > byte string "\r\n". > > smart_str_appendl(&csvline, PHP_EOL, sizeof(PHP_EOL)-1); should be used > here. > > > > > […] > > Ilia Alshanetsky > > > > > Hi Ilia, I can see your point about scanner's having trouble with different EOL characters, but I know in our use case the problem is that the CSV files created will come out the lines all mashed together, which is a problem for readability of the file. And since most CSV scanners know how to deal with Windows, I would think we are pretty safe ;-) Attached is an updated patch that uses this function instead. -- John Mertic jmertic@gmail.com | http://jmertic.wordpress.com --- ## #41722 — John Mertic — 2008-11-06T13:52:47+00:00 URL: https://externals.io/message/41320#41722 · In reply to: #41328 > > > […] > > Hi Ilia, > > I can see your point about scanner's having trouble with different EOL > characters, but I know in our use case the problem is that the CSV > files created will come out the lines all mashed together, which is a > problem for readability of the file. And since most CSV scanners know > how to deal with Windows, I would think we are pretty safe ;-) > > Attached is an updated patch that uses this function instead. > > -- > John Mertic > jmertic@gmail.com | http://jmertic.wordpress.com > Hi All, I haven't seen any further comments on this bug; does it seem safe to commit or should it wait? I've re-attached the patch and test again for everyone's reference. Thanks! John Mertic jmertic@gmail.com | http://jmertic.wordpress.com --- ## #44781 — John Mertic — 2009-07-07T17:46:16+00:00 URL: https://externals.io/message/41320#44781 · In reply to: #41722 Hi All, Bringing this one back once more; let me know what everyone thinks about it. If it's safe to commit than if someone could ( or give me the karma to do so ) that would be great. If not, let me know what should be done about it instead. Thanks! John Mertic jmertic@gmail.com | http://jmertic.wordpress.com --- ## #44785 — Jani Taskinen — 2009-07-07T18:27:29+00:00 URL: https://externals.io/message/41320#44785 · In reply to: #44781 John Mertic kirjoitti: > Hi All, > > Bringing this one back once more; let me know what everyone thinks > about it. If it's safe to commit than if someone could ( or give me > the karma to do so ) that would be great. If not, let me know what > should be done about it instead. Is \r\n okay on Mac? Is \r okay on Windows? etc. Short: Shouldn't this be always \r\n? Or better yet, default to \r\n and add extra option for fputcsv with which you can set it to anything you like.. :) --Jani --- ## #44789 — Brian Moon — 2009-07-07T19:22:57+00:00 URL: https://externals.io/message/41320#44789 · In reply to: #44785 > John Mertic kirjoitti: > > […] > > Is \r\n okay on Mac? Is \r okay on Windows? etc. > > Short: Shouldn't this be always \r\n? Or better yet, default to \r\n and > add extra option for fputcsv with which you can set it to anything you > like.. :) > > --Jani The only program I know of in existence that has issues is Notepad on Windows. All the mac programs I use will deal with whatever. Having said that, it should be just as flexible for creating CSV files as fgetcsv is for reading them. Of course, a good ol' fputs does a fine job of making a CSV file too. -- Brian. -------- http://brian.moonspot.net/ --- ## #44793 — John Mertic — 2009-07-07T21:10:03+00:00 URL: https://externals.io/message/41320#44793 · In reply to: #44789 The big issue I saw was that `fgetcsv()` used `PHP_EOL` for determining line endings, but `fputcsv()` didn't, which on Windows was causing csv files written by PHP not be able to be read back ( assuming auto_detect_line_endings is turned off ). John Mertic jmertic@gmail.com | http://jmertic.wordpress.com > > > […] > > The only program I know of in existence that has issues is Notepad on > Windows.  All the mac programs I use will deal with whatever. > > Having said that, it should be just as flexible for creating CSV files as > fgetcsv is for reading them. > > Of course, a good ol' fputs does a fine job of making a CSV file too. > > -- > > Brian. > -------- > http://brian.moonspot.net/