Skip to content

fputcsv improvements to allow RFC 4180 compliance #197

Closed
wants to merge 9 commits into from

6 participants

@tml
tml commented Sep 17, 2012

Allows users to assert that something other than the backslash
should be considered an escape char; also follows the RFC 4180
recommendation that fields containing a " be enclosed.

@tml tml Expose fputcsv's escape_char to userland
Allows users to assert that something other than the backslash
should be considered an escape char; also follows the RFC 4180
recommendation that fields containing a " be enclosed.
49f1ef4
@lstrojny

We definitely need a test for that. Could you add one?

@tml
tml commented Sep 19, 2012

I'm actually all thumbs with .phpt syntax, but I'll see if I can outsource it. :)

@lstrojny lstrojny commented on the diff Sep 19, 2012
ext/standard/file.c
@@ -1839,6 +1840,17 @@ static const char *php_fgetcsv_lookup_trailing_spaces(const char *ptr, size_t le
enclosure = *enclosure_str;
}
+ if (escape_str != NULL) {
+ if (escape_str_len < 1) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "escape must be a character");
+ RETURN_FALSE;
+ } else if (escape_str_len > 1) {
+ php_error_docref(NULL TSRMLS_CC, E_NOTICE, "escape must be a single character");
@lstrojny
lstrojny added a note Sep 19, 2012

Should be "Escape string must be a single character"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lstrojny lstrojny commented on the diff Sep 19, 2012
ext/standard/file.c
@@ -1839,6 +1840,17 @@ static const char *php_fgetcsv_lookup_trailing_spaces(const char *ptr, size_t le
enclosure = *enclosure_str;
}
+ if (escape_str != NULL) {
+ if (escape_str_len < 1) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "escape must be a character");
@lstrojny
lstrojny added a note Sep 19, 2012

Message should be "Escape string must be a character"

@LawnGnome
LawnGnome added a note Jan 15, 2013

This and line 1848 are fine — the messages are consistent with fgetcsv() and the other warnings in the function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lstrojny
lstrojny commented Dec 2, 2012

Any news?

@tml
tml commented Dec 3, 2012

No - if you know of anyone who would be willing to write a .phpt for it, feel free to pass it along to them.

@lstrojny

@tml ping

@theoreticaLee

@lstrojny I have a test coming your way. branching and submitting a pull request

@theoreticaLee

@tml @lstrojny

TML, hey, It's error_code from irc. I just submitted a pull request https://github.com/tml/php-src/pull/1 on your repo like we talked about on monday.

Either hit me up on irc or here if you want me to work further on it.

thanks guys!

@lstrojny

@tml @theoreticaLee could you both coordinate and consolidate a single PR including the test?

@LawnGnome

@lstrojny It's OK, I'm about to make this even more annoying for everyone — it needs a rebase after the fix for https://bugs.php.net/bug.php?id=43225, and SplFileObject::fputcsv() also needs to have an escape argument added to keep it in line with fputcsv().

It turned out that @theoreticaLee and I were working on this simultaneously unbeknownst to each other — I'll consolidate the commits from @tml and @theoreticaLee together with my own changes for SplFileObject and the aforementioned rebase and commit it.

@LawnGnome

Actually, forget what I said about rebasing — the #43225 fix isn't right, so I've reverted it for now, since I can't spend any more time on it at present.

@tml @theoreticaLee As @lstrojny said, if you guys can consolidate this, that'd be grand.

My earlier note about SplFileObject::fputcsv() stands, though.

@tml
tml commented Jan 15, 2013

@LawnGnome Thank you for finding that - it was why I couldn't manage to get a solid .phpt out of this, but I couldn't find the bottom of the stack of changes. As always, your heroic efforts are greatly appreciated.

@lstrojny, the test case from @theoreticaLee has been merged into this pull request; as far as I'm concerned, @LawnGnome and @theoreticaLee need the credit for this patch; I just happened to code monkey it, they did the heavy lifting.

@tml tml commented on the diff Jan 15, 2013
ext/standard/file.c
@@ -817,7 +817,7 @@ static void file_globals_dtor(php_file_globals *file_globals_p TSRMLS_DC)
if (p_len > 64) {
p[63] = '\0';
}
-
+
@tml
tml added a note Jan 15, 2013

@theoreticaLee cleaned up stray tab

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tml tml commented on the diff Jan 15, 2013
ext/standard/file.c
@@ -1377,13 +1377,13 @@ PHPAPI int php_mkdir(char *dir, long mode TSRMLS_DC)
{
long arg1 = 0;
int oldumask;
-
+
@tml
tml added a note Jan 15, 2013

@theoreticaLee cleaned up stray tab

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tml tml commented on the diff Jan 15, 2013
ext/standard/file.c
oldumask = umask(077);
if (BG(umask) == -1) {
BG(umask) = oldumask;
}
-
+
@tml
tml added a note Jan 15, 2013

@theoreticaLee cleaned up stray tab

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev

This patch seems to break fputcsv_variation6.phpt, fputcsv_error.phpt and fputcsv_variation9.phpt. Changes to fputcsv_error.phpt are trivial but for two others I'm not sure what's wrong. @tml, please look into it.

@smalyshev

@tml any news?

@tml
tml commented Aug 19, 2013

I personally feel these tests are horribly incorrect (not to mention bloated with c/p code that doesn't make much sense at all). They're testing what PHP has historically done, rather than what makes sense (most notably, if I explicitly request delimiters and enclosures, why is PHP dropping them!?).

The line that is causing these tests to fail was FPUTCSV_FLD_CHK('"'), added because RFC 4180 2.6 says: " Fields containing line breaks (CRLF), double quotes, and commas should be enclosed in double-quotes..." (emphasis mine). However, I have no interest in a BC fight, so I've retracted the fix; now we can just brush the problem under the rug for the next 10 years.

@tml
tml commented Aug 19, 2013

I cannot reproduce the failed builds at this point; the tests failing are:

PostgreSQL notice function [ext/pgsql/tests/09notice.phpt]
Bug #32223 (8.0+) (weird behaviour of pg_last_notice using define) [ext/pgsql/tests/80_bug32223b.phpt]

Neither of them appears to bear the slightest relationship to my fputcsv changes.

@smalyshev

Looks like doesn't fail for me anymore, so I guess it's ok.

@php-pulls

Comment on behalf of stas at php.net:

merged

@php-pulls php-pulls closed this Aug 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.