Ready for review.

I did not run the tests locally and they might need to be updated.

Wondering if it might make sense to use a method like this to build a correctly escaped CSV string

function convertArrayToCSV($data) {
    $fh = fopen('php://temp', 'rw');
    fputcsv($fh, array_keys(current($data)));

    foreach ($data as $row) {
        fputcsv($fh, $row);
    }
    rewind($fh);
    $csv = stream_get_contents($fh);
    fclose($fh);

    return $csv;
}

Haven't tested it though, but could maybe prevent other issues with CSV.

This issue is in "needs review" but there has been no activity for 7 days. ping @tsteur @sgiehl @diosmosis @flamisz

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

@sgiehl There is also buildCsvString() which is only called from renderDataTable(). So creating an extra csv-rendering function for renderDataTableMap() seems a bit redundant.

May it be feasible to combine both?

Sorry. Seems I didn't have a closer look before 🙈
I guess it might simply be enough to use the formatValue method, which should automatically escape the string if needed.

I rebased onto current 4.x-dev and modified it to use formatValue(). It does seem to do the trick! :D

Thanks for updating the PR. Looks good now. Will wait until the tests finished and if all relevant tests are passing will merge it.

should we add some unit tests for regression testing?

should we add some unit tests for regression testing?

That would be nice. Maybe we could add one here: https://github.com/matomo-org/matomo/blob/4.x-dev/tests/PHPUnit/System/CsvExportTest.php#L35-L39

I guess CsvExportTest::$fixture = new TwoVisitsWithContents(); here would need to be extended as $dateTime = self::$fixture->dateTime; is currently set to a single date and not a time span?

In the test array linked above you can simply add something like this:

            array($apiToCall, array('idSite'                 => $idSite,
                                    'date'                   => Date::factory($dateTime)->toString() .','. Date::factory($dateTime)->addDay(21)->toString(),
                                    'period'                 => 'week',
                                    'format'                 => 'csv',
                                    'testSuffix'             => '_multi')),

Running the tests should then produce 2 new result files in processed folder, that can simply be moved to expected files.

I guess, this is as good as it gets for the moment. Tests are added. I'm waiting for Travis opinion on my work. Locally the tests run through.

-> ready for review

@ziegenberg Thanks for that. With those tests we actually have all we need. Tests also seem to pass 👍