17 years ago by John Mertic — view source — reply

unread

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

17 years ago by Lars Strojny — view source — reply

unread

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

17 years ago by John Mertic — view source — reply

unread

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:
[...]

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

17 years ago by Ilia Alshanetsky — view source — reply

unread

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 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

--

Ilia Alshanetsky

17 years ago by John Mertic — view source — reply

unread

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.
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:
[...]

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

--
Ilia Alshanetsky

17 years ago by Ilia Alshanetsky — view source — reply

unread

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 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.
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

--
Ilia Alshanetsky

Ilia Alshanetsky

17 years ago by John Mertic — view source — reply

unread

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

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

--
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

16 years ago by John Mertic — view source — reply

unread

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

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

--
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

16 years ago by John Mertic — view source — reply

unread

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

16 years ago by Jani Taskinen — view source — reply

unread

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

16 years ago by Brian Moon — view source — reply

unread

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

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/

16 years ago by John Mertic — view source — reply

unread

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

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

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/

17 years ago by John Mertic — view source — reply

unread

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:
>>
>> 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);
>>>
>>
>>
>
> 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

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

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');

Thanks, Lars

Index: file.c

Thanks, Lars

Index: file.c

Thanks, Lars

Index: file.c

Thanks, Lars

Index: file.c

Thanks, Lars

Index: file.c

Thanks, Lars

Index: file.c

Thanks, Lars

Brian.

Brian.

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');