flysystem

flysystem copied to clipboard

Bug Report

Q	A
BC Break	no
Version	3.2.0

Summary

Filesystem::visibility() must return string, however in that function $this->adapter->visibility(...) returns a FileAttributes object, which can and may return null as return value for the visibility method. This happens for adapters that dont allow setting visibility and return a UnableToSetVisibility exception for the setVisibility method since they dont support that (f.e. spatie dropbox adapter). This leads to the exception:

League\Flysystem\Filesystem::visibility(): Return value must be of type string, null returned

How to reproduce

For me this happens when trying to register the FileSystem as stream wrapper and performing a file operation, but this could happen at any time when a library calls visibility on a registered filesystem that does not support the concept of visibility.

@jurriaanr what situation is this occurring for you? What would the expected/desired outcome be (and why that)? For me, I didn’t model this because it seems if you don’t use a filesystem that supports visibility, then trying to resolve it is not a use-case

I'm usiing the M2MTech\FlysystemStreamWrapper\FlysystemStreamWrapper to set Spatie\FlysystemDropbox\DropboxAdapter as a streamwrapper for a filepath:

        $client = new Client('mytoken');
        $adapter = new DropboxAdapter($client);
        $filesystem = new Filesystem($adapter, ['case_sensitive' => false]);

        FlysystemStreamWrapper::register('projectfolder', $filesystem, [
            FlysystemStreamWrapper::IGNORE_VISIBILITY_ERRORS => true,
        ]);

The problem is that the streamwrapper uses the following code when performing a file operation:

        try {
            $visibility = $current->filesystem->visibility($current->file);
        } catch (UnableToRetrieveMetadata $e) {
            if (!$current->ignoreVisibilityErrors()) {
                throw $e;
            }

            $visibility = Visibility::PUBLIC;
        }

As you can see they are already trying to overcome a visibility problem, but in this case $current->filesystem->visibility($current->file) returns null, as the adapter returns a new FileAttributes object which has visibility default to null

class FileAttributes implements StorageAttributes
{
    ....
    
    /**
     * @var string|null
     */
    private $visibility;
    ...
    public function __construct(
        string $path,
        ?int $fileSize = null,
        ?string $visibility = null,
        ?int $lastModified = null,
        ?string $mimeType = null,
        array $extraMetadata = []
    ) {
        $this->path = ltrim($path, '/');
        $this->fileSize = $fileSize;
        $this->visibility = $visibility;
        $this->lastModified = $lastModified;
        $this->mimeType = $mimeType;
        $this->extraMetadata = $extraMetadata;
    }

So in this case a TypeError is thrown and the code still breaks.

I think the problem is 3 fold and I am not sure where it is best to solve. I think it starts with the default in FileAttributes being null, however, the response of FileSystem->visibility() not having null as a valid outcome. The adapter is probably at fault for not returning any real visibility options, but than again, it is possible for an adapter not to have any. There is even an exception for that in the library (UnableToSetVisibility). Here is the code from the adapter:

    public function setVisibility(string $path, string $visibility): void
    {
        throw UnableToSetVisibility::atLocation($path, 'Adapter does not support visibility controls.');
    }

    /**
     * @inheritDoc
     */
    public function visibility(string $path): FileAttributes
    {
        // Noop
        return new FileAttributes($path);
    }

And than there is the streamwrapper that is aware of a problem, but doesn't fix it correctly in this case, because it doesnt catch the TypeError

@frankdejonge

@jurriaanr what situation is this occurring for you? What would the expected/desired outcome be (and why that)? For me, I didn’t model this because it seems if you don’t use a filesystem that supports visibility, then trying to resolve it is not a use-case

For me this happens when I use the mount manager and perform a copy from Dropbox to a Local filesystem.

Return value must be of type string, null returned 

#0 /app/vendor/league/flysystem/src/MountManager.php(367): League\Flysystem\Filesystem->visibility('2.27/other/READ...')
#1 /app/vendor/league/flysystem/src/MountManager.php(245): League\Flysystem\MountManager->copyAcrossFilesystem(NULL, Object(League\Flysystem\Filesystem), '2.27/other/READ...', Object(League\Flysystem\Filesystem), 'c708144349b20b7...', 'source://2.27/o...', 'target://c70814...') 
#2 /app/src/Queue/Dossier/CloudDrive/SyncFromCloudDriveJob.php(103): League\Flysystem\MountManager->copy('source://2.27/o...', 'target://c70814...') 

Perhaps Dropbox adapter should be changed to return a default. Perhaps a change could be made Flysystem. It seems inconsistent to allow ?string for visibility from the adapter and expect only a string in the filesystem interface.

edit: doing a $manager->copy('fs1://path', 'fs2://path', ['visibility' => 'public]); resolved my immediate issue.