Everything

php

[RFC] Kill proprietary CSV escaping mechanism

php.internals

Christoph Becker

7 years ago

Hi!

I hereby put the “Kill proprietary CSV escaping mechanism” under discussion:

<https://wiki.php.net/rfc/kill-csv-escaping>

Any comments are welcome!

--
Christoph M. Becker

Theodore Brown

7 years ago

On Thursday, September 27, 2018 5:29 AM Christoph M. Becker <cmbecker69@gmx.de> wrote:

> I hereby put the “Kill proprietary CSV escaping mechanism” under discussion:
>
> <https://wiki.php.net/rfc/kill-csv-escaping>
>
> Any comments are welcome!

Huge +1 from me! This would remove the need for a lot of nasty hacks and
workarounds when reading/writing CSVs.

From my perspective, the first part of the proposal (allow passing an empty
string as the $escape argument) hardly needs an RFC, since it's essentially
a bugfix and shouldn't break userland.

So it seems like the main question the RFC will decide is a deprecation
timeline for the $escape argument. I'm not sure it makes sense to have
separate deprecation phases for passing a non-empty $escape string, and
passing an explicit escape argument at all. This would require developers
to change their code twice: once to pass an empty string, and again to
remove the empty string.

Would it be acceptable to change the default value of $escape to an empty
string and deprecate passing an explicit $escape parameter in PHP 7.4?
This would be a minor BC break, but would allow developers to fix their
code once, so it will continue working when the escape parameter is removed
(possibly in PHP 8).

But in the end I don't care so much about the exact timeline, as long as
this issue can finally be fixed!

Thanks again for your work on this,

Theodore Brown

Stas Malyshev

7 years ago

Hi!

> I hereby put the “Kill proprietary CSV escaping mechanism” under discussion:
>
> <https://wiki.php.net/rfc/kill-csv-escaping>

For fputcsv (and generally writing) this is probably OK. I am much more
concerned about reading - this may make files generated by PHP be
unreadable by php. And some use cases keep archives for a very long
time. Trying to find something in a backup and discovering that your
code is not longer able to read your data is no fun.

--
Stas Malyshev
smalyshev@gmail.com

Pierre Joye

7 years ago

Hi Stas

On Fri, Sep 28, 2018, 2:00 PM Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> Hi!
>
> > I hereby put the “Kill proprietary CSV escaping mechanism” under
> discussion:
> >
> > <https://wiki.php.net/rfc/kill-csv-escaping>
>
> For fputcsv (and generally writing) this is probably OK. I am much more
> concerned about reading - this may make files generated by PHP be
> unreadable by php. And some use cases keep archives for a very long
> time. Trying to find something in a backup and discovering that your
> code is not longer able to read your data is no fun.

This is a very good point. Removing it in read mode is going to break
things badly out there (I had a few projects using it. We need a sensible
decision here.

best,
Pierre

Christoph Becker

7 years ago

On 30.09.2018 at 06:06, Pierre Joye wrote:

> On Fri, Sep 28, 2018, 2:00 PM Stanislav Malyshev <smalyshev@gmail.com>
> wrote:
>
>>> I hereby put the “Kill proprietary CSV escaping mechanism” under
>> discussion:
>>>
>>> <https://wiki.php.net/rfc/kill-csv-escaping>
>>
>> For fputcsv (and generally writing) this is probably OK. I am much more
>> concerned about reading - this may make files generated by PHP be
>> unreadable by php. And some use cases keep archives for a very long
>> time. Trying to find something in a backup and discovering that your
>> code is not longer able to read your data is no fun.
>
> This is a very good point. Removing it in read mode is going to break
> things badly out there (I had a few projects using it. We need a sensible
> decision here.

If we cannot eventually get rid of the escaping for CSV reading (note
that I proposed to keep the possibility to use it until PHP *9*), I
don't think that we should remove it for CSV writing either, since that
would make SplFileObject::setCsvControl() and ::getCsvControl() pretty
much confusing.  Consequently, it wouldn't make sense to deprecate using
a non-empty string as $escape, and therefore we can't change the
default.  So we're basically left with
<https://github.com/php/php-src/pull/3515>, which, in my opinion,
doesn't need an RFC at all.

Anybody who uses the escaping mechanism (deliberately or unwittingly)
should be aware of the roundtrip issues (see
<https://bugs.php.net/74713> and <https://bugs.php.net/76940>), though.

--
Christoph M. Becker

Stas Malyshev

7 years ago

Hi!

> If we cannot eventually get rid of the escaping for CSV reading (note
> that I proposed to keep the possibility to use it until PHP *9*), I
> don't think that we should remove it for CSV writing either, since that
> would make SplFileObject::setCsvControl() and ::getCsvControl() pretty
> much confusing.  Consequently, it wouldn't make sense to deprecate using

SplFileObject is a bit of a problem since it has single setting for both
reading and writing. I'd say it's fine to deprecate using escape there,
with the idea that if somebody needs it for reading, they'd ignore the
deprecation message, or use fgetcsv. I guess using single set of setting
for both read and write is not that great an idea, in retrospect, but we
probably can't do too much about it.

I think allowing not having an escape and making it the default is a
good idea, though mismatch between read and write may be problematic. At
least the first part - allowing not having escape - I think should be
done and does not need RFC even, but the default change probably does
and I think this should be in a major version, since it's a BC break,
albeit not a major one. So, I'd do this:

1. Enable no escape char at all ASAP
2. Switch to no escape by default for read & write for PHP 8
3. Deprecate non-empty escape for SplFileObject and fputcsv for PHP 8,
leave fgetcsv alone.

--
Stas Malyshev
smalyshev@gmail.com

Christoph Becker

7 years ago

On 30.09.2018 at 22:17, Stanislav Malyshev wrote:

>> If we cannot eventually get rid of the escaping for CSV reading (note
>> that I proposed to keep the possibility to use it until PHP *9*), I
>> don't think that we should remove it for CSV writing either, since that
>> would make SplFileObject::setCsvControl() and ::getCsvControl() pretty
>> much confusing.  Consequently, it wouldn't make sense to deprecate using
>
> SplFileObject is a bit of a problem since it has single setting for both
> reading and writing. I'd say it's fine to deprecate using escape there,
> with the idea that if somebody needs it for reading, they'd ignore the
> deprecation message, or use fgetcsv. I guess using single set of setting
> for both read and write is not that great an idea, in retrospect, but we
> probably can't do too much about it.
>
> I think allowing not having an escape and making it the default is a
> good idea, though mismatch between read and write may be problematic. At
> least the first part - allowing not having escape - I think should be
> done and does not need RFC even, but the default change probably does
> and I think this should be in a major version, since it's a BC break,
> albeit not a major one. So, I'd do this:
>
> 1. Enable no escape char at all ASAP
> 2. Switch to no escape by default for read & write for PHP 8
> 3. Deprecate non-empty escape for SplFileObject and fputcsv for PHP 8,
> leave fgetcsv alone.

I don't think we're doing anyone a favor by introducing even more
inconsistencies between reading and writing CSV.  Also, if we stick with
the legacy CSV reading, we can't simplify the overly complex
implementation.  Therefore, I suggest to only add support for an empty
$escape parameter (PR #3515), and withdraw the RFC to deprecate and
eventually remove the proprietary escaping mechanism.

--
Christoph M. Becker

Stas Malyshev

7 years ago

Hi!

> implementation.  Therefore, I suggest to only add support for an empty
> $escape parameter (PR #3515),

Sounds good.

--
Stas Malyshev
smalyshev@gmail.com

Nikita Popov

7 years ago

On Thu, Sep 27, 2018 at 12:29 PM Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> Hi!
>
> I hereby put the “Kill proprietary CSV escaping mechanism” under
> discussion:
>
> <https://wiki.php.net/rfc/kill-csv-escaping>
>
> Any comments are welcome!
>

Could you please add a description of how the escaping mechanism currently
works for read and write? My vague recollection is that the write and read
behavior actually have nothing to do with each other and the fputcsv
$escape parameter would be better described as the $corrupt parameter. We
may want to treat both cases in different ways.

Nikita

Christoph Becker

7 years ago

On 30.09.2018 at 12:42, Nikita Popov wrote:

> On Thu, Sep 27, 2018 at 12:29 PM Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
>
>> I hereby put the “Kill proprietary CSV escaping mechanism” under
>> discussion:
>>
>> <https://wiki.php.net/rfc/kill-csv-escaping>
>>
>> Any comments are welcome!
>
> Could you please add a description of how the escaping mechanism currently
> works for read and write? My vague recollection is that the write and read
> behavior actually have nothing to do with each other and the fputcsv
> $escape parameter would be better described as the $corrupt parameter. We
> may want to treat both cases in different ways.

It's hard for me to describe something whose sense escapes me.  How can
two competing escaping mechanisms work at the same time?

Anyhow, from looking at the implementation the write case (fputcsv)[1]
is pretty clear:

* if a field contains an escape character, it is enclosed with the
enclosure character
* if an enclosure character is preceeded by an escape character in a
field, both are written verbatim (i.e. the enclosure character is not
doubled)

The implementation of the CSV reading[2] is way more complex, but
appears to be broken anyway.  For instance:

$str = '"\\\\"a"';
print_r(str_getcsv($str));

outputs:

Array
(
[0] => \\a"
)

Note that $str could have been produced by fputcsv($stream, ['\\\\"a']).

[1]
<https://github.com/php/php-src/blob/php-7.3.0RC2/ext/standard/file.c#L1932-L1990>
[2]
<https://github.com/php/php-src/blob/php-7.3.0RC2/ext/standard/file.c#L2092-L2349>

--
Christoph M. Becker