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.

Sign up for GitHub

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

[5.8] Fix exception: The filename fallback must only contain ASCII characters. #28551

Merged
merged 7 commits into from
May 17, 2019
Merged

[5.8] Fix exception: The filename fallback must only contain ASCII characters. #28551

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
faf2936
add test for exception: The filename fallback must only contain ASCII…
4n70w4 May 16, 2019
b50fc20
fix exception: The filename fallback must only contain ASCII characters.
4n70w4 May 16, 2019
9271738
add test for content-disposition header and filename*=utf-8
4n70w4 May 16, 2019
254c0b1
add test case for empty $name
4n70w4 May 16, 2019
8df7498
refactorung
4n70w4 May 16, 2019
0b3af53
cutout setlocale
4n70w4 May 16, 2019
4fe69b4
rename $target_name to $targetName
4n70w4 May 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactorung
  • Loading branch information
@4n70w4
4n70w4 committed May 16, 2019
commit 8df7498e6352e54f1b50d3074ab92b140b4a14b9
3 changes: 2 additions & 1 deletion src/Illuminate/Filesystem/FilesystemAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ public function response($path, $name = null, array $headers = [], $disposition
{
$response = new StreamedResponse;

$disposition = $response->headers->makeDisposition($disposition, $name ?? basename($path), Str::ascii($name));
$target_name = $name ?? basename($path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$target_name camelCase 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fragkp continuous-integration/styleci/pr says that everything is ok. camelCase is this private requirement?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why stylci is not failing. camelCase is the default for variable names in laravel. You could check this in all other files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StyleCI cannot fail for those cases, because it is not possible to fix such cases without breaking dynamic code. There are also various special cases where snake case is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I fixed it.
But need to fix the automatic codestyle verification.
External developers don't know your local codestyle rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But need to fix the automatic codestyle verification.

As I said, it is not broken. It is inherently not possible to safely rename variables.

$disposition = $response->headers->makeDisposition($disposition, $target_name, Str::ascii($target_name));

$response->headers->replace($headers + [
'Content-Type' => $this->mimeType($path),
Expand Down