Re: [PATCH] Bug #46367 - fputcsv does not add the correct newline character on Windows

On Wed, Oct 22, 2008 at 1:28 PM, John Mertic <jmertic@php.net> wrote: > On Wed, Oct 22, 2008 at 1:16 PM, Ilia Alshanetsky <ilia@prohost.org> wrote: >> 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. >> >> On 22-Oct-08, at 2:35 PM, John Mertic wrote: >> >>> 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 <lstrojny@php.net> wrote: >>>> >>>> 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 >>>> >>> >>> -- >>> PHP Internals - PHP Runtime Development Mailing List >>> To unsubscribe, visit: http://www.php.net/unsub.php >> >> 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 > 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

Attachment: [application/force-download] file.c.patch