Skip to content
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

Make download headers customizable #2162

Merged
merged 1 commit into from Apr 17, 2019

Conversation

tpraxl
Copy link
Contributor

@tpraxl tpraxl commented Apr 16, 2019

Requirements

Please take note of our contributing guidelines: https://laravel-excel-docs.dev/docs/3.0/getting-started/contributing https://docs.laravel-excel.com/3.1/getting-started/contributing.html
Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.

Mark the following tasks as done:

  • Checked the codebase to ensure that your feature doesn't already exist.
  • Checked the pull requests to ensure that another person hasn't already submitted the feature or fix.
  • Adjusted the Documentation. <- will be done as soon as you signal to merge the pull request (done)
  • Added tests to ensure against regression.

Description of the Change

Before, CSV downloads would be sent with a mime type of 'text/plain'.
This made Firefox propose to open the file with a text editor instead of LibreOffice / Excel.

This change allows to send custom headers, including ['Content-Type' => 'text/csv'], to fix this pragmatically.

Why Should This Be Added?

  • I think, serving csv as text/plain instead of text/csv is a problem, it's most likely a common stake holder request to change the mime type
  • It's hard to find an alternative workaround (requires some digging)
  • The workaround without this fix would be a bit longwinded:
	$response = Excel::download(
            new MyExport($data), "{$filename}.csv",
            \Maatwebsite\Excel\Excel::CSV
        );

	// send the correct mime type
        $response->headers->set('Content-Type', 'text/csv');

        return $response;

With this fix:

	return Excel::download(
	    new MyExport($data), "{$filename}.csv",
            \Maatwebsite\Excel\Excel::CSV,
	    ['Content-Type' => 'text/csv']
        );

Benefits

Shorter code, no need to find a workaround. Will be documented.

Possible Drawbacks

Doesn't break the API. Doesn't change behavior. Custom headers are optional.

Adding another optional parameter might make the API more complex than it needs to be. Maintaining the API with another optional parameter might be harder.

Verification Process

What process did you follow to verify that your change has the desired effects?

  • How did you verify that all new functionality works as expected?
  • How did you verify that all changed functionality works as expected?

Tests.

  • How did you verify that the change has not introduced any regressions?

Running all existing tests.

Applicable Issues

none, afaik.

[Edit]
updated the check mark for "Adjusted the Documentation"

Before, CSV downloads would be sent with a mime type of 'text/plain'.
This made Firefox propose to open the file with a text editor instead of LibreOffice / Excel.

The workaround without this fix would be a bit longwinded:

```php
	$response = Excel::download(
            new MyExport($data), "{$filename}.csv",
            \Maatwebsite\Excel\Excel::CSV
        );

	// send the correct mime type
        $response->headers->set('Content-Type', 'text/csv');

        return $response;
```

With this fix:

```php
	return Excel::download(
	    new MyExport($data), "{$filename}.csv",
            \Maatwebsite\Excel\Excel::CSV,
	    ['Content-Type' => 'text/csv']
        );
```

Doesn't break the API. Doesn't change behavior.  Custom headers are optional.
@patrickbrouwers
Copy link
Member

@tpraxl thanks for your contribution. I'm fine with adding this. Can you PR the docs?

@tpraxl
Copy link
Contributor Author

tpraxl commented Apr 17, 2019

@patrickbrouwers Great. Documentation PR is done: SpartnerNL/laravel-excel-docs#54

@patrickbrouwers
Copy link
Member

@tpraxl even though I don't mind having the headers param, I'm thinking that we maybe can always add the correct content-type when downloading. So based on the writer type add the correct content-type to the headers. Would you be willing to adjust your PR to add that?

@tpraxl
Copy link
Contributor Author

tpraxl commented Apr 17, 2019

@patrickbrouwers I originally thought about it when I dug into the code and avoided it for these reasons:

  • That would be more like a breaking change for those who actually do want to download csv files as text/plain
  • Also, there seem to be multiple valid / common options for the csv mime type
  • Setting the mime type dependent on the $writerType might get a bit messy
  • What about TSV and the other formats – might raise discussions that block the simple solution

My proposal:

  1. Please merge this PR, so that we are able to easily workaround the issue. The docs now have helpful hints next to the CSV download example.
  2. Maybe I'll look into it and create a 2nd PR for the new default mime type. But that should be a breaking change for the next major version.

Just a warning: I might not find the time or may become distracted from writing the 2nd PR. Actually, I'm pretty sure, I won't create it, because of focus shift.

@patrickbrouwers patrickbrouwers merged commit e1816f7 into SpartnerNL:3.1 Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants