Provide an ASCII fallback for multibyte filenames

[kamil4]

Fixes #1452

An equivalent of an old fix we had implemented for albums in #384

[codecov]

Codecov Report

Merging #1453 (8da77f2) into master (fe173e4) will decrease coverage by 0.25%.
The diff coverage is 100.00%.

[nagmat84]

The fix looks good. I also compared it with the analogous code for the albums to ensure consistency, i.e.

Lychee/app/Actions/Album/Archive.php

Lines 72 to 76 in fe173e4

	$disposition = HeaderUtils::makeDisposition(
	HeaderUtils::DISPOSITION_ATTACHMENT,
	$zipTitle . '.zip',
	mb_check_encoding($zipTitle, 'ASCII') ? '' : 'Album.zip'
	);

Let's see, if I find the time to add a test case this evening so that we won't tumble across this regression a third time. Or you can add it. It shouldn't be too difficult. Just upload a photo, rename it to something with multi-byte characters in it and then download it. Optimally, we also add a test for an album, too, while we are on it.

[ildyria]

LGTM.

[kamil4]

Strictly speaking, we don't need a new image file with a multi-byte character; we could just upload one of the existing ones and then change the title. But I guess there may be value in testing multi-byte uploads as well...

I agree with @nagmat84 that we should also add a test for multi-byte album downloads.

[kamil4]

Looking through the testsuite, I could easily add a relevant album test by adding just a couple of lines to testManyFunctionsAtOnce, around:

Lychee/tests/Feature/PhotosOperationsTest.php

Line 139 in ef102c4

$this->albums_tests->download($albumID);

Is that considered acceptable? There's a comment at the start of that method that says: Preferably, all the tested actions should be seperated into individual tests.

[nagmat84]

I will push some commits myself which address my remarks.

[nagmat84]

Strictly speaking, we don't need a new image file with a multi-byte character;

That's what I thought of, too.

we could just upload one of the existing ones and then change the title. But I guess there may be value in testing multi-byte uploads as well...

Agreed. And as @ildyria has already added a sample file, I'm fine with that.

[nagmat84]

Looking through the testsuite, I could easily add a relevant album test by adding just a couple of lines to testManyFunctionsAtOnce, around:

Lychee/tests/Feature/PhotosOperationsTest.php

Line 139 in ef102c4

$this->albums_tests->download($albumID);

Is that considered acceptable? There's a comment at the start of that method that says: Preferably, all the tested actions should be seperated into individual tests.

I would like to avoid that. When I added a lot of test as part of #1351, I needed to get this "god test" out of my way but did not want to lose what has already been there. In the long run this god test should be split, hence I would prefer not to add to that mess.

Let's see if I can create a test this evening.

[nagmat84]

Fixed all my concerns myself. I also added a test to download an archive with a multi-byte title. Feel free to merge and resolve my remarks. I only left them open for you such that they are more easily to read.