FIX Use column names instead of display names for CSV exports

[robbieaverill]

Fixes #7081

[worikgh]

This effects version 4
I applied the change in this patch

    protected function getExportColumnsForGridField(GridField $gridField)
    {
        if ($this->exportColumns) {
            return $this->exportColumns;
        }
        /** @var GridFieldDataColumns $dataCols */
        $dataCols = $gridField->getConfig()->getComponentByType(GridFieldDataColumns::class);
        if ($dataCols) {
            return $dataCols->getColumnsHandled($gridField);
        }
        return DataObject::singleton($gridField->getModelClass())->summaryFields();
    }

Using getColumnsHandled
Reimporting the data it failed on a column that is an extension column for the data field.

[dhensby]

I feel this change isn't suitable for a patch release. We are fundamentally changing the markup of the CSV which developers who import these CSVs may well be relying upon.

I'd fix the importer rather than the exporter

[robbieaverill]

I think that's a fair point, but I also think that this is the way it "should" be fixed. Perhaps it could become a minor change?

[tractorcow]

I left a comment https://github.com/silverstripe/silverstripe-framework/issues/7081#issuecomment-351548579

I think the issue to fix is the importer not the exporter.

[worikgh]

I disagree, strongly. CSV exports are not for pretty printed reports IMO they are for data transfer. As such the column headers ought to be the SQL column names, not the display names (as the Exporter does) and the data should be the raw data not the display data (again the exporter exports data as displayed not stored)

A fundemental principal is that if I export a file I should be able to import it as exported (duplicating records, or not, and the errors associated with that are a separate issue).

As it stands the Exporter is horridly broken.

[tractorcow]

Well, the basis of my thinking is that the input should match the output, rather than that the "column title" is superior. We can't really switch the output format without breaking semver.

We can add this feature in with a flag, as long as the default value matches the current status quo.

[tractorcow]

@robbieaverill if you like, should we port this to 4.1, but with the config option?

[robbieaverill]

What about putting it into 3.7?

[tractorcow]

I'm not 100% sure 3.7.0 is going to be a high release priority for us. 4.1.0 will be out sooner, so I'd focus there. Think of 3.7.0 as being the 2.5 of 3.x. ;P

[robbieaverill]

Yeah sure. It's not a particularly high impact bug, so we could live with it in SS3 and "fix" it in SS4. I'll look at it when I get a chance :)

[dhensby]

@worikgh you are correct that CSV should be a data transfer format, but I'm not sure how that can cause you to disagree that the issue is with the importer.

The data we export has a defined format that everyone who handles CSVs from SilverStripe is used to and has set up the data transfer tools to work with. Changing that now will indisputably break any software or tools that make assumptions about column names.

Breaking the export markup because our importer makes incorrect assumptions about the export structure is not a logical conclusion.

Thought of another way, if you had a client with a data importer and it wasn't working because it expected the "Surname" column to be "LastName", would you open a PR to SilverStripe to change the exporter or would you tell your client they need to fix their importer?

The only sensible approach here is to make sure our importer can handle the data we export. We can consider using DB column names in the export in 5.x, but the real main thing is that the column names are consistent, not that they are a direct mirroring of the DB column name.