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

[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

merged 7 commits into from
May 17, 2019

Conversation

4n70w4
Copy link
Contributor

@4n70w4 4n70w4 commented May 16, 2019

Generate and pass $filenameFallback param to \Symfony\Component\HttpFoundation\HeaderUtils::makeDisposition().

Problem: downloading files with Cyrillic names does not work. Test complete.
\Illuminate\Tests\Filesystem\FilesystemAdapterTest::testDownloadNonAsciiFilename()

tests/Filesystem/FilesystemAdapterTest.php Outdated Show resolved Hide resolved
@driesvints driesvints changed the title fix exception: The filename fallback must only contain ASCII characters. [5.8] Fix exception: The filename fallback must only contain ASCII characters. May 16, 2019
@4n70w4
Copy link
Contributor Author

4n70w4 commented May 16, 2019

@driesvints thanks, I improved the test!

@4n70w4
Copy link
Contributor Author

4n70w4 commented May 16, 2019

Related: #2703 #2305 #28369

@taylorotwell
Copy link
Member

$name defaults to null. So you are passing Str::ascii(null) potentially? I feel like we aren't thinking this issue through every time we get an issue or PR about it.

@4n70w4
Copy link
Contributor Author

4n70w4 commented May 16, 2019

@taylorotwell thank you, I did not think about it. I fix it!

@@ -138,7 +138,8 @@ public function response($path, $name = null, array $headers = [], $disposition
{
$response = new StreamedResponse;

$disposition = $response->headers->makeDisposition($disposition, $name ?? basename($path));
$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.

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.

5 participants