Skip to content

Commit

Permalink
Fixed issue: Better Excel compatibility for multiline response fields
Browse files Browse the repository at this point in the history
  • Loading branch information
c-schmitz committed Feb 1, 2018
1 parent 90180f3 commit 7b91558
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions application/helpers/admin/export/CsvWriter.php
Expand Up @@ -27,7 +27,7 @@ class CsvWriter extends Writer
function __construct()
{
$this->output = '';
$this->separator = ',';
$this->separator = ';';
$this->hasOutputHeader = false;
}

Expand All @@ -39,6 +39,7 @@ public function init(SurveyObj $survey, $sLanguageCode, FormattingOptions $oOpti

if ($oOptions->output == 'file') {
$this->file = fopen($this->filename, 'w');
fwrite($this->file,"\xEF\xBB\xBF"); // Write UTF-8 Byte Order Mark (BOM)
}

}
Expand All @@ -60,7 +61,7 @@ protected function outputRecord($headers, $values, FormattingOptions $oOptions)
$index++;
}
//Output the header...once and only once.
$sRecord .= implode($this->separator, $headers).PHP_EOL;
$sRecord .= implode($this->separator, $headers)."\r\n";
}
$this->hasOutputHeader = true;
}
Expand All @@ -70,7 +71,7 @@ protected function outputRecord($headers, $values, FormattingOptions $oOptions)
$values[$index] = $this->csvEscape($value);
$index++;
}
$sRecord .= implode($this->separator, $values).PHP_EOL;
$sRecord .= implode($this->separator, $values)."\r\n";
if ($oOptions->output == 'display') {
echo $sRecord;
$this->output = '';
Expand Down

9 comments on commit 7b91558

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awfull, csv is not csv anymore !

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@c-schmitz
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it wasn't CSV before and it now it's still not CSV ;) - just that Excel properly reads it now.

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this broke all automatic usage of this file …

If move to clean plugin : can set it in option, but default must be , and https://www.ietf.org/rfc/rfc4180.txt compliant (OK for the CRLF)

@c-schmitz
Copy link
Contributor Author

@c-schmitz c-schmitz commented on 7b91558 Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4180 is just an RFC, not a fixed standard. The document itself states "While there are various specifications and implementations for the CSV format (for ex. [4], [5], [6] and [7]), there is no formal
specification in existence, which allows for a wide variety of interpretations of CSV files."

Obviously there is no fixed standard for the separator character, either.

Anyway, I have not heard about any complaints since I changed this several months ago - but before my change I heard a truckload of complaints from users not being able to import it in Excel with a few clicks.

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don(t have a lot of user using 3.X, and don't find the issue https://bugs.limesurvey.org/view.php?id=13747

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS : can i create a pull request to add a new CSV (excel compatible) format and revert this one ?

@DeveloperChris
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an export to excel function already. It works very well.

Why would you break the default CSV format ?
CSV stands for "COMMA Separated values" Not Semicolon Separated Values

@TonisOrmisson
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit simple to say that replacing the comma with a semicolon will make Excel read the CSV properly now.

Excel will read the CSV and use the column delimiter depending on your regional settings. Some regions use semicolon, some use comma. By replacing one with the other you will fix it by default for some regions and break it for the others.

A logical way to solve this wold be to have the delimiter choice as a setting as many systems have it.

Choosing one over the other is never a "correct" option - it just depends what most of your user's location settings are.

Please sign in to comment.