-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 exporting main metrics with a period of weeks and export format CSV #17442
Conversation
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 May it be feasible to combine both? |
Sorry. Seems I didn't have a closer look before 🙈 |
I rebased onto current 4.x-dev and modified it to use |
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? |
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 |
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. |
… weeks and export format CSV Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
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 👍 |
Description:
FIXES #17441.
Escape the first column in CSV exports because it might contain a date span, and the date span will contain a comma.
If a test to prevent regressions is desired, I would need help with implementing that.
Note: As soon PHP 8 is the required minimum escaping could be limited with
str_contains()
.Review