-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for badly escaped values in CSV export #6973
Fix for badly escaped values in CSV export #6973
Conversation
Will fix tests/issue shortly when possible, but open for discussion |
*/ | ||
public function generateExportFileData($gridField) { | ||
$separator = $this->csvSeparator; | ||
$csvColumns = $this->getExportColumnsForGridField($gridField); | ||
$fileData = ''; | ||
|
||
$stream = fopen('php://memory', 'w'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is to put this entire structure into a try {} finally {}, so you can safely nest output buffering and cleanup in finally.
If you are writing directly to a memory stream, you shouldn't need to use the output buffering feature, should you? As I understand it, ob_start(); affects the php://output
stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue you had is that headers_sent() was getting flagged to true, even though you weren't really sending any content from this block. Maybe double check your output when downloading locally?
If you are still having issues, maybe just try writing it to a temp file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't use try {} finally {}
as that's php 5.6 and 3.x has to support 5.3.3+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't use try {} finally {} as that's php 5.6 and 3.x has to support 5.3.3+
Oh darn, that's my bad. =( You will need to mock it with a try {} catch (Exception $ex) {} then with a conditional on $ex.
Feedback still stands... also needs a rebase @zanderwar if you're still up for it. :) |
f9eb042
to
93136d3
Compare
Right, I've decided to cherry-pick the commit from #4432 because this does what we need but was applied to 4.0. I've taken out the adding of new APIs so this is semver compliant |
93136d3
to
8efc1a4
Compare
8efc1a4
to
5df1ec7
Compare
The current functionality wouldn't escape characters correctly, have switched this over to
fputcsv
and result is much better inclusive of the fact that values will only be encapsulated with quotes IF required etc.I couldn't discover why this was never used in the first place
Aside, I tried to refrain from using
ob_start()
andob_get_clean()
butstream_get_contents($stream)
had unexpected results (the page would render unstyled instead of downloading the CSV)