Grokbase › Groups › PHP › php-internals › October 2008
FAQ
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
[email protected]
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

Search Discussions

  • Lars Strojny at Oct 22, 2008 at 5:36 pm ⇧
    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
  • John Mertic at Oct 22, 2008 at 6:35 pm ⇧
    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
    [email protected]
    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:
    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: [email protected]
    Weblog: http://usrportage.de
  • Ilia Alshanetsky at Oct 22, 2008 at 8:17 pm ⇧
    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
    [email protected]
    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:
    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: [email protected]
    Weblog: http://usrportage.de
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    Ilia Alshanetsky
  • John Mertic at Oct 22, 2008 at 8:26 pm ⇧
    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
    [email protected]
    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:
    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
    [email protected]
    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:

    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: [email protected]
    Weblog: http://usrportage.de
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    Ilia Alshanetsky



  • Ilia Alshanetsky at Oct 22, 2008 at 8:33 pm ⇧
    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.

    On 22-Oct-08, at 4:26 PM, John Mertic wrote:

    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
    [email protected]
    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:
    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
    [email protected]
    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 <[email protected]>
    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: [email protected]
    Weblog: http://usrportage.de
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    Ilia Alshanetsky



    Ilia Alshanetsky
  • John Mertic at Oct 22, 2008 at 9:28 pm ⇧

    On Wed, Oct 22, 2008 at 1:16 PM, Ilia Alshanetsky 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
    [email protected]
    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:

    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: [email protected]
    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 at Nov 6, 2008 at 1:52 pm ⇧

    On Wed, Oct 22, 2008 at 1:28 PM, John Mertic wrote:
    On Wed, Oct 22, 2008 at 1:16 PM, Ilia Alshanetsky 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
    [email protected]
    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:

    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: [email protected]
    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
    [email protected] | 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
    [email protected] | http://jmertic.wordpress.com
  • John Mertic at Jul 7, 2009 at 5:46 pm ⇧
    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
    [email protected] | http://jmertic.wordpress.com
  • Jani Taskinen at Jul 7, 2009 at 6:27 pm ⇧

    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
  • Brian Moon at Jul 7, 2009 at 8:05 pm ⇧

    On 7/7/09 1:27 PM, Jani Taskinen wrote:
    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.
  • John Mertic at Jul 7, 2009 at 9:10 pm ⇧
    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
    [email protected] | http://jmertic.wordpress.com



    On Tue, Jul 7, 2009 at 3:22 PM, Brian Moonwrote:
    On 7/7/09 1:27 PM, Jani Taskinen wrote:

    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/
  • John Mertic at Oct 22, 2008 at 6:33 pm ⇧
    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
    [email protected]
    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:14 AM, Stefan Walk wrote:
    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedOct 22, '08 at 5:03p
activeJul 7, '09 at 9:10p
posts13
users5
websitephp.net

5 users in discussion

John Mertic: 8 posts Ilia Alshanetsky: 2 posts Lars Strojny: 1 post Jani Taskinen: 1 post Brian Moon: 1 post

Content

People

Support

Translate

site design / logo © 2023 Grokbase