fisharebest / webtrees

Online genealogy
https://webtrees.net
GNU General Public License v3.0
491 stars 302 forks source link

Control panel - manage media with server on windows #3890

Closed ddrury closed 3 years ago

ddrury commented 3 years ago

Selecting 'Unused files' returns all files.

I think this is due to the fact that in app\Http\RequestHandlers\ManageMediaData.php (lines 213/214) the collections of $disk_files and $db_files differ in their directory separators so when a diff is computed all items are returned.

fisharebest commented 3 years ago

The file-system abstraction layer is supposed to normalize this, and use / consistently.

Do you have any debug/testing to confirm this? I might need to raise this with the library maintainers...

ddrury commented 3 years ago

Changing the code to

$disk_files = $this->media_file_service->allFilesOnDisk($data_filesystem, $media_folder, $subfolders === 'include')
    ->map(static function (string $file) {
            return str_replace("\\", "/", $file);
      });

solves it

fisharebest commented 3 years ago

Can you create a script test.php, and run it from the command line:

<?php
include 'vendor/autoload.php';

mkdir('test/foo', 0777, true);
touch('test/foo/bar');

$adapter = new League\Flysystem\Local\LocalFilesystemAdapter('test');
$filesystem = new League\Flysystem\Filesystem($adapter);
$contents = $filesystem->listContents('/', true)->map(fn ($attributes) => $attributes->path())->toArray();
var_dump($contents);

What output do you get?

ddrury commented 3 years ago

array (size=2) 0 => string 'foo' (length=3) 1 => string 'foo\bar' (length=7)

fisharebest commented 3 years ago

Do you have a webtrees 2.0 system around? If so, can you run the following script there:

(It uses the older version of the library, and I want to confirm that it is a library change rather than an issue with our code)

<?php
include 'vendor/autoload.php';

mkdir('test/foo', 0777, true);
touch('test/foo/bar');

$adapter = new League\Flysystem\Adapter\Local('test');
$filesystem = new League\Flysystem\Filesystem($adapter);
$contents = $filesystem->listContents('/', true);
$contents = array_map(fn ($x) => $x['path'], $contents);
var_dump($contents);
ddrury commented 3 years ago

I'll have to set one up - bear with me

ddrury commented 3 years ago

Hm, test.php throws an error in webtrees 2.0.16 Fatal error: Uncaught Error: Class "League\Flysystem\Local\LocalFilesystemAdapter" not found on line 7 of test.php but for what it's worth the Unused files list is empty so I presume the diff is working

fisharebest commented 3 years ago

I posted two very similar scripts. The second one is for 2.0.

ddrury commented 3 years ago

array(2) { [0]=> string(3) "foo" [1]=> string(7) "foo/bar" }

fisharebest commented 3 years ago

https://github.com/thephpleague/flysystem/issues/1311

fisharebest commented 3 years ago

@ddrury - can you test something for me.

In composer.json, find the entry for league/flysystem, change the version from ~2.0 to 2.x-dev, and then run composer update and composer install --no-dev.

This should hopefully fix the issue.

ddrury commented 3 years ago

Yep that fixes it. (And I didn't think your remarks were smart - DeJonge needs to lighten up)

frankdejonge commented 3 years ago

Flysystem 2.0.7 is tagged which ships this fix. And @ddrury, as you might understand I disagree. Saying you need an abstraction around Flysystem for this, while it would have been fixed with one additional ->map call (literally a one-liner) is an unneeded exaggeration that I don't care for. Besides that, no abstraction is perfect, because perfect abstractions don't exist. 80/20 solutions are pretty much the best you can achieve, so suggesting something missed the point because of one error is (once again) an unneeded exaggeration.