@ziegenberg opened this Pull Request on April 9th 2021 Contributor

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

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review was done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@ziegenberg commented on April 9th 2021 Contributor

Ready for review.

@ziegenberg commented on April 9th 2021 Contributor

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

@sgiehl commented on April 9th 2021 Member

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.

@github-actions[bot] commented on June 4th 2021 Contributor

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

@github-actions[bot] commented on July 17th 2021 Contributor

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

@ziegenberg commented on July 19th 2021 Contributor

@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?

@sgiehl commented on July 19th 2021 Member

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.

@ziegenberg commented on July 20th 2021 Contributor

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

@sgiehl commented on July 20th 2021 Member

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

@ziegenberg commented on July 20th 2021 Contributor

should we add some unit tests for regression testing?

@sgiehl commented on July 20th 2021 Member

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

@ziegenberg commented on July 20th 2021 Contributor

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?

@sgiehl commented on July 20th 2021 Member

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.

@ziegenberg commented on July 20th 2021 Contributor

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

@sgiehl commented on July 20th 2021 Member

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

This Pull Request was closed on July 20th 2021
Powered by GitHub Issue Mirror