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

File upload: Take ASCII conversion rules of current language and/or user language into account #4972

Closed
lukasbestle opened this issue Dec 27, 2022 · 6 comments · Fixed by #6606
Assignees
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby

Comments

@lukasbestle
Copy link
Member

Description

Kirby only uses the default conversion rules when converting filenames on upload. This breaks when files with names in another language are uploaded.

Expected behavior

Kirby should take the current language into account. We need to determine whether this should be the current content language, the current user language or both.

To reproduce

  1. Name a file 日本語.pdf
  2. Upload the file to a files section
  3. Error "You are not allowed to upload invisible files" because all characters are stripped by the default ASCII rules

Additional context

getkirby/getkirby.com#1817

@lukasbestle lukasbestle added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Dec 27, 2022
@distantnative
Copy link
Member

I would think that the current user language has the higher chances to match the language of uploaded file names.

@lukasbestle
Copy link
Member Author

Thinking about it again, I think you are right. The content language does not make much sense as the filenames are independent from the language. I.e. if you upload a file while the content language is set to language A, the filename will be exactly the same once you switch to language B.

So we can only rely on the user language here.

@distantnative
Copy link
Member

I would think this is the line where we would need to add the change: https://github.com/getkirby/kirby/blob/main/src/Cms/FileActions.php#L183

However, I am wondering whether it would make sense to use File\Filename here as well, just without any attributes in the template string. And then add the logic to File\Filename instead, which should call F::safeName probably with a new second parameter for additional rules?

@distantnative
Copy link
Member

Something like this for File\Filename::sanitizeName():

// temporarily store language rules
$rules = Str::$language;
$kirby = App::instance(null, true);

// if current user with language, use those rules for `Str` class
if ($language = $kirby?->language($kirby?->translation()->code())) {
	Str::$language = $language->rules();
}

// sanitize name
$name = F::safeName($name);

// reset language rules
Str::$language = $rules;
return $name;

@lukasbestle
Copy link
Member Author

Sounds about right. I think we should also harmonize the logic of F::safeName() and Filename::sanitize*(). They currently behave a bit differently.

@distantnative distantnative self-assigned this Oct 8, 2023
@distantnative
Copy link
Member

Harmonizing methods: #5760

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants