fputcsv improvements to allow RFC 4180 compliance #197
Conversation
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.
|
We definitely need a test for that. Could you add one? |
|
I'm actually all thumbs with .phpt syntax, but I'll see if I can outsource it. :) |
| 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"); |
There was a problem hiding this comment.
Should be "Escape string must be a single character"
|
Any news? |
|
No - if you know of anyone who would be willing to write a .phpt for it, feel free to pass it along to them. |
|
Have a look here: http://blog.doh.ms/2009/08/23/writing-tests-with-phpt/ |
|
@tml ping |
|
@lstrojny I have a test coming your way. branching and submitting a pull request |
|
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! |
|
@tml @theoreticaLee could you both coordinate and consolidate a single PR including the test? |
|
@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. |
|
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. |
…ation for new pull request
|
@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. |
| @@ -817,7 +817,7 @@ static void file_globals_dtor(php_file_globals *file_globals_p TSRMLS_DC) | |||
| if (p_len > 64) { | |||
| p[63] = '\0'; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@theoreticaLee cleaned up stray tab
|
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. |
|
@tml any news? |
|
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. |
…parameter is allowed.
|
I cannot reproduce the failed builds at this point; the tests failing are: PostgreSQL notice function [ext/pgsql/tests/09notice.phpt] Neither of them appears to bear the slightest relationship to my fputcsv changes. |
|
Looks like doesn't fail for me anymore, so I guess it's ok. |
|
Comment on behalf of stas at php.net: merged |
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.