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.

Sign up for GitHub

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

[9.x] Optionally cascade thrown Flysystem exceptions #41308

Merged
merged 7 commits into from
Mar 3, 2022
Merged

[9.x] Optionally cascade thrown Flysystem exceptions #41308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
85c505f
Optionally cascade thrown Flysystem exceptions
driesvints Mar 3, 2022
c005788
Apply fixes from StyleCI
StyleCIBot Mar 3, 2022
4d18aff
wip
driesvints Mar 3, 2022
dfebc33
wip
driesvints Mar 3, 2022
1e11a68
use helper - rename option
taylorotwell Mar 3, 2022
dc22a28
wip
driesvints Mar 3, 2022
33749ab
Revert "wip"
driesvints Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions src/Illuminate/Filesystem/FilesystemAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ public function get($path)
try {
return $this->driver->read($path);
} catch (UnableToReadFile $e) {
//
throw_if($this->throwsExceptions(), $e);
}
}

Expand Down Expand Up @@ -330,6 +330,8 @@ public function put($path, $contents, $options = [])
? $this->driver->writeStream($path, $contents, $options)
: $this->driver->write($path, $contents, $options);
} catch (UnableToWriteFile $e) {
throw_if($this->throwsExceptions(), $e);

return false;
}

Expand Down Expand Up @@ -405,6 +407,8 @@ public function setVisibility($path, $visibility)
try {
$this->driver->setVisibility($path, $this->parseVisibility($visibility));
} catch (UnableToSetVisibility $e) {
throw_if($this->throwsExceptions(), $e);

return false;
}

Expand Down Expand Up @@ -461,6 +465,8 @@ public function delete($paths)
try {
$this->driver->delete($path);
} catch (UnableToDeleteFile $e) {
throw_if($this->throwsExceptions(), $e);

$success = false;
}
}
Expand All @@ -480,6 +486,8 @@ public function copy($from, $to)
try {
$this->driver->copy($from, $to);
} catch (UnableToCopyFile $e) {
throw_if($this->throwsExceptions(), $e);

return false;
}

Expand All @@ -498,6 +506,8 @@ public function move($from, $to)
try {
$this->driver->move($from, $to);
} catch (UnableToMoveFile $e) {
throw_if($this->throwsExceptions(), $e);

return false;
}

Expand Down Expand Up @@ -545,7 +555,7 @@ public function readStream($path)
try {
return $this->driver->readStream($path);
} catch (UnableToReadFile $e) {
//
throw_if($this->throwsExceptions(), $e);
}
}

Expand All @@ -557,6 +567,8 @@ public function writeStream($path, $resource, array $options = [])
try {
$this->driver->writeStream($path, $resource, $options);
} catch (UnableToWriteFile $e) {
throw_if($this->throwsExceptions(), $e);

return false;
}

Expand Down Expand Up @@ -753,6 +765,8 @@ public function makeDirectory($path)
try {
$this->driver->createDirectory($path);
} catch (UnableToCreateDirectory $e) {
throw_if($this->throwsExceptions(), $e);

return false;
}

Expand All @@ -770,6 +784,8 @@ public function deleteDirectory($directory)
try {
$this->driver->deleteDirectory($directory);
} catch (UnableToDeleteDirectory $e) {
throw_if($this->throwsExceptions(), $e);

return false;
}

Expand Down Expand Up @@ -838,6 +854,16 @@ public function buildTemporaryUrlsUsing(Closure $callback)
$this->temporaryUrlCallback = $callback;
}

/**
* Determine if Flysystem exceptions should be thrown.
*
* @return bool
*/
protected function throwsExceptions(): bool
{
return (bool) ($this->config['throw'] ?? false);
}

/**
* Pass dynamic methods call onto Flysystem.
*
Expand Down
50 changes: 50 additions & 0 deletions tests/Filesystem/FilesystemAdapterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
use League\Flysystem\Filesystem;
use League\Flysystem\Ftp\FtpAdapter;
use League\Flysystem\Local\LocalFilesystemAdapter;
use League\Flysystem\UnableToReadFile;
use League\Flysystem\UnableToWriteFile;
use Mockery as m;
use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpFoundation\StreamedResponse;
Expand Down Expand Up @@ -393,4 +395,52 @@ public function testTemporaryUrlWithCustomCallback()
$filesystemAdapter->temporaryUrl($path, $expiration, $options)
);
}

public function testThrowExceptionForGet()
{
$adapter = new FilesystemAdapter($this->filesystem, $this->adapter, ['throw' => true]);

try {
$adapter->get('/foo.txt');
} catch (UnableToReadFile $e) {
$this->assertTrue(true);

return;
}

$this->fail('Exception was not thrown.');
}

public function testThrowExceptionsForReadStream()
{
$adapter = new FilesystemAdapter($this->filesystem, $this->adapter, ['throw' => true]);

try {
$adapter->readStream('/foo.txt');
} catch (UnableToReadFile $e) {
$this->assertTrue(true);

return;
}

$this->fail('Exception was not thrown.');
}

/** @requires OS Linux|Darwin */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get this test to work on Windows but I'd assume it's related to how I'm creating the directory so I expect exceptions throwing works fine on it 😅

Copy link
Contributor

@Jubeki Jubeki Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May have something todo with the different directory separator:
Linux/MacOS: /
Windows: \

Maybe use: DIRECTORY_SEPARATOR

EDIT:
Reference: https://www.php.net/manual/en/dir.constants.php

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but it didn't work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah seems like windows ignores the permissions which are set by mkdir: https://www.php.net/manual/en/function.mkdir.php

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could create a 400 permission read-only file and attempt to overwrite it. I verified this passes on Windows 10:

public function testThrowExceptionsForPut()
{
    $this->filesystem->write('foo.txt', 'Hello World');

    chmod(__DIR__.'/tmp/foo.txt', 0400);

    $adapter = new FilesystemAdapter($this->filesystem, $this->adapter, ['throws_exceptions' => true]);

    try {
        $adapter->put('foo.txt', 'Hello World!');

        $this->fail('UnableToWriteFile exception was not thrown.');
    } catch (UnableToWriteFile $e) {
        $this->assertTrue(true);
    } finally {
        chmod(__DIR__.'/tmp/foo.txt', 0600);
    }
}

It needs chmod(__DIR__.'/tmp/foo.txt', 0600) before the tearDown() deletion of ./tmp which will fail if the subdirectory contains a read-only file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derekmd that didn't seem to work so I'm reverting that. Keeping tests on Linux/macOS for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it failed because the wip commit used old config key throws_exceptions after Taylor renamed it to throw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah could be yeah. Gonna leave it now myself but would appreciate a PR if anyone wants to.

public function testThrowExceptionsForPut()
{
mkdir(__DIR__.'/tmp/bar', 0600);

$adapter = new FilesystemAdapter($this->filesystem, $this->adapter, ['throw' => true]);

try {
$adapter->put('/bar/foo.txt', 'Hello World!');
} catch (UnableToWriteFile $e) {
$this->assertTrue(true);

return;
}

$this->fail('Exception was not thrown.');
}
}